Prevent aspects from executing on incompatible targets (#15984)
* Prevent aspects from executing on incompatible targets
This patch prevents aspect functions from being evaluated when their
associated targets are incompatible. Otherwise aspects can generate
errors that should never be printed at all. For example, an aspect
evaluating an incompatible target may generate errors about missing
providers. This is not the desired behaviour.
The implementation in this patch short-circuits the `AspectValue`
creation if the associated target is incompatible.
I had tried to add an `IncompatiblePlatformProvider` to the
corresponding `ConfiguredAspect` instance, but then Bazel
complained about having duplicate `IncompatiblePlatformProvider`
instances.
Fixes #15271
(cherry picked from commit 6d71850f6d5e9989826114395d58377769f44e4b)
* Fix build and test
Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
index 2633d47..bb0446d 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
@@ -36,6 +36,7 @@
import com.google.devtools.build.lib.analysis.DependencyKind;
import com.google.devtools.build.lib.analysis.DuplicateException;
import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException;
+import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException;
import com.google.devtools.build.lib.analysis.PlatformOptions;
import com.google.devtools.build.lib.analysis.ResolvedToolchainContext;
@@ -292,6 +293,20 @@
throw new IllegalStateException("Name already verified", e);
}
+ // If the target is incompatible, then there's not much to do. The intent here is to create an
+ // AspectValue that doesn't trigger any of the associated target's dependencies to be evaluated
+ // against this aspect.
+ if (associatedTarget.get(IncompatiblePlatformProvider.PROVIDER) != null) {
+ return new AspectValue(
+ key,
+ aspect,
+ target.getLocation(),
+ ConfiguredAspect.forNonapplicableTarget(),
+ /*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder
+ .<Package>stableOrder()
+ .build());
+ }
+
if (AliasProvider.isAlias(associatedTarget)) {
return createAliasAspect(
env,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 468b129..e8b8287 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -258,6 +258,7 @@
"//src/main/java/com/google/devtools/build/lib/analysis:duplicate_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:exec_group_collection",
"//src/main/java/com/google/devtools/build/lib/analysis:extra_action_artifacts_provider",
+ "//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception",
"//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
"//src/main/java/com/google/devtools/build/lib/analysis:platform_options",
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index fd43268..d00895b 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -1066,4 +1066,163 @@
expect_log "target platform (//target_skipping:foo3_platform) didn't satisfy constraint //target_skipping:foo1"
}
+# Use aspects to interact with incompatible targets and validate the behaviour.
+function test_aspect_skipping() {
+ cat >> target_skipping/BUILD <<EOF
+load(":defs.bzl", "basic_rule", "rule_with_aspect")
+# This target is compatible with all platforms and configurations. This target
+# exists to validate the behaviour of aspects running against incompatible
+# targets. The expectation is that the aspect should _not_ propagate to this
+# compatible target from an incomaptible target. I.e. an aspect should _not_
+# evaluate this target if "basic_foo3_target" is incompatible.
+basic_rule(
+ name = "basic_universal_target",
+)
+# An alias to validate that incompatible target skipping works as expected with
+# aliases and aspects.
+alias(
+ name = "aliased_basic_universal_target",
+ actual = ":basic_universal_target",
+)
+basic_rule(
+ name = "basic_foo3_target",
+ deps = [
+ ":aliased_basic_universal_target",
+ ],
+ target_compatible_with = [
+ ":foo3",
+ ],
+)
+# This target is only compatible when "basic_foo3_target" is compatible. This
+# target exists to validate the behaviour of aspects running against
+# incompatible targets. The expectation is that the aspect should _not_
+# evaluate this target when "basic_foo3_target" is incompatible.
+basic_rule(
+ name = "other_basic_target",
+ deps = [
+ ":basic_foo3_target",
+ ],
+)
+alias(
+ name = "aliased_other_basic_target",
+ actual = ":other_basic_target",
+)
+rule_with_aspect(
+ name = "inspected_foo3_target",
+ inspect = ":aliased_other_basic_target",
+)
+basic_rule(
+ name = "previously_inspected_basic_target",
+ deps = [
+ ":inspected_foo3_target",
+ ],
+)
+rule_with_aspect(
+ name = "twice_inspected_foo3_target",
+ inspect = ":previously_inspected_basic_target",
+)
+genrule(
+ name = "generated_file",
+ outs = ["generated_file.txt"],
+ cmd = "echo '' > \$(OUTS)",
+ target_compatible_with = [
+ ":foo1",
+ ],
+)
+rule_with_aspect(
+ name = "inspected_generated_file",
+ inspect = ":generated_file",
+)
+EOF
+ cat > target_skipping/defs.bzl <<EOF
+BasicProvider = provider()
+def _basic_rule_impl(ctx):
+ return [DefaultInfo(), BasicProvider()]
+basic_rule = rule(
+ implementation = _basic_rule_impl,
+ attrs = {
+ "deps": attr.label_list(
+ providers = [BasicProvider],
+ ),
+ },
+)
+def _inspecting_aspect_impl(target, ctx):
+ print("Running aspect on " + str(target))
+ return []
+_inspecting_aspect = aspect(
+ implementation = _inspecting_aspect_impl,
+ attr_aspects = ["deps"],
+)
+def _rule_with_aspect_impl(ctx):
+ out = ctx.actions.declare_file(ctx.label.name)
+ ctx.actions.write(out, "")
+ return [
+ DefaultInfo(files=depset([out])),
+ BasicProvider(),
+ ]
+rule_with_aspect = rule(
+ implementation = _rule_with_aspect_impl,
+ attrs = {
+ "inspect": attr.label(
+ aspects = [_inspecting_aspect],
+ ),
+ },
+)
+EOF
+ cd target_skipping || fail "couldn't cd into workspace"
+ local debug_message1="Running aspect on <target //target_skipping:basic_universal_target>"
+ local debug_message2="Running aspect on <target //target_skipping:basic_foo3_target>"
+ local debug_message3="Running aspect on <target //target_skipping:other_basic_target>"
+ local debug_message4="Running aspect on <target //target_skipping:previously_inspected_basic_target>"
+ local debug_message5="Running aspect on <target //target_skipping:generated_file>"
+ # Validate that aspects run against compatible targets.
+ bazel build \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo3_platform \
+ --platforms=@//target_skipping:foo3_platform \
+ //target_skipping:all &> "${TEST_log}" \
+ || fail "Bazel failed unexpectedly."
+ expect_log "${debug_message1}"
+ expect_log "${debug_message2}"
+ expect_log "${debug_message3}"
+ expect_log "${debug_message4}"
+ expect_not_log "${debug_message5}"
+ # Invert the compatibility and validate that aspects run on the other targets
+ # now.
+ bazel build \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo1_bar1_platform \
+ --platforms=@//target_skipping:foo1_bar1_platform \
+ //target_skipping:all &> "${TEST_log}" \
+ || fail "Bazel failed unexpectedly."
+ expect_not_log "${debug_message1}"
+ expect_not_log "${debug_message2}"
+ expect_not_log "${debug_message3}"
+ expect_not_log "${debug_message4}"
+ expect_log "${debug_message5}"
+ # Validate that explicitly trying to build a target with an aspect against an
+ # incompatible target produces the normal error message.
+ bazel build \
+ --show_result=10 \
+ --host_platform=@//target_skipping:foo1_bar1_platform \
+ --platforms=@//target_skipping:foo1_bar1_platform \
+ //target_skipping:twice_inspected_foo3_target &> "${TEST_log}" \
+ && fail "Bazel passed unexpectedly."
+ # TODO(#15427): Should use expect_log_once here when the issue is fixed.
+ expect_log 'ERROR: Target //target_skipping:twice_inspected_foo3_target is incompatible and cannot be built, but was explicitly requested.'
+ expect_log '^Dependency chain:$'
+ expect_log '^ //target_skipping:twice_inspected_foo3_target$'
+ expect_log '^ //target_skipping:previously_inspected_basic_target$'
+ expect_log '^ //target_skipping:inspected_foo3_target$'
+ expect_log '^ //target_skipping:aliased_other_basic_target$'
+ expect_log '^ //target_skipping:other_basic_target$'
+ expect_log " //target_skipping:basic_foo3_target <-- target platform (//target_skipping:foo1_bar1_platform) didn't satisfy constraint //target_skipping:foo3$"
+ expect_log 'FAILED: Build did NOT complete successfully'
+ expect_not_log "${debug_message1}"
+ expect_not_log "${debug_message2}"
+ expect_not_log "${debug_message3}"
+ expect_not_log "${debug_message4}"
+ expect_not_log "${debug_message5}"
+}
+
run_suite "target_compatible_with tests"
