Skip to content

Commit

Permalink
Prevent aspects from executing on incompatible targets (#15984)
Browse files Browse the repository at this point in the history
* 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 6d71850)

* Fix build and test

Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>
  • Loading branch information
philsc and ShreeM01 authored Jul 27, 2022
1 parent a24d1bc commit 4ae8538
Show file tree
Hide file tree
Showing 3 changed files with 175 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
159 changes: 159 additions & 0 deletions src/test/shell/integration/target_compatible_with_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"

0 comments on commit 4ae8538

Please sign in to comment.