From 807f2a1929e23b60b237c63fadb25af81de2e3c3 Mon Sep 17 00:00:00 2001 From: Philipp Schrader 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 Incompatible + * Target Skipping. + */ + 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 <> 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