Not possible to select() on constraint_value in alias rules

[staalb]

Description of the problem / feature request:

According to #8583, it should be possible to directly use a constraint_value in a select() expression. This seems not be the case for alias rules though.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

cc_binary(
    name = "appl",
    srcs = ["main.c"],
)   

constraint_value(
    name = "mycpu",
    constraint_setting = "@platforms//cpu:cpu",
)

alias(
    name = "foo",
    actual = select({
        ":mycpu" : ":appl",
    }),
)

When running bazel build :foo, the following error is produced:

ERROR: /tmp/bzl_alias/BUILD:12:6: //:mycpu is not a valid select() condition for //:foo.
To inspect the select(), run: bazel query --output=build //:foo.
For more help, see https://docs.bazel.build/be/functions.html#select.

ERROR: Analysis of target '//:foo' failed; build aborted: //:mycpu is not a valid select() condition for //:foo.
To inspect the select(), run: bazel query --output=build //:foo.
For more help, see https://docs.bazel.build/be/functions.html#select.

INFO: Elapsed time: 0.077s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 1 target configured)
FAILED: Build did NOT complete successfully (0 packages loaded, 1 target configured)

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

release 3.7.0, release 4.0.0

Have you found anything relevant by searching the web?

Tried hard, didn't find anything relevant.

Any other information, logs, or outputs that you want to share?

I studied the relevant code a little bit, found a comment that might be interesting:

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigConditions.java

Lines 73 to 74 in 79989f9

	// If platformInfo == null, that means the owning target doesn't invoke toolchain
	// resolution, in which case depending on a constraint_value is nonsensical.

If targetPlatform is null (which apparently is the case if using alias), there's some reason why this doesn't work which I don't fully understand. If this doesn't work by design, then it might be helpful to have this documented.

Workaround for me now is to use a cc_library with a select() on the deps attribute instead of an alias rule.

[joneshf]

Just passing by. I don't know either if this is intentional or an oversight, but another workaround is to define a config_setting that wraps the constraint_value and use that in the select:

@@ -3,6 +3,13 @@
     srcs = ["main.c"],
 )   
 
+config_setting(
+    name = "mycpu_config",
+    constraint_values = [
+        ":mycpu",
+    ],
+)
+
 constraint_value(
     name = "mycpu",
     constraint_setting = "@platforms//cpu:cpu",
@@ -11,6 +18,6 @@
 alias(
     name = "foo",
     actual = select({
-        ":mycpu" : ":appl",
+        ":mycpu_config" : ":appl",
     }),
 )

This allows using the alias in the spirit of the original motivation, though it means going through a layer of indirection.

[staalb]

but another workaround is to define a config_setting that wraps the constraint_value and use that in the select

Correct. However, as far as I understand the whole point of #8583 is to solve "Unnecessary bloat for situations where config_setting is just abstraction overhead", which is the case in this exampe.

[joneshf]

Oh geez, I only skimmed that other issue. I didn't realize I was basically copy/pasting what was already known. Sorry about the noise!

[gregestren]

Nice catch. I'm fairly certain this is the same as #12899 (filegroup had the same problem for the same reason). Also see #12899 (comment) specifically. FYI @katre .

[gregestren]

@katre: would removing this line

bazel/src/main/java/com/google/devtools/build/lib/rules/Alias.java

Lines 92 to 95 in 9868455

	// Aliases themselves do not need toolchains or an execution platform, so this is fine.
	// The actual target will resolve platforms and toolchains with no issues regardless of
	// this setting.
	.useToolchainResolution(false)

be a sufficient fix?

All: anyone want to try a PR with that change?

[staalb]

I tried the change, and it works for me. I'll prepare the change along with a simple test as a PR. Have to read about contributing / writing tests etc, so hold on for a moment ;)

[katre]

Thanks for looking into this. That change should be good, but we need to be sure to run full downstream tests. Assign the PR to me and I'll make sure the relevant test suites are run.

[staalb]

Forget the first PR, used the wrong credentials.

[katre]

Here's the comment I left on the PR, explaining the test failures, and why #13101 can't be submitted in its current form:

Okay, took a look at your test failures. I'm actually surprised that this passes anywhere. The underlying problem is the reason why we first exempted alias from toolchain resolution: there are aliases used in defining core constraints.

Take a look at the BUILD file for @bazel_tools//platforms. You'll see that these constraints are all actually aliases for canonical values from the @platforms repo. If alias targets required toolchain resolution, you'd see the following:

1. Bazel attempts to analyze a target, say //java/package:foo
2. Bazel needs to perform toolchain resolution for that target, so it begins analyzing the target platform. This then includes also analyzing the constraints for that platform.
   1. Bazel analyzes the platform and constraints in the same configutation as //java/package:foo. There's not actually a good reason for this, but we have to analyze them in some configuration, and this one is handy. We've discussed the concept of "null-configured targets" before, but didn't get very far with this because there are a lot of assumptions in Bazel about targets always having a configuration.
3. When Bazel analyzes one of the constraints that happens to be an alias, with your change it now needs to perform toolchain resolution
4. We now have a dependency cycle, because in order to finish toolchain resolution for the original target we've re-invoked toolchain resolution with essentially the same arguments. In your tests this shows up as an unreported dependency cycle, because there's no special handling for this case.

I'm afraid that your change isn't going to be submittable as-is without a lot of underlying changes to how Bazel handles toolchain resolution. I'm going to copy this comment back to the original issue so we can continue discussing potential solutions there.

[katre]

Possible solutions to the underlying problem:

1. Remove (and forbid) all uses of alias in platform and constraint definition. This is probably a non-starter, because people generally expect alias to be useful everywhere.
2. Revive the concept of the "null-configured target", which would (I think) allow us to cut short the cycle leading to duplicate toolchain resolution calls. (Null-configured targets would behave as if they had useToolchainResolution(false), just for that target.)
3. For toolchain resolution, transition the configuration to set a flag that disables further uses of toolchain resolution, thus short-circuiting the problem.

Of these, 2 is more elegant (and reduces the problem of having each platform configured multiple times for different configurations), but 3 might be easier to implement.

[philsc]

AFAICT this is also the reason that we can't specify constraints in the compatbility.all_off() helper that @trybka is working on: trybka/bazel-skylib#1

I.e. selects.config_setting_group() is implemented using aliases.

[gregestren]

I'd love to see this get solved. It's such a frustrating itch.

It seems 2. is the winning approach:

• it solves the problem
• it's something we want to do anyway, and makes Bazel (marginally) more efficient

@katre would having platform rules auto-set to the null configuration be sufficient? Like, would that automatically propagate to the constraint_values the platforms reference?

Then we'd need a check in ConfiguredTargetFunction to automatically dsiable toolchain resolution for any null-configured target. This is nicely consistent with the existing interpretation of 'null', for source files, which is that there's nothing to "do" with them aside from pull them into the graph as-is.

[gregestren]

Oh, I can think of approach 4 the classic "quick hack, probably works, terribly unrobust":

If an alias has a select(), perform platform resolution on it. Else don't. Then either assume or find some way to enforce that aliases on constraint_values don't use `select(), which today's instances don't.

[katre]

Oh, one more potential solution I just thought of: for an alias, resolve the target (not the configured target) pointed to by actual, and check to see if that rule supports toolchain resolution or not. Apply this recursively for alias chains.

We're going to need to load that target anyway when we start handling the alias's dependencies, so it's a reordering, not a new skyframe call.

[katre]

And, replying to @gregestren: yes, if the platform rules had no configuration, that would propagate to dependencies. I think we'd need to finish a few cleanups first, though, because there are still uses of platform auto-configuration.

[katre]

Greg and I discussed this with the team further this morning. Here are our takeaways:

1. Forbidding the use of alias in creating constraints and platforms continues to be unreasonable.
2. My idea to have alias check the target being pointed to to determine whether to perform toolchain resolution is flawed, because we'd need to resolve the aliased target, which would require full analysis including toolchain resolution.
3. Implementing null-configured targets for platform would likely work, but we still are using auto-configured platforms inside Google's monorepo, and would need to clean that up and migrate usages before we could consider null-configured platforms. This is a non-trivial task and would take dedicated effort, and we don't have anyone who can take that on currently.
4. @gregestren's idea to delay decisions on toolchain resolution for alias specifically might work, but will require some further investigation.