Make target_compatible_with work better on alias() targets

Currently the `target_compatible_with` on `alias()` targets only works
when a `select()` is involved. That's because `alias()` targets by
default don't have a toolchains evaluated.

This patch changes the behaviour by making it so all `alias()` targets
with a `target_compatible_with` attribute are skipped appropriately.
When `target_compatible_with` is present, then toolchains are
evaluated for `alias()` targets.

The implementation is basically an enhancement to @gregestren's
1c3a245 (#14310). In
addition to resolving toolchains when a `select()` is present, Bazel
will now also resolve toolchains when a `target_compatible_with`
attribute is set.

I also took this opportunity to rename `HAS_SELECT` to something
more appropriate. The name may be too long now, but I feel
that at least it's descriptive.

Fixes #17663

Closes #17983.

PiperOrigin-RevId: 522446438
Change-Id: I80bce3e840553b75960022545df7f27ad1d1d363

src/main/java/com/google/devtools/build/lib/packages/Rule.java

@@ -1188,18 +1188,21 @@ public Collection<Label> getAspectLabelsSuperset(DependencyFilter predicate) {
    * <ol>
    *   <li>The rule uses toolchains by definition ({@link
    *       RuleClass.Builder#useToolchainResolution(ToolchainResolutionMode)}
-   *   <li>The rule instance has a select(), which means it may depend on target platform properties
-   *       that are only provided when toolchain resolution is enabled.
+   *   <li>The rule instance has a select() or target_compatible_with attribute, which means it may
+   *       depend on target platform properties that are only provided when toolchain resolution is
+   *       enabled.
    * </ol>
    */
   public boolean useToolchainResolution() {
     ToolchainResolutionMode mode = ruleClass.useToolchainResolution();
     if (mode.isActive()) {
       return true;
-    } else if (mode == ToolchainResolutionMode.HAS_SELECT) {
+    } else if (mode == ToolchainResolutionMode.ENABLED_ONLY_FOR_COMMON_LOGIC) {
       RawAttributeMapper attr = RawAttributeMapper.of(this);
-      return (attr.has(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
-          && !attr.get(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, BuildType.LABEL_LIST).isEmpty());
+      return ((attr.has(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE)
+              && !attr.get(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE, BuildType.LABEL_LIST).isEmpty())
+          || (attr.has(RuleClass.TARGET_COMPATIBLE_WITH_ATTR)
+              && !attr.get(RuleClass.TARGET_COMPATIBLE_WITH_ATTR, BuildType.LABEL_LIST).isEmpty()));
     } else {
       return false;
     }

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

@@ -227,12 +227,15 @@ public enum ToolchainResolutionMode {
     /** The rule should not use toolchain resolution. */
     DISABLED,
     /**
-     * The rule instance uses toolchain resolution if it has a select().
+     * The rule instance uses toolchain resolution if it has a select() or has a
+     * target_compatible_with attribute.
      *
      * <p>This is for rules that don't intrinsically use toolchains but have select()s on {@link
-     * com.google.devtools.build.lib.rules.platform.ConstraintValue}, which are part of the build's
-     * platform. Such instances need to know what platform the build is targeting, which Bazel won't
-     * provide unless toolchain resolution is enabled.
+     * com.google.devtools.build.lib.rules.platform.ConstraintValue} or have a
+     * target_compatible_with attribute with {@link
+     * com.google.devtools.build.lib.rules.platform.ConstraintValue} targets, which are part of the
+     * build's platform. Such instances need to know what platform the build is targeting, which
+     * Bazel won't provide unless toolchain resolution is enabled.
      *
      * <p>This is set statically in rule definitions on an opt-in basis. Bazel doesn't automatically
      * infer this for any target with a select().
@@ -241,7 +244,7 @@ public enum ToolchainResolutionMode {
      * href="https://github.com/bazelbuild/bazel/issues/12899#issuecomment-767759147}#12899</a>is
      * addressed, so platforms are unconditionally provided for all rules.
      */
-    HAS_SELECT,
+    ENABLED_ONLY_FOR_COMMON_LOGIC,
     /** The rule should inherit the value from its parent rules. */
     INHERIT;
 
@@ -266,7 +269,7 @@ boolean isActive() {
         case ENABLED:
           return true;
         case DISABLED:
-        case HAS_SELECT: // Not true for RuleClass, but Rule may enable it.
+        case ENABLED_ONLY_FOR_COMMON_LOGIC: // Not true for RuleClass, but Rule may enable it.
           return false;
         default:
       }

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

@@ -76,9 +76,11 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
           .canHaveAnyProvider()
           // 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. The only time an alias directly needs the platform is when it has a
-          // select() on a constraint_setting, so special-case enable those instances too.
-          .useToolchainResolution(ToolchainResolutionMode.HAS_SELECT)
+          // this setting. In some circumstances an alias directly needs the platform:
+          // - when it has a select() on a constraint_setting, or
+          // - when it has a target_compatible_with attribute.
+          // Special-case enable those instances too.
+          .useToolchainResolution(ToolchainResolutionMode.ENABLED_ONLY_FOR_COMMON_LOGIC)
           .build();
     }
 

src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java

@@ -1426,7 +1426,7 @@ public void nonToolchainResolvingTargetsCantSelectDirectlyOnConstraints() throws
   public void selectOnlyToolchainResolvingTargetsCanSelectDirectlyOnConstraints() throws Exception {
     // Tests select()ing directly on a constraint_value when the rule uses toolchain resolution
     // *only if it has a select()*. As of this test, alias() is the only rule that supports that
-    // (see Alias#useToolchainResolution(ToolchainResolutionMode.HAS_SELECT).
+    // (see Alias#useToolchainResolution(ToolchainResolutionMode.ENABLED_ONLY_FOR_COMMON_LOGIC).
     scratch.file(
         "conditions/BUILD",
         "constraint_setting(name = 'fruit')",

src/test/shell/integration/target_compatible_with_test.sh

@@ -1042,6 +1042,40 @@ EOF
   expect_log 'ERROR: Build did NOT complete successfully'
 }
 
+# Validate what happens when setting `target_compatible_with` directly on an
+# alias(). This is a regression test for
+# https://github.com/bazelbuild/bazel/issues/17663.
+function test_alias_incompatibility() {
+  cat >> target_skipping/BUILD <<'EOF'
+filegroup(
+    name = "test_cc_filegroup",
+    srcs = ["test.cc"],
+)
+
+alias(
+    name = "test_cc_filegroup_alias",
+    actual = ":test_cc_filegroup",
+    target_compatible_with = [":foo3"],
+)
+
+cc_library(
+    name = "test_cc",
+    srcs = [":test_cc_filegroup_alias"],
+)
+EOF
+
+  echo > target_skipping/test.cc
+
+  cd target_skipping || fail "couldn't cd into workspace"
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo1_bar1_platform \
+    --platforms=@//target_skipping:foo1_bar1_platform \
+    //target_skipping:test_cc &> "${TEST_log}" \
+    && fail "Bazel passed unexpectedly"
+  expect_log_once 'ERROR: Target //target_skipping:test_cc is incompatible and cannot be built, but was explicitly requested.'
+}
+
 # Validate that an incompatible target with a toolchain not available for the
 # current platform will not cause an analysis error. This is a regression test
 # for https://github.com/bazelbuild/bazel/issues/12897.