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

Propagate all experimental options to the exec configuration #23326

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 18, 2024

Experimental flags that don't propagate to the exec configuration can create very surprising failure cases for both Bazel users and devs. Unless there is a valid reason to not propagate them, they should be.

This is now enforced by a test and existing flags have been cleaned up where possible. The test strategy is the same as for incompatible_ flags.

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 18, 2024

This is motivated by #23328, in which a number of tests based on genrule I added failed open due to an experimental flag not propagating to the exec configuration.

@fmeum fmeum marked this pull request as ready for review August 18, 2024 19:08
@fmeum fmeum requested review from a team, lberki, comius and rickeylev as code owners August 18, 2024 19:08
@fmeum fmeum requested review from aranguyen and removed request for a team, comius and rickeylev August 18, 2024 19:08
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-Java Issues for Java rules team-Rules-CPP Issues for C++ rules team-Rules-Python Native rules for Python team-Rules-ObjC Issues for Objective-C maintainers awaiting-review PR is awaiting review from an assigned reviewer labels Aug 18, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 18, 2024

@lberki Could you review this as Bazel's "Chief Flag Officer"? :⁠-⁠)
@aranguyen for configurability

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 13, 2024

@lberki Friendly ping

@lberki lberki requested review from gregestren and removed request for lberki September 16, 2024 08:12
@lberki
Copy link
Contributor

lberki commented Sep 16, 2024

Uh, sorry. I'll defer to @gregestren here. This seems like a good idea on the surface. You also probably want to give --incompatible_* the same treatment.

@fmeum
Copy link
Collaborator Author

fmeum commented Sep 16, 2024

I added a test for incompatible tests about a year ago, but back then I wasn't sure whether experimental flags should be treated in the same way.

@gregestren gregestren removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 17, 2024
@gregestren gregestren added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 17, 2024
@fmeum fmeum requested a review from gregestren October 1, 2024 08:52
@gregestren
Copy link
Contributor

Do we need re-review?

@fmeum
Copy link
Collaborator Author

fmeum commented Oct 2, 2024

Do we need re-review?

Sorry, clicked the button accidentally while going over my pending PRs.

@@ -130,6 +130,28 @@ bazel_fragments["AndroidConfiguration.Options"] = fragment(
"//command_line_option:internal_persistent_busybox_tools",
"//command_line_option:internal_persistent_multiplex_busybox_tools",
"//command_line_option:incompatible_disable_native_android_rules",
"//command_line_option:android_databinding_use_androidx",
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @fmeum . We're trying to get this merged for Bazel 8.

Why were these flags added? Does this have something to do with the experimental_android_databinding_v2below?

@copybara-service copybara-service bot closed this in 4ca3db2 Oct 4, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules team-Rules-Java Issues for Java rules team-Rules-ObjC Issues for Objective-C maintainers team-Rules-Python Native rules for Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants