Repository: bazelbuild/bazel
Issue: Aspects get falsely evaluated against incompatible targets
Labels: P2
team-Configurability

### Description of the bug:


It looks like aspects run on incompatible targets when they shouldn't.

I think the answer here is that the aspects should not run and instead just provide the `IncompatiblePlatformProvider` to signify that it's incompatible as well.

### What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.


The absolute minimal example:
`example.bzl`:
```python
def _custom_rule_impl(ctx):
    pass

custom_rule = rule(
    implementation = _custom_rule_impl,
)

def _example_aspect_impl(target, ctx):
    print("Running aspect on " + str(target))
    return []

_example_aspect = aspect(
    implementation = _example_aspect_impl,
    attr_aspects = ["dep"],
)

def _basic_rule_with_aspect_impl(ctx):
    pass

basic_rule_with_aspect = rule(
    implementation = _basic_rule_with_aspect_impl,
    attrs = {
        "dep": attr.label(
            aspects = [_example_aspect],
        ),
    },
)
```
`BUILD`:
```python
load(":example.bzl", "basic_rule_with_aspect", "custom_rule")

custom_rule(
    name = "target1",
    target_compatible_with = ["@platforms//:incompatible"],
)

basic_rule_with_aspect(
    name = "target2",
    dep = ":target1",
)
```
When you build this target, you'll see that the `print()` statement in the aspect gets executed:
```console
$ bazel build //:target2
DEBUG: /home/pschrader/work/test_repo/example.bzl:9:10: Running aspect on <target //:target1>
ERROR: Target //:target2 is incompatible and cannot be built, but was explicitly requested.
Dependency chain:
    //:target2
    //:target1   <-- target platform (@local_config_platform//:host) didn't satisfy constraint @platforms//:incompatible
INFO: Elapsed time: 0.070s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded, 2 targets configured)
```
This can lead to undesirable effects. Largely because the aspect evaluation can generate errors before bazel gets a chance to tell the user about the incompatible target being requested.

### Which operating system are you running Bazel on?


Ubuntu 18.04

### What is the output of `bazel info release`?


release 5.1.0

### If `bazel info release` returns `development version` or `(@non-git)`, tell us how you built Bazel.


N/A

### What's the output of `git remote get-url origin; git rev-parse master; git rev-parse HEAD` ?


```text
N/A
```


### Have you found anything relevant by searching the web?

_No response_

### Any other information, logs, or outputs that you want to share?

_No response_

Issue comments:
@gregestren , FYI.

I'll work on this once the robotics season is over at the end of this month and after I finish with #14096.
Makes total sense, and I agree. Thanks for the report and commitment, and good luck with robotics!

Pull request review comments:

Patch that resolved the issue:
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",


Test patch:
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"