prevent crash when migrated native flag gets deleted#27793
prevent crash when migrated native flag gets deleted#27793aranguyen wants to merge 0 commit intobazelbuild:masterfrom
Conversation
gregestren
left a comment
gregestren
left a comment
There was a problem hiding this comment.
Noted your chat comment about startup_options_test failing. Do you have a link to the specific failure?
There was a problem hiding this comment.
Are the changes in SkyframeExecutor unrelated to this PR?
| parsedOptionName = name; | ||
| } else { | ||
| throw new OptionsParsingException("Invalid options syntax: " + arg, arg); | ||
| throw new OptionsParsingException("Invalid options syntax: " + arg, arg); |
| if (!isFirstParsing) { | ||
| throw new OptionsParsingException("Unrecognized option: " + arg + suggestion, arg); | ||
| } else { | ||
| swapShorthandAlias(arg); |
There was a problem hiding this comment.
My first thought is this wouldn't have any effect?
I'm I'm reading this right, we reach this line on --foo before MODULE.bazel has mapped --foo=--//actual. Wouldn't that mean there isn't yet any alias mapping for --foo so this function couldn't do anything?
| PriorityCategory.COMPUTED_DEFAULT, | ||
| "Options required to fetch target", | ||
| ImmutableList.of("--nobuild")); | ||
| ImmutableList.of("--nobuild"), false); |
There was a problem hiding this comment.
If I'm reading correctly there's only one case where you want to skip failing on an unknown option, and all other parsing calls should behave as before.
since OptionsParser has a builder interface:
Default it to false unless explicitly set differently in a builder method.
I'd also use more direct behavioral wording than isFirstParsing, although I appreciate "first parsing" is the context that triggers the behavior. But I'd name the variable after the actual behavior, like failOnUnknownOptions. Then add a comment in the caller code that sets this to true explaining why the first parsing sets this field this way.
Note that the post-repo mapping parsing step at
true field back to false for the second parsing.
There was a problem hiding this comment.
Ahhh that's a really good idea to make it part of the Builder. I myself thought passing it around was not a good idea. And agree on the naming as well. Thanks!!!!
They can't be deleted until bazelbuild#27793 merges.
This removes hard-coded flags `--python_path`, `--experimental_python_import_all_repositories`, and `--incompatible_remove_ctx_bazel_py_fragment`. The first two are redefined in `rules_python` 1.7.0+ and linked into Bazel in #27792. This PR graveyards them but we can delete them outright when #27793 merges. `--incompatible_remove_ctx_bazel_py_fragment` was added in Bazel 9 to allow toggling between the native and Starlark definitions. By removing `--incompatible_remove_ctx_bazel_py_fragment`, this change removes the ability to revert to native definitions for post-9 Bazel. For #26521 and bazel-contrib/rules_python#3252. Closes #27842. PiperOrigin-RevId: 842305784 Change-Id: If55aa925d0cd546e0671ddcb44f9e2819697b22f
Related to #27842. This removes hard-coded flags `--build_python_zip`, `--incompatible_default_to_explicit_init_py`, `--python_native_rules_allowlist`, `incompatible_python_disallow_native_rules`, and `incompatible_remove_ctx_py_fragment`. These flags are re-difined in rules_python 1.7.0+ and linked into Bazel in #27792. This PR graveyards them but we can delete them outright when #27793 merges. By removing --incompatible_remove_ctx_py_fragment, this change removes the ability to revert to native definitions for post-9 Bazel. For #26521 and bazel-contrib/rules_python#3252. Closes #27900. PiperOrigin-RevId: 843734148 Change-Id: I1bb9624b3e5a579f6aa50663eb63ef4d7d482b57
2 participants
For native flags that are migrated with
flag_aliasin MODULE.bazel, bazel previously crashed withUnrecognized Optionerror if the native flags get deleted.This PR prevents blaze from crashing in such scenario.