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

Provide ability to directly select() on constraint_value #8583

Closed
gregestren opened this issue Jun 7, 2019 · 6 comments
Closed

Provide ability to directly select() on constraint_value #8583

gregestren opened this issue Jun 7, 2019 · 6 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions

Comments

@gregestren
Copy link
Contributor

Description of the problem / feature request:

A common and annoyingly redundant pattern is:

constraint_value(
    name = "my_value",
    constraint_setting = "my_setting")

config_setting(
    name = "my_value_selectable",
    constraint_values = [":my_value"])

some_rule(
    ...,
    attr = select({
        ":my_value_selectable": ...
    })

It would be nice to be able to just write:

some_rule(
    ...,
    attr = select({
        ":my_value": ...
    })

Feature requests: what underlying problem are you trying to solve with this feature?

Unnecessary bloat for situations where config_setting is just abstraction overhead.

Have you found anything relevant by searching the web?

Examples:

Also see #7668 and bazelbuild/bazel-skylib#89 (comment).

@gregestren gregestren added P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions labels Jun 7, 2019
@gregestren
Copy link
Contributor Author

@katre I tried mocking up an implementation of this since it's conceptually at least easy (and select was always designed to support rules beside config_setting).

Some challenges that arose that I appreciate your perspective on:

  1. config_setting implements

    /**
    * Rules that support platforms can use toolchains and execution platforms. Rules that are part
    * of configuring toolchains and platforms should set this to {@code false}.
    */
    public Builder supportsPlatforms(boolean flag) {
    which is necessary for it to always invoke toolchain resolution so there are actual target constraint_values to compare against. The constraint_value rule doesn't do that though. What options do we have here?

  2. What configuration do constraint_value rules evaluate in today? If in a select would they properly use the same configuration of the rule with the select in it?

  3. If this change involves evaluating constraint_value in more configurations, would that bloat the build graph? Or maybe just do this only in the context of a select? Maybe some secret alias rule that secretly sets a requires platforms just for this call option in PlatformOptions that constraint_value can specially consume?

@gregestren
Copy link
Contributor Author

@hlopko

@katre
Copy link
Member

katre commented Jun 7, 2019

  1. The supportsPlatforms method is badly named (and if I ever think of a better one, I will). It's intended to allow rules to opt out of toolchain resolution, because those rules are themselves used in toolchain resolution (in this case, toolchain resolution loads platforms, which the load constraint values). If constraint_value needs to access the current platform in order to correctly provide the matching provider, I think that will work, as long as it is using the target platform, not expecting a valid execution platform.

  2. Correct, the configuration is maintained. Someday I want to get rid of it, but we could just make it optional (and if there's no config, a false matching provider is returned?).

  3. Every (mostly) configured target already loads platforms and constraint values, so there shouldn't be a huge increase. Worth monitoring, however.

@gregestren
Copy link
Contributor Author

Promising, thanks! I'll try to whip this up into a proper PR in my spare time.

@hlopko
Copy link
Member

hlopko commented Jun 21, 2019

CC @jayconrod who wanted this too :)

@gregestren
Copy link
Contributor Author

Welp, I successfully prototyped this.

Went down a different path than above. I put the platform-retrieving logic in the target that owns the select(), not in the constraint_value. Putting it directly in constraint_value creates a dependency in that resolving the platform requires resolving its constraints.

I'm interested to explore this in review.

gregestren added a commit to gregestren/bazel that referenced this issue Sep 9, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants