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.

Sign up for GitHub

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

Jump to bottom

Investigate whether filegroup can use platform-conditional selects #12899

Closed
katre opened this issue Jan 26, 2021 · 4 comments
Closed

Investigate whether filegroup can use platform-conditional selects #12899

katre opened this issue Jan 26, 2021 · 4 comments
Assignees
@katre
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@katre
Copy link
Member

katre commented Jan 26, 2021

Branched from #12071:

Currently, the filegroup rule doesn't participate in toolchain resolution (see https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/bazel/rules/common/BazelFilegroupRule.java;l=78), which means the target platform doesn't exist and so platform-conditional selects don't work.

As I recall, it led to a cycle in evaluation, because filegroups were needed at a low-enough level that we couldn't process loading platforms without them.

I'll investigate whether this is still an issue and whether this can be re-enabled.
@katre katre added type: feature request P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Jan 26, 2021
@katre katre self-assigned this Jan 26, 2021
katre added a commit to katre/bazel that referenced this issue Jan 26, 2021
@katre
Enable toolchain resolution for filegroup targets.
47de281 
Fixes bazelbuild#12899.
@katre katre mentioned this issue Jan 26, 2021
Closed
@katre
Copy link
Member Author

katre commented Jan 26, 2021

To clarify, we have a test for this specific behavior: https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java;l=1232

katre added a commit to katre/bazel that referenced this issue Jan 26, 2021
@katre
Update platforms_test to not rely on filegroup not using toolchain
7f37c18 
resolution.

Part of work on bazelbuild#12899.
@katre katre mentioned this issue Jan 26, 2021
Closed
katre added a commit to katre/bazel that referenced this issue Jan 26, 2021
@katre
Update SkyframeTests to not rely on filegroup not using toolchain
5cf7314 
resolution.

Part of work on bazelbuild#12899.
@katre katre mentioned this issue Jan 26, 2021
Closed
katre added a commit to katre/bazel that referenced this issue Jan 26, 2021
@katre
Update ConfigurableAttributesTest to not rely on filegroup not using …
1c27ee7 
…toolchain

resolution.

Part of work on bazelbuild#12899.
@katre katre mentioned this issue Jan 26, 2021
Closed
@gregestren
Copy link
Contributor

Before advancing, what do you think of my question in #12071 (comment)? Could it be select that instantiates the platform info when it's not already available? Would that also require a toolchain or could it just directly load the platform?

How would that compare to opting filegroup back into toolchains?

That's really interesting about the test. Was that intentional?

@katre
Copy link
Member Author

katre commented Jan 26, 2021

I made the change and ran tests (see #12901). All the failures, including ConditionalAttributesTest, were just using filegroup as a placeholder rule that doesn't use toolchain resolution.

To reply to your comment: I think it does make sense that even without toolchain resolution, configured targets always have a target platform. This shouldn't be a major change to how things happen now, and I can file a (low-priority) issue to investigate further.

As far as changing filegroup it looks very reasonable to me and I'm going to continue with the PRs to make that change. Once the tests are cleaned up I'll need to be sure to run internal tests for the same behavior and make sure there are no further blockers.

bazel-io pushed a commit that referenced this issue Jan 26, 2021
@katre @copybara-github
Update platforms_test to not rely on filegroup not using toolchain
f31e867 
resolution.

Part of work on #12899.

Closes #12906.

PiperOrigin-RevId: 353936927
bazel-io pushed a commit that referenced this issue Jan 26, 2021
@katre @copybara-github
Update SkyframeTests to not rely on filegroup not using toolchain
13031e5 
resolution.

Part of work on #12899.

Closes #12907.

PiperOrigin-RevId: 353937486
bazel-io pushed a commit that referenced this issue Jan 26, 2021
@katre @copybara-github
Update ConfigurableAttributesTest to not rely on filegroup not using …
1165182 
…toolchain

resolution.

Part of work on #12899.

Closes #12908.

PiperOrigin-RevId: 353938904
katre added a commit to katre/bazel that referenced this issue Jan 26, 2021
@katre
Enable toolchain resolution for filegroup targets.
772c2a9 
Fixes bazelbuild#12899.
@gregestren gregestren mentioned this issue Feb 19, 2021
Closed
@philsc philsc mentioned this issue Mar 13, 2021
Closed
philwo pushed a commit that referenced this issue Mar 15, 2021
@katre @philwo
Enable toolchain resolution for filegroup targets.
f966749 
Fixes #12899.

Closes #12901.

PiperOrigin-RevId: 354087352
philwo pushed a commit that referenced this issue Mar 15, 2021
@katre @philwo
Enable toolchain resolution for filegroup targets.
99aa968 
Fixes #12899.

Closes #12901.

PiperOrigin-RevId: 354087352
@katre
Copy link
Member Author

katre commented Mar 15, 2021

Re-opening because I undid this in #13194.

I need to add a test and figure out the right solution for the issue of split internal/external filegroup implementations.

@katre katre reopened this Mar 15, 2021
katre added a commit to katre/bazel that referenced this issue Mar 15, 2021
@katre
Bazel's filegroup should participate in toolchain resolution.
30be40a 
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 mentioned this issue Mar 15, 2021
Closed
philwo pushed a commit that referenced this issue Apr 20, 2021
@katre @philwo
Update platforms_test to not rely on filegroup not using toolchain
a8d20b9 
resolution.

Part of work on #12899.

Closes #12906.

PiperOrigin-RevId: 353936927
philwo pushed a commit that referenced this issue Apr 20, 2021
@katre @philwo
Update SkyframeTests to not rely on filegroup not using toolchain
a7051b3 
resolution.

Part of work on #12899.

Closes #12907.

PiperOrigin-RevId: 353937486
philwo pushed a commit that referenced this issue Apr 20, 2021
@katre @philwo
Update ConfigurableAttributesTest to not rely on filegroup not using …
f1e04ae 
…toolchain

resolution.

Part of work on #12899.

Closes #12908.

PiperOrigin-RevId: 353938904
gregestren added a commit to gregestren/bazel that referenced this issue Nov 23, 2021
@gregestren
Support select() on constraint_value for aliases.
2f5048d 
This implements approach bazelbuild#4 of
bazelbuild#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target
has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platfrom
info to *all* rules
(bazelbuild#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes bazelbuild#13047.

PiperOrigin-RevId: 411684872
Change-Id: I998ef9bba3226871651fc14bd9ed268e9a3de82c
@gregestren gregestren mentioned this issue Nov 23, 2021
Closed
bazel-io pushed a commit that referenced this issue Nov 23, 2021
@gregestren @copybara-github
Support select() on constraint_value for aliases.
1c3a245 
This implements approach #4 of
#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platform info to *all* rules (#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes #13047.

Closes #14310.

PiperOrigin-RevId: 411868223
Bencodes pushed a commit to Bencodes/bazel that referenced this issue Jan 10, 2022
@gregestren @Bencodes
Support select() on constraint_value for aliases.
e0aaa40 
This implements approach bazelbuild#4 of
bazelbuild#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platform info to *all* rules (bazelbuild#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes bazelbuild#13047.

Closes bazelbuild#14310.

PiperOrigin-RevId: 411868223
brentleyjones pushed a commit to brentleyjones/bazel that referenced this issue Feb 8, 2022
@gregestren @brentleyjones
Support select() on constraint_value for aliases.
1936c05 
This implements approach bazelbuild#4 of
bazelbuild#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platform info to *all* rules (bazelbuild#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes bazelbuild#13047.

Closes bazelbuild#14310.

PiperOrigin-RevId: 411868223
(cherry picked from commit 1c3a245)
@brentleyjones brentleyjones mentioned this issue Feb 8, 2022
Merged
Wyverald pushed a commit that referenced this issue Feb 9, 2022
@brentleyjones @gregestren
Support select() on constraint_value for aliases. (#14754)
0df1851 
This implements approach #4 of
#13047 (comment).

The basic change adds a new toolchain resolution mode: "resolve iff the target has a select()". It then sets alias() to that mode.

We could remove this special casing if we ever ubiquitously provide platform info to *all* rules (#12899 (comment)).

RELNOTES: alias() can now select() directly on constraint_value()

Fixes #13047.

Closes #14310.

PiperOrigin-RevId: 411868223
(cherry picked from commit 1c3a245)

Co-authored-by: Greg Estren <gregestren@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@katre katre

Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Milestone
No milestone
2 participants
@katre @gregestren