Check target incompatibility before toolchain resolution

Known missing toolchains are a valid reason for declaring a target incompatible and should not result in the target failing to build rather than being skipped as incompatible.

This is realized by moving the check for target incompatibility to the beginning of `PrerequisiteProducer#evaluate`, which requires fetching the target platform and config conditions.

Closes #17947.

PiperOrigin-RevId: 522534102
Change-Id: I9870edbf9cdb9873641f63610c86eb642f5c1d29

Author: fmeum

src/main/java/com/google/devtools/build/lib/analysis/BUILD

@@ -331,6 +331,7 @@ java_library(
         ":make_variable_supplier",
         ":options_diff_predicate",
         ":package_specification_provider",
+        ":platform_configuration",
         ":platform_options",
         ":provider_collection",
         ":repo_mapping_manifest_action",

src/main/java/com/google/devtools/build/lib/analysis/Util.java

@@ -96,6 +96,20 @@ public static ImmutableSet<ConfiguredTargetKey> findImplicitDeps(RuleContext rul
       }
     }
 
+    if (ruleContext.getRule().useToolchainResolution()) {
+      // Rules that participate in toolchain resolution implicitly depend on the target platform to
+      // check whether it matches the constraints in the target_compatible_with attribute.
+      if (ruleContext.getConfiguration().hasFragment(PlatformConfiguration.class)) {
+        PlatformConfiguration platformConfiguration =
+            ruleContext.getConfiguration().getFragment(PlatformConfiguration.class);
+        maybeImplicitDeps.add(
+            ConfiguredTargetKey.builder()
+                .setLabel(platformConfiguration.getTargetPlatform())
+                .setConfiguration(ruleContext.getConfiguration())
+                .build());
+      }
+    }
+
     ToolchainCollection<ResolvedToolchainContext> toolchainContexts =
         ruleContext.getToolchainContexts();
     if (toolchainContexts != null) {

src/main/java/com/google/devtools/build/lib/analysis/constraints/IncompatibleTargetChecker.java

@@ -98,10 +98,12 @@ public class IncompatibleTargetChecker {
    * </ul>
    */
   public static class IncompatibleTargetProducer implements StateMachine, Consumer<SkyValue> {
+
     private final Target target;
     @Nullable // Non-null when the target has an associated rule.
     private final BuildConfigurationValue configuration;
     private final ConfigConditions configConditions;
+    // Non-null when the target has an associated rule and does not opt out of toolchain resolution.
     @Nullable private final PlatformInfo platformInfo;
     @Nullable private final NestedSetBuilder<Package> transitivePackages;
 
@@ -133,7 +135,7 @@ public IncompatibleTargetProducer(
     @Override
     public StateMachine step(Tasks tasks, ExtendedEventHandler listener) {
       Rule rule = target.getAssociatedRule();
-      if (rule == null || rule.getRuleClass().equals("toolchain") || platformInfo == null) {
+      if (rule == null || !rule.useToolchainResolution() || platformInfo == null) {
         sink.accept(Optional.empty());
         return DONE;
       }

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

@@ -318,6 +318,41 @@ public boolean evaluate(
     //  would exit this SkyFunction and restart it when permits were available.
     semaphoreLocker.acquireSemaphore();
     try {
+      // Check target compatibility before requesting toolchains - known missing toolchains are a
+      // valid reason for declaring a target incompatible and should not result in the target
+      // failing to build rather than being skipped as incompatible.
+      // Non-rule targets and those that are part of the toolchain resolution system do not support
+      // target compatibility checking anyway.
+      if (targetAndConfiguration.getTarget() instanceof Rule
+          && ((Rule) targetAndConfiguration.getTarget()).useToolchainResolution()) {
+        platformInfo = loadTargetPlatformInfo(env);
+        // loadTargetPlatformInfo may return null even when no deps are missing.
+        if (env.valuesMissing()) {
+          return false;
+        }
+
+        configConditions =
+            computeConfigConditions(
+                env,
+                targetAndConfiguration,
+                transitivePackages,
+                platformInfo,
+                transitiveRootCauses);
+        if (configConditions == null) {
+          return false;
+        }
+
+        if (!checkForIncompatibleTarget(
+            env,
+            state,
+            targetAndConfiguration,
+            configConditions,
+            platformInfo,
+            transitivePackages)) {
+          return false;
+        }
+      }
+
       // Determine what toolchains are needed by this target.
       ComputedToolchainContexts result =
           computeUnloadedToolchainContexts(
@@ -335,9 +370,17 @@ public boolean evaluate(
           unloadedToolchainContexts != null ? unloadedToolchainContexts.getTargetPlatform() : null;
 
       // Get the configuration targets that trigger this rule's configurable attributes.
-      configConditions =
-          computeConfigConditions(
-              env, targetAndConfiguration, transitivePackages, platformInfo, transitiveRootCauses);
+      // Has been computed as part of checking for incompatible targets at the beginning of this
+      // function unless the current target is not a rule participating in toolchain resolution.
+      if (configConditions == null) {
+        configConditions =
+            computeConfigConditions(
+                env,
+                targetAndConfiguration,
+                transitivePackages,
+                platformInfo,
+                transitiveRootCauses);
+      }
       if (configConditions == null) {
         return false;
       }
@@ -361,10 +404,6 @@ public boolean evaluate(
                 getPrioritizedDetailedExitCode(causes)));
       }
 
-      if (!checkForIncompatibleTarget(env, state, transitivePackages)) {
-        return false;
-      }
-
       // Calculate the dependencies of this target.
       depValueMap =
           computeDependencies(
@@ -405,13 +444,44 @@ public boolean evaluate(
     return true;
   }
 
+  // May return null even when no deps are missing, use env.valuesMissing() to check.
+  @Nullable
+  private PlatformInfo loadTargetPlatformInfo(Environment env)
+      throws InterruptedException, ToolchainException {
+    PlatformConfiguration platformConfiguration =
+        targetAndConfiguration.getConfiguration().getFragment(PlatformConfiguration.class);
+    if (platformConfiguration == null) {
+      // No restart required in this case.
+      return null;
+    }
+    Label targetPlatformLabel = platformConfiguration.getTargetPlatform();
+    ConfiguredTargetKey targetPlatformKey =
+        ConfiguredTargetKey.builder()
+            .setLabel(targetPlatformLabel)
+            .setConfiguration(targetAndConfiguration.getConfiguration())
+            .build();
+
+    Map<ConfiguredTargetKey, PlatformInfo> platformInfoMap =
+        PlatformLookupUtil.getPlatformInfo(ImmutableList.of(targetPlatformKey), env);
+    if (platformInfoMap == null) {
+      return null;
+    }
+
+    return platformInfoMap.get(targetPlatformKey);
+  }
+
   /**
    * Checks if a target is incompatible because of its "target_compatible_with" attribute.
    *
    * @return false if a {@code Skyframe} restart is needed.
    */
-  private boolean checkForIncompatibleTarget(
-      Environment env, State state, @Nullable NestedSetBuilder<Package> transitivePackages)
+  private static boolean checkForIncompatibleTarget(
+      Environment env,
+      State state,
+      TargetAndConfiguration targetAndConfiguration,
+      @Nullable ConfigConditions configConditions,
+      @Nullable PlatformInfo targetPlatformInfo,
+      @Nullable NestedSetBuilder<Package> transitivePackages)
       throws InterruptedException, IncompatibleTargetException {
     if (state.incompatibleTarget == null) {
       if (state.incompatibleTargetProducer == null) {
@@ -421,7 +491,7 @@ private boolean checkForIncompatibleTarget(
                     targetAndConfiguration.getTarget(),
                     targetAndConfiguration.getConfiguration(),
                     configConditions,
-                    platformInfo,
+                    targetPlatformInfo,
                     transitivePackages,
                     state));
       }

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

@@ -46,6 +46,7 @@
 import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent;
 import com.google.devtools.build.lib.skyframe.ActionLookupConflictFindingFunction;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestConstants.InternalTestExecutionMode;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.util.Pair;
@@ -502,7 +503,9 @@ public void testGetDirectPrerequisites() throws Exception {
     Iterable<Label> labels = Iterables.transform(targets, TransitiveInfoCollection::getLabel);
     assertThat(labels)
         .containsExactly(
-            Label.parseCanonical("//package:inner"), Label.parseCanonical("//package:file"));
+            Label.parseCanonical("//package:inner"),
+            Label.parseCanonical("//package:file"),
+            Label.parseCanonical(TestConstants.PLATFORM_LABEL));
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java

@@ -424,6 +424,9 @@ public void testToolsAreExecConfiguration() throws Exception {
           assertThat(getConfiguration(prereq)).isNull();
           foundSetup = true;
           break;
+        case "host":
+          // Ignore the dependency on the target platform.
+          break;
         default:
           fail("unexpected prerequisite " + prereq + " (name: " + name + ")");
       }

src/test/java/com/google/devtools/build/lib/metrics/MetricsCollectorTest.java

@@ -619,8 +619,8 @@ public void skymeldNullIncrementalBuild_buildGraphMetricsCollected() throws Exce
     addOptions("--experimental_merged_skyframe_analysis_execution");
     BuildGraphMetrics expected =
         BuildGraphMetrics.newBuilder()
-            .setActionLookupValueCount(3)
-            .setActionLookupValueCountNotIncludingAspects(3)
+            .setActionLookupValueCount(8)
+            .setActionLookupValueCountNotIncludingAspects(8)
             .setActionCount(2)
             .setActionCountNotIncludingAspects(2)
             .setInputFileConfiguredTargetCount(1)

src/test/shell/integration/target_compatible_with_test.sh

@@ -1609,4 +1609,33 @@ EOF
   expect_not_log "${debug_message5}"
 }
 
+function test_skipping_with_missing_toolchain() {
+  mkdir -p missing_toolchain
+
+  cat > missing_toolchain/BUILD <<'EOF'
+load(":rule.bzl", "my_rule")
+
+toolchain_type(name = "my_toolchain_type")
+
+my_rule(
+    name = "my_rule",
+    target_compatible_with = ["@platforms//:incompatible"],
+)
+EOF
+
+  cat > missing_toolchain/rule.bzl <<'EOF'
+def _my_rule_impl(ctx):
+    pass
+
+my_rule = rule(
+    _my_rule_impl,
+    toolchains = ["//missing_toolchain:my_toolchain_type"],
+)
+EOF
+
+  bazel build --show_result=10 //missing_toolchain:all &> "${TEST_log}" \
+    || fail "Bazel failed unexpectedly."
+  expect_log "Target //missing_toolchain:my_rule was skipped"
+}
+
 run_suite "target_compatible_with tests"