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

Bazel's filegroup should participate in toolchain resolution. #13223

Closed
wants to merge 1 commit into from

Conversation

katre
Copy link
Member

@katre katre commented Mar 15, 2021

This reverts dce861d, which was a
mistake.

It also removes a test which is now useless. There are no more build
rules (which have the target_compatible_with attribute for target
skipping) which also have toolchain resolution disabled, so this cannot
be tested for.

Fixes #12899 and undoes #13194.

This reverts dce861d, which was a
mistake.

It also removes a test which is now useless. There are no more build
rules (which have the target_compatible_with attribute for target
skipping) which also have toolchain resolution disabled, so this cannot
be tested for.

Fixes bazelbuild#12899 and undoes bazelbuild#13194.
@katre katre requested a review from gregestren March 15, 2021 16:10
@google-cla google-cla bot added the cla: yes label Mar 15, 2021
@katre
Copy link
Member Author

katre commented Mar 15, 2021

@philsc thanks for pointing out that this change was a mistake. I'm removing the test that caused my confusion.

@gregestren
Copy link
Contributor

What was the full context here (losing all the threads in my inbox)? Being able to use select in filegroup?

@katre
Copy link
Member Author

katre commented Mar 15, 2021

Originally, #12899 was filed because selects in filegroup worked, except for selects using constraint values. I fixed this by enabling toolchain resolution for filegroup.

Then, last week, I noticed that

  • We have tests that assume filegroup does not take part in toolchain resolution, and
  • The internal version of filegroup and the external version deviate on this.

I picked the fix the make them both not take part in toolchain resolution, forgetting about #12899. That, unfortunately, regressed that issue, so now I am reverting that change (and removing a useless test).

The test is useless because it was testing whether a build rule (therefore, one that can use target_compatible_with for target skipping) that had toolchain resolution disabled work work properly. However, with this change to filegroup, there are no remaining build rules that have toolchain resolution disabled, so this isn't a valid state.

@gregestren
Copy link
Contributor

Makes sense. Thanks so much for the clarification.

@bazel-io bazel-io closed this in 28c0c8c Mar 16, 2021
@katre katre deleted the b12899-re-fix-filegroup branch March 16, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate whether filegroup can use platform-conditional selects
2 participants