From 807f2a1929e23b60b237c63fadb25af81de2e3c3 Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Tue, 16 Mar 2021 11:45:45 -0700
Subject: [PATCH] Fix Incompatible Target Skipping for test args

Before the fix, the test in this patch would error out with the
following message:

    ERROR: ...: in args attribute of sh_test rule //target_skipping:foo_test: label '//target_skipping:some_foo3_target' in $(location) expression expands to no files

The problem was that the `RunfilesSupport` class for the
`RunfilesProvider` instance was computing the arguments as the
incompatible target was being constructed. That meant that the
arguments get evaluated and it would try to resolve things like
`$(location :incompatible_dep)`. That can't really succeed so bazel
would throw an error.

This patch makes it so arguments are ignored when constructing an
incompatible target.

Closes #13219.

PiperOrigin-RevId: 363233765
---
 .../build/lib/analysis/RunfilesSupport.java   | 16 ++++++++++++++
 .../RuleContextConstraintSemantics.java       |  2 +-
 .../target_compatible_with_test.sh            | 22 ++++++++++++++++++-
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index ef74b411cb9d87..0fdf7f6566cb3d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -453,6 +453,22 @@ public static RunfilesSupport withExecutable(
         computeActionEnvironment(ruleContext));
   }
 
+  /**
+   * Creates and returns a {@link RunfilesSupport} object for the given rule and executable. This
+   * version discards all arguments. Only use this for <a
+   * href="https://docs.bazel.build/versions/master/platforms.html#skipping-incompatible-targets">Incompatible
+   * Target Skipping</a>.
+   */
+  public static RunfilesSupport withExecutableButNoArgs(
+      RuleContext ruleContext, Runfiles runfiles, Artifact executable) {
+    return RunfilesSupport.create(
+        ruleContext,
+        executable,
+        runfiles,
+        CommandLine.EMPTY,
+        computeActionEnvironment(ruleContext));
+  }
+
   private static CommandLine computeArgs(RuleContext ruleContext, CommandLine additionalArgs) {
     if (!ruleContext.getRule().isAttrDefined("args", Type.STRING_LIST)) {
       // Some non-_binary rules create RunfilesSupport instances; it is fine to not have an args
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 c2be9cd5251fce..ff38aa85d09c17 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
@@ -1024,7 +1024,7 @@ private static ConfiguredTarget createIncompatibleConfiguredTarget(
     if (!outputArtifacts.isEmpty()) {
       Artifact executable = outputArtifacts.get(0);
       RunfilesSupport runfilesSupport =
-          RunfilesSupport.withExecutable(ruleContext, runfiles, executable);
+          RunfilesSupport.withExecutableButNoArgs(ruleContext, runfiles, executable);
       builder.setRunfilesSupport(runfilesSupport, executable);
 
       ruleContext.registerAction(
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index 92ddb167c32c7f..d247dc1c5662d3 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -378,7 +378,10 @@ function atest_build_event_protocol() {
 # incompatible targets are themselves deemed incompatible and should therefore
 # not be built.
 function test_non_top_level_skipping() {
-  cat >> target_skipping/BUILD <<EOF
+  touch target_skipping/foo_test.sh
+  chmod +x target_skipping/foo_test.sh
+
+  cat >> target_skipping/BUILD <<'EOF'
 genrule(
     name = "genrule_foo1",
     target_compatible_with = [":foo1"],
@@ -391,6 +394,15 @@ sh_binary(
     srcs = ["foo1.sh"],
     target_compatible_with = [":foo2"],
 )
+
+# Make sure that using an incompatible target in Make variable substitution
+# doesn't produce an unexpected error.
+sh_test(
+    name = "foo_test",
+    srcs = ["foo_test.sh"],
+    data = [":some_foo3_target"],
+    args = ["$(location :some_foo3_target)"],
+)
 EOF
 
   cd target_skipping || fail "couldn't cd into workspace"
@@ -402,6 +414,14 @@ EOF
     //target_skipping:sh_foo2 &> "${TEST_log}" && fail "Bazel passed unexpectedly."
   expect_log 'ERROR: Target //target_skipping:sh_foo2 is incompatible and cannot be built, but was explicitly requested'
   expect_log 'FAILED: Build did NOT complete successfully'
+
+  bazel build \
+    --show_result=10 \
+    --host_platform=@//target_skipping:foo2_bar1_platform \
+    --platforms=@//target_skipping:foo2_bar1_platform \
+    //target_skipping:foo_test &> "${TEST_log}" && fail "Bazel passed unexpectedly."
+  expect_log 'ERROR: Target //target_skipping:foo_test is incompatible and cannot be built, but was explicitly requested'
+  expect_log 'FAILED: Build did NOT complete successfully'
 }
 
 # Validate that targets are skipped when the implementation is in Starlark