Support constraint_value directly in select() #12071

gregestren · 2020-09-09T20:53:02Z

No more need for a redundant config_setting. See #8583 for an example.

This was a bit subtle. constraint_value can't directly export a ConfigMatchingProvider because it needs to know the
platform to determine if it's a match. But platforms are built out of constraint_values, so the platform isn't available
yet. So the parent target with the select() provides this detail.

Also beautifies "invalid select() key" errors in support of #11984.

Fixes #8583.

RELNOTES[NEW]: select() directly supports constraint_value (no need for an intermediate config_setting).

No more need for a redundant config_setting. See bazelbuild#8583 for an example. This was a bit subtle. constraint_value can't directly export a ConfigMatchingProvider because it needs to know the platform to determine if it's a match. But platforms are built out of constraint_values, so the platform isn't available yet. So the parent target with the select() provides this detail. Also beautifies "invalid select() key" errors in support of bazelbuild#11984. Fixes bazelbuild#8583. RELNOTES[NEW]: select() directly supports constraint_value (no need for an intermediate config_setting). PiperOrigin-RevId: 327885099 Change-Id: Ifbdfa275ff83f0cb30207a5bd77aca317599fc21

gregestren · 2020-09-09T20:53:16Z

@philsc @AustinSchuh FYI

katre

This is really clever and I like it.

Most of my comments are not actionable, just ways I want to think about the API. I will probably come back tomorrow and turn some of them into standalone issues, sorry for adding my stream-of-consciousness to your PR review.

katre · 2020-09-09T23:03:47Z

site/docs/configurable-attributes.md

+```
+
+This saves the need for boilerplate `config_setting`s when you only need to
+check against single values.


Should we give an example of using config_setting for multiple constraint values?

The example before here already does this, so I think we have good coverage overall: https://docs.bazel.build/versions/3.5.0/configurable-attributes.html#platforms.

katre · 2020-09-09T23:05:12Z

src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintValueInfo.java

+   * Returns a {@link ConfigMatchingProvider} that matches if the owning target's platform includes
+   * this constraint.
+   *
+   * <p>The {@link com.google.devtools.build.lib.rules.platform.ConstraintValue;ConstraintValue}


The @link syntax looks wrong here, what are you intending to do?

Right. Just removing the part after the semi-colon works. I fully qualified it because otherwise an import on its package adds a dependency which messes with the BUILD dependency structure. Which isn't worth it just for a code link (I believe that's our actual style guidance too: to not add new dependencies just for doc links).

katre · 2020-09-09T23:05:32Z

src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintValueInfo.java

+   *
+   * <p>Instead, a target with a <code>select()</code> on a {@link
+   * com.google.devtools.build.lib.rules.platform.ConstraintValue} passes its platform info to this
+   * method.


That is really clever. :)

katre · 2020-09-09T23:08:00Z

src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

              env,
              ctgValue,
              transitivePackagesForPackageRootResolution,
+              unloadedToolchainContexts == null
+                  ? null
+                  : unloadedToolchainContexts.getDefaultToolchainContext().targetPlatform(),


All toolchain contexts for the same configured target, regardless of exec group, should have the same target platform (right @juliexxia ?)

We should consider enshrining that in the API by adding a unloadedToolchainContexts.targetPlatform() method.

Yeah that sounds right and looks right according to https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java;l=531?q=ConfiguredTargetFunction&ss=bazel

@katre I like that idea. Do you think there's anything else to discuss there? If not I can mail something.

I don't think so, it makes sense. Go ahead if you have time, I was going to file an issue and then deal with it later :)

To be clear, this suggestion is for a followup PR?

Yes, nothing to do in this PR.

katre · 2020-09-09T23:11:20Z

src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

+        // If platformInfo == null, that means the owning target doesn't invoke toolchain
+        // resolution, in which case depending on a constraint_value is non-sensical.
+        configConditions.put(
+            entry.getLabel(), constraintValueInfo.configMatchingProvider(platformInfo));


Could we add a ConfigMatchingProvider.configMatchingProvider(???) method, so that we'd only have one branch here? We'd have to do some weird generic tricks to get the type right, and it might not be worth it, but if we ever add a third, we should consider a way to collapse the choices.

Good point. Given the frequency of rule expansion (very infrequent), I vote for not supporting that much extra abstraction yet. But I added a note with your suggestion if this ever goes further.

gregestren · 2020-09-10T22:26:34Z

This is really clever and I like it.

I assure you my first pass on this looked 10,000 times worse! I was surprised and happy to reduce this to something so non-invasive.

Most of my comments are not actionable, just ways I want to think about the API. I will probably come back tomorrow and turn some of them into standalone issues, sorry for adding my stream-of-consciousness to your PR review.

No worries, thanks for the comments. I'll follow up properly on them as soon as I can.

pcjanzen · 2021-01-24T18:46:25Z

Is this supposed to work in a filegroup?

filegroup(
    name = "cv_in_filegroup",
    srcs = select({
        "@platforms//os:windows": ["dummy"],
        "//conditions:default": ["dummy"],
    }),
)

$ bazel-4.0.0-darwin-x86_64 build //:cv_in_filegroup
ERROR: BUILD:84:10: @platforms//os:windows is not a valid select() condition for //:cv_in_filegroup.
To inspect the select(), run: bazel query --output=build //:cv_in_filegroup.
For more help, see https://docs.bazel.build/be/functions.html#select.

but the corresponding version with a config_setting works correctly, as does the select clause in (for example) a sh_binary.

cc @UebelAndre

gregestren · 2021-01-26T00:09:25Z

Interesting catch!

My first instinct was to say it shouldn't matter what rule uses the select - that's not where the logic kicks in. But lo and behold I replicated your exact error, then changed the filegroup to a cc_library and suddenly it worked. This calls for more digging...

UebelAndre · 2021-01-26T00:12:45Z

One other thing I noticed is the ctx.attr.bool does not accept select statements. Not sure if that's directly related to this 😅

gregestren · 2021-01-26T00:27:07Z

Found the problem!

bazel/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java

Line 814 in 63e0392

} else if (constraintValueInfo != null && platformInfo != null) {

In this example platformInfo is null.

Matching the constraint_value with the platform requires knowing what the platform is. select() logic gets all its info from the target it's attached to. Most rules trigger platform resolution, but some don't. filegroup is one of the exceptions.

I think the idea is that filegroup is such a simple rule it doesn't matter what the platform is, since there's no filegroup toolchain to match it to. So Bazel avoids the extra work of platform resolution. As a side effect you get this bug.

i believe filegroup is one of the rare rules with this problem. CC @katre if there's any obvious fix to this blip.

ctx.attr.bool sounds like a separate issue. Can you file a distinct bug for it?

katre · 2021-01-26T13:25:18Z

Filegroup was explicitly added to the set of rules that don't participate in toolchain resolution (and that thus don't have a target platform) back during the early development. 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 take another look at this and see if we can turn it back on.

gregestren · 2021-01-26T14:38:41Z

If that doesn't make sense conceptually, I think we could conditionally load the platform when we notice a constraint_value within the select() logic.

gregestren requested a review from katre September 9, 2020 20:53

gregestren requested review from aiuto, floriographygoth and jin as code owners September 9, 2020 20:53

googlebot added the cla: yes label Sep 9, 2020

katre approved these changes Sep 9, 2020

View reviewed changes

Review feedback updates

aff487a

katre approved these changes Sep 11, 2020

View reviewed changes

bazel-io closed this in 12b06b2 Sep 11, 2020

gregestren deleted the select_on_constraints branch September 11, 2020 20:53

UebelAndre mentioned this pull request Jan 26, 2021

Allow the attr parameter default to accept select statements #12895

Closed

katre mentioned this pull request Jan 26, 2021

Investigate whether filegroup can use platform-conditional selects #12899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support constraint_value directly in select() #12071

Support constraint_value directly in select() #12071

gregestren commented Sep 9, 2020 •

edited

Loading

gregestren commented Sep 9, 2020

katre left a comment

katre Sep 9, 2020

gregestren Sep 11, 2020

katre Sep 9, 2020

gregestren Sep 11, 2020

katre Sep 9, 2020

katre Sep 9, 2020

juliexxia Sep 10, 2020

katre Sep 10, 2020

gregestren Sep 11, 2020

katre Sep 11, 2020

katre Sep 9, 2020

gregestren Sep 11, 2020

gregestren commented Sep 10, 2020

pcjanzen commented Jan 24, 2021

gregestren commented Jan 26, 2021

UebelAndre commented Jan 26, 2021

gregestren commented Jan 26, 2021

katre commented Jan 26, 2021

gregestren commented Jan 26, 2021

Support constraint_value directly in select() #12071

Support constraint_value directly in select() #12071

Conversation

gregestren commented Sep 9, 2020 • edited Loading

gregestren commented Sep 9, 2020

katre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregestren commented Sep 10, 2020

pcjanzen commented Jan 24, 2021

gregestren commented Jan 26, 2021

UebelAndre commented Jan 26, 2021

gregestren commented Jan 26, 2021

katre commented Jan 26, 2021

gregestren commented Jan 26, 2021

gregestren commented Sep 9, 2020 •

edited

Loading