select()'s unambiguous specialization resolution fails with platform constraint_value()s

[cpsauer]

Hi, wonderful Bazel folks. Thanks for all the great work you do, including extending official support for platforms.

This is a bug report at the intersection of select() and constraint_value() (for platforms).
You can definitely work around it, but it's a bit annoying. I'm guessing it's just an early rough edge, since platforms are still in flux.

This is easiest by example. Say you have:

config_setting(
    name="ios-aarch64",
    constraint_values = [
        "@platforms//os:ios",
        "@platforms//cpu:aarch64",
    ]
)

And you try to select() between :ios-aarch64 and @platforms//os:ios.

You'll get an error like:

Illegal ambiguous match on configurable attribute <...>
:ios-aarch64
@platforms//os:ios
Multiple matches are not allowed unless one is unambiguously more specialized.

Despite the former being unambiguously more specialized.

This unambiguous specialization behavior works correctly for normal, non-platform config_setting()s that use the values parameter instead of constraint_values, and is documented here.

I'm seeing this on the latest macOS (12.1) with the latest Bazel rolling (6.0.0-pre.20220112.2).

Thanks so much for taking a peek!
Chris
(ex-Googler)

[pcjanzen]

It's a bit surprising at first, but the docs at https://docs.bazel.build/versions/main/be/functions.html#select do say:

However, the number of constraint values that A and B have are not considered in this comparison -- one condition cannot match a platform more than another condition does.

[cpsauer]

Wow, good eye!
If that's the intended behavior, probably worth putting it also in the other, more expansive doc, especially because it has the potential to surprise with its inconsistency with the rest of the behavior of select().

Is that really the behavior we'd want, though? It's not clear to me the logic holds. Constraint values are subsets of the properties a platform has. You can definitely match a larger or smaller subset of properties. Further, it's useful! Presumably that's why select has that behavior more generally.

[gregestren]

The good news is that comment is pretty old (b9a5f56) and a lot's changed since then. We'll get this an updated review next week and get some better feedback.

[aranguyen]

an update on this. commit 0e7051a was made to allow config_setting to match on constraint_values, but it seems that the implementation for specialization to work with constraint_values were left out. I also doubled checked with katre@ and brandjon@. Since users are expeciting this behavior and it's better to keep it consistent with the behavior of select(), we should fix this.

Briefly looking at our code, i think we would need to pass the constraint values in the ConfigMatchingProvider here https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/config/ConfigSetting.java#L125

and the information could be used to "refines" a condition here https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigMatchingProvider.java#L77

I probably will have time to work on this in the next iteration.

[cpsauer]

Woo! Thanks guys. Team-configurability for the win :)

[meteorcloudy]

Should we close this one since 240af80 is now submitted?

[aranguyen]

Yes we can close this one. Thanks!

[cpsauer]

Thank you all so much for being great and landing code to fix.

I am, however, still seeing this with the latest Bazel rolling (6.0.0-pre.20220421.3), when I tried to remove our workarounds. And I think 240af80 should be in that version based on the tags linked on the commit. Am I missing something here, or is it possible that the issue is still outstanding somehow?

[aranguyen]

@cpsauer could you share your full example so I could try and reproduce it on my end? 240af80 should address this issue.

[cpsauer]

Sure! It should repro in the the minimal case, above.
Gimme a couple minutes and I'll create an archive of it for ya

[cpsauer]

Here you go!
platform_select_repro.zip

Just run bazel build example --platforms=//:ios-aarch64

And you'll get the same error:

ERROR:<...>/platform_select_repro/BUILD:17:11: Illegal ambiguous match on configurable attribute "linkopts" in //:example:
//:ios-aarch64-for-select
@platforms//os:ios
Multiple matches are not allowed unless one is unambiguously more specialized.

---

It's just a blank WORKSPACE, plus a BUILD file with the above, (arbitrarily) selecting on linkopt in a cc_library, plus a matching platform definition.

The .bazelversion therein points bazelisk to rolling--for easy future testing. At the time of writing, that's 6.0.0-pre.20220421.3, same as above, which it sounds like we both think should contain the commit.

---

Thanks so much!
Chris

[katre]

I'll leave it to @aranguyen to look into the actual issue (but I do think that this repro should be unambiguous).

As regards using platform in a select(), I think that this has confusing semantics. If I write the following:

platform(name = "plat", constraint_values = ["//foo", "//bar"])
platform(name = "other_plat", constraint_values = ["//baaz"])
platform(name = "duplicate_plat", constraint_values = ["//foo", "//bar"])
platform(name = "child_plat", parents = [":plat"])

cc_library(
    name = "lib",
    ...,
    linkopts = select({
        ":plat": ["--flag"],
        "//conditions:default": [],
    }),
)

Case 1: bazel build --platforms=:plat :lib

In this case, I'd expect the select to match and the linkopts to be --flag.

Case 2: bazel build --platforms=:other_plat :lib

In this case, I'd expect the select not to match, and the linkopts to be empty.

Cases 3 and 4: bazel build --platforms=:duplicate_plat :lib or bazel build --platforms=:child_plat :lib

These cases are ambiguous: does the select mean "the target platform has the same constraint values as :plat", or does it mean "--platforms equals :plat"?

Will every user who reads this BUILD file expect the same thing?

What are the chances that this ambiguity will confuse things and cause people's builds to be wrong?

So, here's my suggestion: if you can write up a proposal that clarifies this and provides non-ambiguous semantics, that we can all agree on, then we can look into implementing it. But at this stage I think this has too much potential confusion to just add without deep consideration.

[cpsauer]

Sweet. Thanks for looking, @katre.
Should we be reopening in the meantime, since this still looks to be active?
If so, could I ask someone to?

---

Also, John I appreciate the thoughtfulness of your read-through! I wasn't sure if anyone would ever see that note, but since I came across it again, I figured I'd jot it down. I don't mean to cause waves; just meant it as maybe-interesting user feedback in context. I'm sure you've thought about this far more than I have.

If useful, it does seems to this user like the ambiguity above might resolve itself nicely with the same semantics config_setting currently has. (And I think for the same reasons.) That is, equivalent platforms--by any name--matching each other.

For why: Intuitively, I'd have assumed this because platforms are billed as just "a named collection of constraint choices", and collections match if their members match. And more practically, I think that the biggest cause of different-name-same-platform is that, without common definitions, every library/workspace defines its own, and those need to match, so that the libraries can be used together. (Related to your good bazelbuild/platforms#36)

[aranguyen]

@katre I was able to use the example @cpsauer provided and it worked without error as is. But when I changed to select() with platform that's when I saw error as expected.

@cpsauer is the following the semantics that you were hoping for instead of creating a duplicate config as shown in the repro? If so, please feel free to reopen and follow @katre's suggestion to write a proposal

platform(
    name = "ios-aarch64",
    constraint_values = [
        "@platforms//os:ios",
        "@platforms//cpu:aarch64",
    ],
)

cc_library(
    name = "not_working_example",
    linkopts = select({
        ":ios-aarch64": ["-ltest1"],
        "@platforms//os:ios": ["-ltest2"],
    }),
)

[cpsauer]

Oh, uh oh. I definitely don't want to be wasting your guys' time. Let me go investigate.

[cpsauer]

Oh, interesting, so a (different?) fix seems to have landed since I created the reproducing case.

It's now fixed in the new latest rolling 6.0.0-pre.20220517.1, but per above, not in the previous 6.0.0-pre.20220421.3, the latest at the time of writing, which contained the commit referenced as fixing, I think (as above). 🤷🏻

[Just to make sure we're all on the same page, this is all about the unambiguous specialization error that's the focus of this issue, not about the platforms-in-select side note.]

Anyway, thanks wonderful Team Configurability for being so responsive to user feedback and polishing this into something consistently great!

[cpsauer]

Seriously, thank you. Just backed out a bunch of code working around this :) Awesome

[cpsauer]

[And finally on the (separate) platform-in-select stuff, @aranguyen, yes, exactly.

I'd just noted it down as a potential improvement when I ran into it because I thought there was a good chance you all would be as positive on it as this one--and that it could be a slam-dunk combo move.

Anyway, rather than reply more here, I'll tap back briefly on that issue that other user subsequently created for it, just to keep things separate. Though also, heads that users can't, in general, reopen issues you close.]