From 96a25514d49abe4f591751dff59b1439f40819d6 Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Wed, 2 Dec 2020 10:37:22 -0800
Subject: [PATCH] Fix builds for filegroup targets with incompatible
 dependencies

When building a `filegroup` with incompatible dependencies, the logic
was such that the incompatible dependencies would actually be built by
bazel. These dependencies only expose `FailAction`s so the build
failed.

This is caused by bazel skipping the incompatibility check for any
rules that don't participate in toolchain resolution.

For example, without the fix in `RuleContextConstraintSemantics.java`,
the added unit test fails with:

    ERROR: /home/phil/.cache/bazel/_bazel_phil/b4868b516be4e3bea4d221937a55ced4/sandbox/linux-sandbox/46/execroot/io_bazel/_tmp/c749fd38ac9a4ad6bd41e9653bbceab5/workspace/target_skipping/BUILD:103:10: Reporting failed target //target_skipping:binary located at /home/phil/.cache/bazel/_bazel_phil/b4868b516be4e3bea4d221937a55ced4/sandbox/linux-sandbox/46/execroot/io_bazel/_tmp/c749fd38ac9a4ad6bd41e9653bbceab5/workspace/target_skipping/BUILD:103:10 failed: Can't build this. This target is incompatible. Please file a bug upstream. caused by //target_skipping:binary
    Target //target_skipping:filegroup failed to build

This patch makes it so targets that don't use toolchain resolution
skip the check for the `target_compatible_with` attribute, but still
check for incompatible dependencies.

Closes #12601.

PiperOrigin-RevId: 345263391
---
 .../RuleContextConstraintSemantics.java       |  9 ++--
 .../target_compatible_with_test.sh            | 45 +++++++++++++++++++
 2 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
index 42a38dd920b399..68697aed40cf8c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/RuleContextConstraintSemantics.java
@@ -854,12 +854,9 @@ public static ConfiguredTarget incompatibleConfiguredTarget(
       RuleContext ruleContext,
       OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap)
       throws ActionConflictException, InterruptedException {
-    if (!ruleContext.getRule().getRuleClassObject().useToolchainResolution()) {
-      return null;
-    }
-
-    // This is incompatible if explicitly specified to be.
-    if (ruleContext.attributes().has("target_compatible_with")) {
+    // The target (ruleContext) is incompatible if explicitly specified to be.
+    if (ruleContext.getRule().getRuleClassObject().useToolchainResolution()
+        && ruleContext.attributes().has("target_compatible_with")) {
       ImmutableList<ConstraintValueInfo> invalidConstraintValues =
           stream(
                   PlatformProviderUtils.constraintValues(
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index f71ca36d6884da..34e42b83752b69 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -252,6 +252,51 @@ function test_console_log_for_tests() {
   expect_log '^//target_skipping:pass_on_foo1_bar2  *  PASSED in'
 }
 
+# Validates that filegroups (i.e. a rule that doesn't use toolchain resolution)
+# is correctly skipped when it depends on an incompatible target. This is a
+# regression test for https://github.com/bazelbuild/bazel/issues/12582.
+function test_filegroup() {
+  cat > target_skipping/binary.cc <<EOF
+#include <cstdio>
+int main() {
+  return 0;
+}
+EOF
+
+  cat >> target_skipping/BUILD <<EOF
+cc_binary(
+    name = "binary",
+    srcs = ["binary.cc"],
+    target_compatible_with = [
+        ":foo3",
+    ],
+)
+
+filegroup(
+    name = "filegroup",
+    srcs = [
+        ":binary",
+    ],
+)
+EOF
+
+  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 \
+    :all &> "${TEST_log}" || fail "Bazel failed unexpectedly."
+  expect_log 'Target //target_skipping:filegroup was skipped'
+
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo1_bar1_platform \
+    --platforms=@//target_skipping:foo1_bar1_platform \
+    :filegroup &> "${TEST_log}" && fail "Bazel passed unexpectedly."
+  expect_log 'Target //target_skipping:filegroup is incompatible and cannot be built'
+}
+
 # Validates that incompatible target skipping errors behave nicely with
 # --keep_going. In other words, even if there's an error in the target skipping
 # (e.g. because the user explicitly requested an incompatible target) we still