From 4ae85387e69db73e507b4f18b36d3e2f799e5d34 Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Wed, 27 Jul 2022 08:29:08 -0700
Subject: [PATCH] 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>
---
 .../build/lib/skyframe/AspectFunction.java    |  15 ++
 .../google/devtools/build/lib/skyframe/BUILD  |   1 +
 .../target_compatible_with_test.sh            | 159 ++++++++++++++++++
 3 files changed, 175 insertions(+)

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 2633d473afdea4..bb0446dba09d9b 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 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       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 468b1290e45e5b..e8b8287413765d 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 @@ java_library(
         "//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 fd432689c736be..d00895b6e302e4 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 @@ function test_aquery_incompatible_target() {
   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"