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

Verify that incompatible options are preserved in the exec cfg #16449

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 11, 2022

Adding incompatible options is error-prone as it is easy to forget to add them to their corresponding FragmentOption's getHost method. If this is not done, the flag will have no effect in the exec configuration, which has already caused very surprising, buggy behavior in numerous cases.

This commit adds a test to verify that all non-deprecated incompatible flags:

  1. are tagged with the INCOMPATIBLE_CHANGE metadata tag;
  2. are preserved in the exec configuration.

Fixes #12238
Fixes #16388

Adding incompatible options is error-prone as adding them to their
corresponding `FragmentOption`'s `getHost` method is easily forgotten.
In that case, the flag will have no effect in the exec configuration,
which has already caused very surprising, buggy behavior in numerous
cases.

This commit adds a test to verify that all non-deprecated incompatible
flags:

1. are tagged with the `INCOMPATIBLE_CHANGE` metadata tag;
2. are preserved in the exec configuration.

Fixes bazelbuild#12238
Fixes bazelbuild#16388
@fmeum fmeum force-pushed the 16388-preserve-incompatible-options-in-exec branch from 945dcce to ba8b7f2 Compare October 11, 2022 07:03
@fmeum fmeum marked this pull request as ready for review October 11, 2022 07:22
@fmeum fmeum requested a review from lberki as a code owner October 11, 2022 07:22
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 11, 2022

@gregestren Could you review? I grew tired of fixing these one-by-one ;-)

@sgowroji sgowroji added team-Rules-Python Native rules for Python awaiting-review PR is awaiting review from an assigned reviewer labels Oct 11, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 11, 2022

@sgowroji This ends up fixing Python issues along the way, but the root cause doesn't lie in rule logic. I expect the configurability team (e.g. @gregestren) to be in a better position to review this.

@sgowroji sgowroji added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Oct 11, 2022
@gregestren gregestren self-assigned this Oct 12, 2022
Copy link
Contributor

@gregestren gregestren left a comment

Choose a reason for hiding this comment

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

Ugh. Good call on the test.

@gregestren gregestren added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Oct 12, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Oct 14, 2022
@fmeum fmeum deleted the 16388-preserve-incompatible-options-in-exec branch October 14, 2022 16:04
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-Python Native rules for Python
Projects
None yet
3 participants