Fix builds for filegroup targets with incompatible dependencies

philsc · copybara-github · commit 84fadcf81f81 · 2020-12-02T10:38:28.000-08:00
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 bazelbuild#12601. PiperOrigin-RevId: 345263391
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
@@ -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
@@ -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
