Fix a couple of bugs with Incompatible Target Skipping

While adding `target_compatible_with` attributes in the FRC team 971's
code base I came across bug bazelbuild#12553. It wasn't possible to have
transitive incompatibility for Python targets.

This patch fixes that issue by teaching `RuleContext` about
incompatible prerequisites.

I struggled a bit to come up with what ended up becoming
`RuleContextConstraintSemantics.IncompatibleCheckResult`. The purpose
of that class is to centralize the logic for checking for
`OutputFileConfiguredTarget`. When I implemented this, I realized that
the `TopLevelConstraintSemantics` logic also had a problem. It didn't
deal with the `OutputFileConfiguredTarget` problem at all. I took the
liberty of fixing that issue in this patch also because it nicely
re-used the same new helper.

I could not figure out a good way to avoid making `RuleContext` depend
on `RuleContextConstraintSemantics`.

Fixes bazelbuild#12553

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

@@ -54,6 +54,7 @@
 import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
 import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
 import com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics;
+import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
@@ -2292,6 +2293,14 @@ private boolean checkRuleDependencyMandatoryProviders(
         return true;
       }
 
+      if (RuleContextConstraintSemantics.checkForIncompatibility(prerequisite.getConfiguredTarget())
+          .isIncompatible) {
+        // If the prerequisite doesn't satisfy the required providers and it is incompatible (e.g.
+        // has an incompatible provider), we pretend that the required providers are satisfied.
+        // Otherwise we prevent bazel from skipping the target by erroring out too early.
+        return true;
+      }
+
       unfulfilledRequirements.add(
           String.format(
               "'%s' does not have mandatory providers: %s",

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

@@ -830,6 +830,42 @@ private static void addSelectValuesToSet(BuildType.Selector<?> select, final Set
     }
   }
 
+  /**
+   * Provides information about a target's incompatibility.
+   *
+   * <p>After calling {@code checkForIncompatibility()}, the {@code isIncompatible} flag tells you
+   * whether the target is incompatible. If the target is incompatible, then {@code
+   * underlyingTarget} tells you which underlying target provided the incompatibility. For the vast
+   * majority of targets this is the same one passed to {@code checkForIncompatibility()}. In some
+   * instances like {@link OutputFileConfiguredTarget}, however, the {@code underlyingTarget} is the
+   * rule that generated the file.
+   */
+  public static final class IncompatibleCheckResult {
+    public final boolean isIncompatible;
+    public final ConfiguredTarget underlyingTarget;
+
+    private IncompatibleCheckResult(boolean isIncompatible, ConfiguredTarget underlyingTarget) {
+      this.isIncompatible = isIncompatible;
+      this.underlyingTarget = underlyingTarget;
+    }
+  };
+
+  /**
+   * Checks whether the target is incompatible.
+   *
+   * <p>See the documentation for {@link RuleContextConstraintSemantics.IncompatibleCheckResult} for
+   * more information.
+   */
+  public static IncompatibleCheckResult checkForIncompatibility(ConfiguredTarget target) {
+    if (target instanceof OutputFileConfiguredTarget) {
+      // For generated files, we want to query the generating rule for providers. genrule() for
+      // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
+      target = ((OutputFileConfiguredTarget) target).getGeneratingRule();
+    }
+    return new IncompatibleCheckResult(
+        target.getProvider(IncompatiblePlatformProvider.class) != null, target);
+  }
+
   /**
    * Creates an incompatible {@link ConfiguredTarget} if the corresponding rule is incompatible.
    *
@@ -873,22 +909,12 @@ public static ConfiguredTarget incompatibleConfiguredTarget(
     }
 
     // This is incompatible if one of the dependencies is as well.
-    ImmutableList.Builder<ConfiguredTarget> incompatibleDependenciesBuilder =
-        ImmutableList.builder();
-    for (ConfiguredTargetAndData infoCollection : prerequisiteMap.values()) {
-      ConfiguredTarget dependency = infoCollection.getConfiguredTarget();
-      if (dependency instanceof OutputFileConfiguredTarget) {
-        // For generated files, we want to query the generating rule for providers. genrule() for
-        // example doesn't attach providers like IncompatiblePlatformProvider to its outputs.
-        dependency = ((OutputFileConfiguredTarget) dependency).getGeneratingRule();
-      }
-      if (dependency.getProvider(IncompatiblePlatformProvider.class) != null) {
-        incompatibleDependenciesBuilder.add(dependency);
-      }
-    }
-
     ImmutableList<ConfiguredTarget> incompatibleDependencies =
-        incompatibleDependenciesBuilder.build();
+        stream(prerequisiteMap.values())
+            .map(value -> checkForIncompatibility(value.getConfiguredTarget()))
+            .filter(result -> result.isIncompatible)
+            .map(result -> result.underlyingTarget)
+            .collect(toImmutableList());
     if (!incompatibleDependencies.isEmpty()) {
       return createIncompatibleConfiguredTarget(ruleContext, incompatibleDependencies, null);
     }
@@ -990,7 +1016,7 @@ private static ConfiguredTarget createIncompatibleConfiguredTarget(
           new FailAction(
               ruleContext.getActionOwner(),
               outputArtifacts,
-              "Can't build this. This target is incompatible.",
+              "Can't build this. This target is incompatible. Please file a bug upstream.",
               Code.CANT_BUILD_INCOMPATIBLE_TARGET));
     }
     return builder.build();

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

@@ -123,11 +123,11 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
     ImmutableSet.Builder<ConfiguredTarget> incompatibleButRequestedTargets = ImmutableSet.builder();
 
     for (ConfiguredTarget target : topLevelTargets) {
-      IncompatiblePlatformProvider incompatibleProvider =
-          target.getProvider(IncompatiblePlatformProvider.class);
+      RuleContextConstraintSemantics.IncompatibleCheckResult result =
+          RuleContextConstraintSemantics.checkForIncompatibility(target);
 
       // Move on to the next target if this one is compatible.
-      if (incompatibleProvider == null) {
+      if (!result.isIncompatible) {
         continue;
       }
 
@@ -143,7 +143,9 @@ public PlatformRestrictionsResult checkPlatformRestrictions(
               String.format(
                   TARGET_INCOMPATIBLE_ERROR_TEMPLATE,
                   target.getLabel().toString(),
-                  reportOnIncompatibility(target));
+                  // We need access to the provider so we pass in the underlying target here that is
+                  // responsible for the incompatibility.
+                  reportOnIncompatibility(result.underlyingTarget));
           throw new ViewCreationFailedException(
               targetIncompatibleMessage,
               FailureDetail.newBuilder()

src/test/shell/integration/target_compatible_with_test.sh

@@ -402,6 +402,51 @@ EOF
   expect_log 'FAILED: Build did NOT complete successfully'
 }
 
+# Validates that rules with custom providers are skipped when incompatible.
+# This is valuable because we use providers to convey incompatibility.
+function test_dependencies_with_providers() {
+  cat > target_skipping/rules.bzl <<EOF
+DummyProvider = provider()
+
+def _dummy_rule_impl(ctx):
+    return [DummyProvider()]
+
+dummy_rule = rule(
+    implementation = _dummy_rule_impl,
+    attrs = {
+        "deps": attr.label_list(providers=[DummyProvider]),
+    },
+)
+EOF
+
+  cat >> target_skipping/BUILD <<EOF
+load("//target_skipping:rules.bzl", "dummy_rule")
+
+dummy_rule(
+    name = "dummy1",
+    target_compatible_with = [
+        "//target_skipping:foo1",
+    ],
+)
+
+dummy_rule(
+    name = "dummy2",
+    deps = [
+        ":dummy1",
+    ],
+)
+EOF
+
+  cd target_skipping || fail "couldn't cd into workspace"
+
+  pwd >&2
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo3_platform \
+    --platforms=@//target_skipping:foo3_platform \
+    //target_skipping/... &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+}
+
 # Validates the same thing as test_non_top_level_skipping, but with a cc_test
 # and adding one more level of dependencies.
 function test_cc_test() {
@@ -435,6 +480,21 @@ EOF
 
   cd target_skipping || fail "couldn't cd into workspace"
 
+  # Validate the generated file that makes up the test.
+  bazel test \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo2_bar1_platform \
+    --platforms=@//target_skipping:foo2_bar1_platform \
+    //target_skipping:generated_test.cc &> "${TEST_log}" && fail "Bazel passed unexpectedly."
+  expect_log "ERROR: Target //target_skipping:generated_test.cc is incompatible and cannot be built, but was explicitly requested"
+
+  # Validate that we get the dependency chain printed out.
+  expect_log '^Dependency chain:$'
+  expect_log '^    //target_skipping:generate_with_tool$'
+  expect_log "^    //target_skipping:generator_tool   <-- target platform didn't satisfy constraint //target_skipping:foo1$"
+  expect_log 'FAILED: Build did NOT complete successfully'
+
+  # Validate the test.
   bazel test \
     --show_result=10 \
     --host_platform=@//target_skipping:foo2_bar1_platform \