Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bazel build implementation, doesn't provide options for the compilation modes opt and debug #2395

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

NolanBrad
Copy link

Fixes #2394

The Bazel toolchain implementation does not have default args for the compilation modes opt and debug
i,e --compilation_mode=dbg and --compilation_mode=opt

Also the rule "@pico-sdk//bazel/toolchain:override_debug" provides defaults of -Og and -g3, but these are enabled by default in the toolchains, and are not changed with the above bazel cmd line --compilation_mode

From the bazel docs the defaults for opt should be -O2 and -DNDEBUG
https://bazel.build/docs/user-manual#compilation-mode

I have added 2 boolean flags
//bazel/config:PICO_COMPILATION_DEBUG_OVERRIDE
//bazel/config:PICO_COMPILATION_OPT_OVERRIDE
These can be set to true ( default is False) which will then remove the defaults from the toolchain. the user can then provide their own args with the cmd line --copts="...."

@lurch
Copy link
Contributor

lurch commented Apr 3, 2025

ping @armandomontanez for his thoughts / review

@lurch lurch added the build label Apr 3, 2025
@@ -294,3 +294,13 @@ label_flag(
name = "PICO_FREERTOS_LIB",
build_setting_default = "//bazel:empty_cc_lib",
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add docstrings above these and add them to the exclusion list in BAZEL_ONLY_ALLOWLIST here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Thanks. These have been added.

@@ -134,16 +152,27 @@ configurable_toolchain_feature(

# TODO: Make this shim unnecessary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shims aren't necessary anymore, you can just directly list opt_args and debug_args in the cc_feature rules.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Thanks. You are right, they weren't needed. Removed

@NolanBrad
Copy link
Author

I have also added for completeness the same implementations for the fastbuild compilation mode, which is the default.
But the bazel documents said -gmlt -Wl,-S (https://bazel.build/docs/user-manual#compilation-mode) which didnt make sense, nor work. So I have left the default as nothing. Which is also the same as the override.
Please check this if this new fastbuild infrastructure should be added, or removed, as at the moment it does nothing.

Thanks
Brad.

@@ -294,3 +294,21 @@ label_flag(
name = "PICO_FREERTOS_LIB",
build_setting_default = "//bazel:empty_cc_lib",
)

# PICO_BAZEL_CONFIG: PICO_COMPILATION_OPT_OVERRIDE, Override the default opt compilation mode arguments to none, type=bool, default=0, group=build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it would make sense if I knew more about bazel, but IMHO "Override the default xxxxx arguments to none" sounds kinda weird?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right, OVERRIDE could be replaced with something like REMOVE ?
Ideally I wanted to override the defaults with user defined replacements. But it quickly got beyond my bazel knowledge.
So I resorted to an override Boolean to remove the defaults, and then the user could optionally replace using
build: --copt="-Os" for example.

I'm good with changing it to REMOVE?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a commit ready to go, if PICO_COMPILATION_xxx_REMOVE_DEFS is better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants