From 309232853d3354155770cbfca1d7dae24479338c Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Fri, 22 Jul 2022 12:23:16 -0700
Subject: [PATCH 1/2] 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)
---
 .../build/lib/skyframe/AspectFunction.java    |  13 ++
 .../google/devtools/build/lib/skyframe/BUILD  |   1 +
 .../target_compatible_with_test.sh            | 159 ++++++++++++++++++
 3 files changed, 173 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..957465a7785c89 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,18 @@ 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(),
+          state.transitivePackagesForPackageRootResolution.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..13e0aae39bfd3d 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"

From 2430961069e7a2da5de84d62277f5c4bee660558 Mon Sep 17 00:00:00 2001
From: Philipp Schrader <philipp.schrader@gmail.com>
Date: Tue, 26 Jul 2022 16:28:17 -0700
Subject: [PATCH 2/2] Fix build and test

---
 .../devtools/build/lib/skyframe/AspectFunction.java  |  4 +++-
 .../shell/integration/target_compatible_with_test.sh | 12 ++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

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 957465a7785c89..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
@@ -302,7 +302,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           aspect,
           target.getLocation(),
           ConfiguredAspect.forNonapplicableTarget(),
-          state.transitivePackagesForPackageRootResolution.build());
+          /*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder
+              .<Package>stableOrder()
+              .build());
     }
 
     if (AliasProvider.isAlias(associatedTarget)) {
diff --git a/src/test/shell/integration/target_compatible_with_test.sh b/src/test/shell/integration/target_compatible_with_test.sh
index 13e0aae39bfd3d..d00895b6e302e4 100755
--- a/src/test/shell/integration/target_compatible_with_test.sh
+++ b/src/test/shell/integration/target_compatible_with_test.sh
@@ -1211,12 +1211,12 @@ EOF
   # 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 '^    //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}"