-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add C++ toolchain selection enforcement infrastructure with user-friendly error messages #42274
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
base: main
Are you sure you want to change the base?
Conversation
- Create tools/build_config package with string_flag for toolchain_identifier - Add enforcement rule that fails build with helpful message if no toolchain selected - Update .bazelrc to enable BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN and enforce toolchain selection - Configure gcc and clang configs to set toolchain_identifier flag Co-authored-by: phlax <[email protected]>
- Remove trailing whitespace in defs.bzl - Improve docstring formatting to follow Python conventions - Enhance comments in .bazelrc to explain enforcement mechanism - Add detailed documentation to rule explaining invocation and behavior Co-authored-by: phlax <[email protected]>
.bazelrc
Outdated
|
|
||
| # Disable automatic C++ toolchain detection | ||
| # Users must explicitly select a toolchain with --config=gcc or --config=clang | ||
| build --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot remove this - we are not ready for this yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in 3e9d1ef. The enforcement mechanism remains in place but the global BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 flag has been removed.
tools/build_config/defs.bzl
Outdated
| toolchain_id = ctx.attr.toolchain_identifier[BuildSettingInfo].value | ||
|
|
||
| if toolchain_id == "": | ||
| fail(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this string to a constant outside the function - eg ERR_MSG_TOOLCHAIN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to ERR_MSG_TOOLCHAIN constant in b292a53.
tools/build_config/defs.bzl
Outdated
| doc = "The toolchain_identifier build setting to check", | ||
| ), | ||
| }, | ||
| doc = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with this -> DOC_TOOLCHAIN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to DOC_TOOLCHAIN constant in b292a53.
.bazelrc
Outdated
|
|
||
| # Disable automatic C++ toolchain detection | ||
| # Users must explicitly select a toolchain with --config=gcc or --config=clang | ||
| build --action_env=BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still havent removed this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already removed in 3e9d1ef. The current HEAD has the flag removed - only the pre-existing rbe-toolchain config retains it at line 394.
Per review feedback, removing the global setting of BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1. The enforcement mechanism and toolchain_identifier flags remain in place, but automatic toolchain detection will not be globally disabled until ready. Co-authored-by: phlax <[email protected]>
|
@copilot see my last review |
Move error message and documentation strings to module-level constants ERR_MSG_TOOLCHAIN and DOC_TOOLCHAIN for better maintainability and reusability. Co-authored-by: phlax <[email protected]>
This PR adds infrastructure to enforce explicit C++ toolchain selection using modern Bazel build settings. The enforcement mechanism is ready but not yet activated globally - automatic toolchain detection remains enabled until the team is ready to enforce toolchain selection.
Implementation
tools/build_config/BUILD: Definesstring_flagfortoolchain_identifier(default: empty string) andtoolchain_enforcementtargettools/build_config/defs.bzl: Custom Starlark rule that reads the build setting and fails with formatted error message when empty. Error message and documentation are defined as module-level constants (ERR_MSG_TOOLCHAINandDOC_TOOLCHAIN) for better maintainability and reusability..bazelrc:--//tools/build_config:enforce_toolchainto all builds (currently no-op without global flag)--config=gccand--config=clangto settoolchain_identifierflagError Message
When enforcement is activated (by setting
BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1), users building without a toolchain config will see:Design
Uses
string_flagfrom@bazel_skylibinstead of legacy--define. Derived configs (rbe-toolchain-*,docker-*) automatically inherit correct settings via Bazel config inheritance. Purely additive—no existing code modified.Activation
To activate enforcement globally, add to
.bazelrc:Note
The global
BAZEL_DO_NOT_DETECT_CPP_TOOLCHAIN=1flag is intentionally not included in this PR. The enforcement infrastructure is in place and ready, but automatic toolchain detection will continue until the team decides to enable enforcement.This pull request was created as a result of the following prompt from Copilot chat.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.