-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Let common
ignore unsupported options
#18130
Conversation
@haxorz If time permits, could you perhaps give early feedback on the general approach taken by this PR? It doesn't come with tests or user-facing docs yet, but should otherwise be fully functional. |
.map(BlazeCommand::getClass) | ||
.flatMap(cmd -> BlazeCommandUtils.getOptions(cmd, runtime.getBlazeModules(), | ||
runtime.getRuleClassProvider()).stream()) | ||
.collect(toImmutableList()); |
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.
If we used a Set<Class>
rather than List<Class>
could we then get rid of the allowDuplicatesParsingEquivalently
and #equivalentForParsing
stuff?
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.
Good point, we should add a .distinct()
here to make the cache key smaller.
I don't think that passing to a Set
would reduce the overall complexity of this PR though: The problem isn't that option classes collide, it's that individual options can collide across classes. For example, both cquery
and query
define an --output
flag, but those flag definition are distinct and differ in certain aspects (e.g. the allowed values). However, crucially, both options accept an argument (that is, are neither boolean nor Void options) and hence parse identically, as checked by equivalentForParsing
. That's why this PR introduces an additional type of equality that is more lenient than Object#equals
for OptionDefinition
s.
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.
Thanks that wasn't clear to me. Hmm that is an annoying bit of necessary complexity of this FR.
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.
I would definitely add more comments as part of polishing this PR if you approve of the approach.
I agree that this additional complexity is annoying, but if it isn't taken care of by Bazel itself, it is pushed onto end users. I actually think that compared to the level of effort required to manually apply flags to all commands they are valid for, the complexity of OptionsParserImpl#equivalentForParsing
is comparatively contained.
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.
For example, both cquery and query define an --output flag ... However, crucially, both options
What would happen if they weren't equivalent for parsing?
However, crucially, both options accept an argument (that is, are neither boolean nor Void options) and hence parse identically
What would be wrong with two different options with the same name that were both booleans?
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.
What would happen if they weren't equivalent for parsing?
That would result in an error when all-supported
is used. This error could be caught by tests. It does restrict what options Bazel commands can provide to some extent, but I see that as necessary to support this feature.
What would be wrong with two different options with the same name that were both booleans?
That would also work. It would only error if one is a boolean and the other accepts a value.
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.
Okay I approve of the approach.
That would result in an error when all-supported is used. This error could be caught by tests. It does restrict what options Bazel commands can provide to some extent, but I see that as necessary to support this feature.
Thanks that's what I figured. Agree this restriction to the codebase is a sensible requirement for this FR. Please make this clear in a code comment.
That would also work. It would only error if one is a boolean and the other accepts a value.
Sounds good. I see that #equivalentForParsing
handles this. I was moreso questioning the phrasing of your PR comment.
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.
I'm not clear on why we're doing the equivalentForParsing
check, especially because it seems like a very loose check. For example, if some_option
accepts different values in different commands (such as two different enums), the check passes correct? What are we trying to guard against exactly?
ead88f5
to
bbf0427
Compare
I added more comments, tests and also mention @haxorz |
fmeum:if-supported |
``` ` |
just brainstorming uninvited |
@haxorz Friendly ping |
85daf39
to
ce78cf5
Compare
Fixes bazelbuild#3054 RELNOTES: Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command. Closes bazelbuild#18130. PiperOrigin-RevId: 534940403 Change-Id: I2ae268ae84de3f2b07ff3d1e36bf382bce714968
) (#18609) * Change --memory_profile_stable_heap_parameters to accept more than one GC specification Currently memory_profile_stable_heap_parameters expects 2 ints and runs that many GCs with pauses between them 2nd param. This CL doesn't change that, but allows any arbitrary number of pairs to be provided that will run the same logic for each pair. This allows experimenting with forcing longer pauses on that thread before doing the quick GCs that allow for cleaner memory measurement. PiperOrigin-RevId: 485646588 Change-Id: Iff4f17cdaae409854f99397b4271bb5f87c4c404 * Let `common` ignore unsupported options Fixes #3054 RELNOTES: Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command. Closes #18130. PiperOrigin-RevId: 534940403 Change-Id: I2ae268ae84de3f2b07ff3d1e36bf382bce714968 --------- Co-authored-by: Googler <kkress@google.com> Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Baseline: 758b44d Release Notes: + Automatic code cleanup. (#18417) + Update CODEOWNERS for 6.3.0 (#18369) + Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition). (#18388) + Add implementation deps support for Objective-C (#18372) + Update release notes scripts (#18400) + Prevent CredentialHelperEnvironment crash when invoking Bazel outside of a workspace. (#18430) + Use wall-time for credential helper invalidation (#18413) + blaze_util_posix: handle killpg failures (#18403) + Pass version to java_runtimes created by local_java_repository (#18415) + Add jsonproto option to query --output flag (#18438) + Don't eagerly flatten a `NestedSet` in `RepoMappingManifestAction` (#18419) + rules_go & rules_python are failing in Downstream CI with Bazel@HEAD (#18447) + Move credential helper setup into remote_helpers.sh so it can be reused by other shell tests. (#18453) + Wire credential helper to repository fetching. (#18429) + Updates/fixes to relnotes script (#18470) + Report percentual download progress in repository rules (#18471) + Support remote symlink outputs when building without the bytes. (#18476) + Enrich local BEP upload errors with file path and digest possible. (#18481) + Set `GTEST_SHARD_STATUS_FILE` in test setup (#18482) + Fix relnotes script (#18491) + Fix Xcode 14.3 compatibility (#18490) + Fix #18493. (#18514) + Extend the credential helper default timeout to 10s. (#18527) + Fix formatting of release notes (#18534) + Use extension rather than local names in ModuleExtensionMetadata (#18536) + [credentialhelper] Ignore all errors when writing stdin (#18540) + Improve error on invalid `-//foo` and `-@repo//foo` options (#18516) + Implement failure circuit breaker (#18541) + Actually check `TEST_SHARD_STATUS_FILE` has been touched (#18418) + Ignore hash string casing (#18414) + Error if repository name isn't supplied (#18425) + Track repo rule label attributes after the first non-existent one (#18412) + Add ServerCapabilities into RemoteExecutionClient (#18442) + RemoteExecutionService: support output_symlinks in ActionResult (#18441) + RemoteExecutionService: Action.Command to set output_paths (#18440) + Use local_termination_grace_seconds when testing LinuxSandbox availability (#18568) + Fix dangling string literal in `extension_metadata` docs (#18598) + Include actual MODULE.bazel location in stack traces (#18612) + Make cpp file extensions case sensitive again (#18552) + Fix error when script is run after the final tag is created. (#18638) + Fix WORKSPACE toolchain resolution with `--enable_bzlmod` (#18649) + Add `ActionExecutionMetadata` as a parameter to `ActionInputPrefetcher#prefetchFiles`. (#18656) + Use failure_rate instead of failure count for circuit breaker (#18559) + Update ignored_error logic for circuit_breaker (#18662) + Don't rewind the build if invocation id stays the same (#18670) + Fix potential memory leak in UI (#18659) + Test that a credential helper can supply credentials for bzlmod. (#18663) + Add flag --experimental_collect_code_coverage_for_generated_files. (#18664) + Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command. Closes #18130. (#18609) + Fix split post-processing of LLVM-based coverage (#18737) + Allow module extension usages to be isolated (#18727) + BEGIN_PUBLIC (#18729) + Declare credential helpers to be a stable feature. (#18752) + Add a new provider for injecting native libs in android_binary (#18753) + Properly handle invalid credential files (#18779) + The REPO.bazel and MODULE.bazel files are now also considered workspace boundary markers. (#18787) + Report remote execution messages as events (#18780) + Fail on isolated extension usages without imports (#18793) + Add changes to cc_shared_library from head to 6.3 (#18606) + Remove option to disable FJP. (#18791) + Update to latest turbine version (#18803) + None. None (#18808) + Wait for outputs downloads before emitting local BEP events that reference these outputs. (#18815) + Perform builtins injection for WORKSPACE-loaded bzl files. (#18819) + Fix non-declared symlink issue for local actions when BwoB. (#18817) + Make grep_includes optional inside cc_common.register_linkstamp_compile_action (#18823) + add feature on windows toolchain with right tag (#18654) + coverage_common.instrumented_files_info now has a metadata_files argument (#18838) + Download directory output for test actions (#18846) + Teach DexMapper to not separate synthetic classes from their context … (#18853) + **[Incompatible]** query --output=proto --order_output=deps now returns targets in topological order (previously there was no ordering). (#18870) + Revert "Don't eagerly flatten a `NestedSet` in `RepoMappingManifestAction` (#18419)" (#18886) + Additional source inputs can now be specified for compilation in cc_library targets using the additional_compiler_inputs attribute, and these inputs can be used in the $(location) function. Fixes #18766. (#18882) + Open-source Google test `ConvenienceSymlinkTest` (#18890) + Update Error Prone to 2.20.0 (#18885) + Check if json.gz files exist, not the gcov version. (#18889) + Lockfile updates (#18894) + handle exception instead of crashing (#18895) + Add a new provider for passing dex related artifacts in android_binary (#18899) + Prevent most side effects of yanked modules (#18908) + Restore the classic desugar tool in the Bazel 6.3.0 branch so that the Bazel Android tools can be built for 6.3.0 without breaking backwards compatibility (#18909) + Update java_tools to v12.5 (#18868) + Add ActionCacheStatistics to BEP (#18914) + Adjust --top_level_targets_for_symlinks (#18916) + Track dev/non-dev `use_extension` calls (#18918) + Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition). (#18921) + Rollforward of https://github.com/bazelbuild/bazel/commit/482d2be27ab… (#18773) + Update Android tools to 0.27.2 for fixes to DexMapper for https://gith... (#18891) + Report dev/non-dev deps imported via non-dev/dev usages (#18922) + Add reverted 'isolate' changes (#18928) + Identify isolated extensions by exported name (#18923) + test-setup.sh: Attempt to raise the original signal once more (#18932) + Ignore broken classic desugar tests (#18933) + Disable UseCorrectAssertInTests by default (#18948) + Fix VS 2022 autodetection (#18960) + Fix absolute file paths showing up in lockfiles (#18993) + Add support for isolated extension usages to the lockfile (#19008) Acknowledgements: This release contains contributions from many people at Google, as well as amishra-u, Andreas Herrmann, Andy Hamon, andyrinne12, Benjamin Lee, Benjamin Peterson, Brentley Jones, Chirag Ramani, Christopher Rydell, Daniel Wagner-Hall, Ed Schouten, Fabian Brandstetter, Fabian Meumertzheim, Greg, Ivan Golub, Jon Landis, JY Lin, Kai Zhang, Keith Smiley, kotlaja, lripoche, oquenchil, Pavan Singh, Rasrack, Son Luong Ngoc, Takeo Sawada, Vertexwahn, Xùdōng Yáng, Yannic.
Baseline: 758b44d Release Notes: + Automatic code cleanup. (#18417) + Update CODEOWNERS for 6.3.0 (#18369) + Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition). (#18388) + Add implementation deps support for Objective-C (#18372) + Update release notes scripts (#18400) + Prevent CredentialHelperEnvironment crash when invoking Bazel outside of a workspace. (#18430) + Use wall-time for credential helper invalidation (#18413) + blaze_util_posix: handle killpg failures (#18403) + Pass version to java_runtimes created by local_java_repository (#18415) + Add jsonproto option to query --output flag (#18438) + Don't eagerly flatten a `NestedSet` in `RepoMappingManifestAction` (#18419) + rules_go & rules_python are failing in Downstream CI with Bazel@HEAD (#18447) + Move credential helper setup into remote_helpers.sh so it can be reused by other shell tests. (#18453) + Wire credential helper to repository fetching. (#18429) + Updates/fixes to relnotes script (#18470) + Report percentual download progress in repository rules (#18471) + Support remote symlink outputs when building without the bytes. (#18476) + Enrich local BEP upload errors with file path and digest possible. (#18481) + Set `GTEST_SHARD_STATUS_FILE` in test setup (#18482) + Fix relnotes script (#18491) + Fix Xcode 14.3 compatibility (#18490) + Fix #18493. (#18514) + Extend the credential helper default timeout to 10s. (#18527) + Fix formatting of release notes (#18534) + Use extension rather than local names in ModuleExtensionMetadata (#18536) + [credentialhelper] Ignore all errors when writing stdin (#18540) + Improve error on invalid `-//foo` and `-@repo//foo` options (#18516) + Implement failure circuit breaker (#18541) + Actually check `TEST_SHARD_STATUS_FILE` has been touched (#18418) + Ignore hash string casing (#18414) + Error if repository name isn't supplied (#18425) + Track repo rule label attributes after the first non-existent one (#18412) + Add ServerCapabilities into RemoteExecutionClient (#18442) + RemoteExecutionService: support output_symlinks in ActionResult (#18441) + RemoteExecutionService: Action.Command to set output_paths (#18440) + Use local_termination_grace_seconds when testing LinuxSandbox availability (#18568) + Fix dangling string literal in `extension_metadata` docs (#18598) + Include actual MODULE.bazel location in stack traces (#18612) + Make cpp file extensions case sensitive again (#18552) + Fix error when script is run after the final tag is created. (#18638) + Fix WORKSPACE toolchain resolution with `--enable_bzlmod` (#18649) + Add `ActionExecutionMetadata` as a parameter to `ActionInputPrefetcher#prefetchFiles`. (#18656) + Use failure_rate instead of failure count for circuit breaker (#18559) + Update ignored_error logic for circuit_breaker (#18662) + Don't rewind the build if invocation id stays the same (#18670) + Fix potential memory leak in UI (#18659) + Test that a credential helper can supply credentials for bzlmod. (#18663) + Add flag --experimental_collect_code_coverage_for_generated_files. (#18664) + Options specified on the pseudo-command `common` in `.rc` files are now ignored by commands that do not support them as long as they are valid options for *any* Bazel command. Previously, commands that did not support all options given for `common` would fail to run. These previous semantics of `common` are now available via the new `always` pseudo-command. Closes #18130. (#18609) + Fix split post-processing of LLVM-based coverage (#18737) + Allow module extension usages to be isolated (#18727) + BEGIN_PUBLIC (#18729) + Declare credential helpers to be a stable feature. (#18752) + Add a new provider for injecting native libs in android_binary (#18753) + Properly handle invalid credential files (#18779) + The REPO.bazel and MODULE.bazel files are now also considered workspace boundary markers. (#18787) + Report remote execution messages as events (#18780) + Fail on isolated extension usages without imports (#18793) + Add changes to cc_shared_library from head to 6.3 (#18606) + Remove option to disable FJP. (#18791) + Update to latest turbine version (#18803) + None. None (#18808) + Wait for outputs downloads before emitting local BEP events that reference these outputs. (#18815) + Perform builtins injection for WORKSPACE-loaded bzl files. (#18819) + Fix non-declared symlink issue for local actions when BwoB. (#18817) + Make grep_includes optional inside cc_common.register_linkstamp_compile_action (#18823) + add feature on windows toolchain with right tag (#18654) + coverage_common.instrumented_files_info now has a metadata_files argument (#18838) + Download directory output for test actions (#18846) + Teach DexMapper to not separate synthetic classes from their context … (#18853) + **[Incompatible]** query --output=proto --order_output=deps now returns targets in topological order (previously there was no ordering). (#18870) + Revert "Don't eagerly flatten a `NestedSet` in `RepoMappingManifestAction` (#18419)" (#18886) + Additional source inputs can now be specified for compilation in cc_library targets using the additional_compiler_inputs attribute, and these inputs can be used in the $(location) function. Fixes #18766. (#18882) + Open-source Google test `ConvenienceSymlinkTest` (#18890) + Update Error Prone to 2.20.0 (#18885) + Check if json.gz files exist, not the gcov version. (#18889) + Lockfile updates (#18894) + handle exception instead of crashing (#18895) + Add a new provider for passing dex related artifacts in android_binary (#18899) + Prevent most side effects of yanked modules (#18908) + Restore the classic desugar tool in the Bazel 6.3.0 branch so that the Bazel Android tools can be built for 6.3.0 without breaking backwards compatibility (#18909) + Update java_tools to v12.5 (#18868) + Add ActionCacheStatistics to BEP (#18914) + Adjust --top_level_targets_for_symlinks (#18916) + Track dev/non-dev `use_extension` calls (#18918) + Overrides specified by non-root modules no longer cause an error, and are silently ignored instead. They were originally treated as an error to allow for the future possibility of overrides in the transitive dependency graph working together; but we've deemed that infeasible (and even if it was, it'd be so complicated and confusing to users that it would not be a good addition). (#18921) + Rollforward of https://github.com/bazelbuild/bazel/commit/482d2be27ab… (#18773) + Update Android tools to 0.27.2 for fixes to DexMapper for https://gith... (#18891) + Report dev/non-dev deps imported via non-dev/dev usages (#18922) + Add reverted 'isolate' changes (#18928) + Identify isolated extensions by exported name (#18923) + test-setup.sh: Attempt to raise the original signal once more (#18932) + Ignore broken classic desugar tests (#18933) + Disable UseCorrectAssertInTests by default (#18948) + Fix VS 2022 autodetection (#18960) + Fix absolute file paths showing up in lockfiles (#18993) + Add support for isolated extension usages to the lockfile (#19008) Acknowledgements: This release contains contributions from many people at Google, as well as amishra-u, Andreas Herrmann, Andy Hamon, andyrinne12, Benjamin Lee, Benjamin Peterson, Brentley Jones, Chirag Ramani, Christopher Rydell, Daniel Wagner-Hall, Ed Schouten, Fabian Brandstetter, Fabian Meumertzheim, Greg, Ivan Golub, Jon Landis, JY Lin, Kai Zhang, Keith Smiley, kotlaja, lripoche, oquenchil, Pavan Singh, Rasrack, Son Luong Ngoc, Takeo Sawada, Vertexwahn, Xùdōng Yáng, Yannic.
@fmeum I think this may not actually be fixed. See It uses Bazel 6.3.2 but fails to run WDYT? |
@alexeagle Good point, this commit didn't change anything for Starlark options. That requires a separate effort. I will take a look. |
Ensures that `bazel version` does not fail when a `common` line in `.bazelrc` contains a Starlark option (similar for `sync` and `shutdown`). There is very little chance for users being confused about these commands accepting and silently ignoring Starlark flags. Fixes #18130 (comment) Closes #19363. PiperOrigin-RevId: 562959337 Change-Id: Ifb4f44d63f1801f6c5a8c2f421138b4014c49a06
Ensures that `bazel version` does not fail when a `common` line in `.bazelrc` contains a Starlark option (similar for `sync` and `shutdown`). There is very little chance for users being confused about these commands accepting and silently ignoring Starlark flags. Fixes bazelbuild#18130 (comment) Closes bazelbuild#19363. PiperOrigin-RevId: 562959337 Change-Id: Ifb4f44d63f1801f6c5a8c2f421138b4014c49a06
…e` (#19417) Ensures that `bazel version` does not fail when a `common` line in `.bazelrc` contains a Starlark option (similar for `sync` and `shutdown`). There is very little chance for users being confused about these commands accepting and silently ignoring Starlark flags. Fixes #18130 (comment) Closes #19363. Commit 954b4d9 PiperOrigin-RevId: 562959337 Change-Id: Ifb4f44d63f1801f6c5a8c2f421138b4014c49a06 Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
A fix for this has been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks! |
Looks like there is still an issue with this feature. Here is some output I got on Bazel 6.4.0:
Perhaps an issue when using the |
@gregmagolan I can't reproduce this on one of my repos:
Do you have a reproducer you can share? |
Ahh. It seems specific to a
|
Further investigation narrows it down to the
flag in if I update that bazelrc file to
Then it works as expected:
|
Narrowed it down even further. It is not specific to the
to the root
|
@gregmagolan I think I found a fix in #20720. This doesn't affect 7.0.0 since the particular expansion has been removed and the other common ones I looked at don't cross option class boundaries. |
When a flag specified with `common` in a `.bazelrc` file expanded to a flag unsupported by the current command, it resulted in an error rather than the flag being ignored. Fixes #18130 (comment) Closes #20720. PiperOrigin-RevId: 597271727 Change-Id: Ieaba4dc00e13495a859e1eedf802759ad7dbf774
When a flag specified with `common` in a `.bazelrc` file expanded to a flag unsupported by the current command, it resulted in an error rather than the flag being ignored. Fixes bazelbuild#18130 (comment) Closes bazelbuild#20720. PiperOrigin-RevId: 597271727 Change-Id: Ieaba4dc00e13495a859e1eedf802759ad7dbf774
When a flag specified with `common` in a `.bazelrc` file expanded to a flag unsupported by the current command, it resulted in an error rather than the flag being ignored. Fixes #18130 (comment) Closes #20720. Commit b303144 PiperOrigin-RevId: 597271727 Change-Id: Ieaba4dc00e13495a859e1eedf802759ad7dbf774 Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Fixes #3054
RELNOTES: Options specified on the pseudo-command
common
in.rc
files are now ignored by commands that do not support them as long as they are valid options for any Bazel command. Previously, commands that did not support all options given forcommon
would fail to run. These previous semantics ofcommon
are now available via the newalways
pseudo-command.