Skip to content

Commit 6d71850

Browse files
philsccopybara-github
philsc
authored and
copybara-github
committed
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 bazelbuild#15271 Closes bazelbuild#15426. PiperOrigin-RevId: 462678126 Change-Id: I6cd6d4d9936bb1dde6a1282399ed4adc389ceed1
1 parent 3b1c8ce commit 6d71850

3 files changed

Lines changed: 173 additions & 0 deletions

File tree

‎src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import com.google.devtools.build.lib.analysis.DuplicateException;
3939
import com.google.devtools.build.lib.analysis.ExecGroupCollection;
4040
import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException;
41+
import com.google.devtools.build.lib.analysis.IncompatiblePlatformProvider;
4142
import com.google.devtools.build.lib.analysis.InconsistentAspectOrderException;
4243
import com.google.devtools.build.lib.analysis.ResolvedToolchainContext;
4344
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
@@ -191,6 +192,18 @@ public SkyValue compute(SkyKey skyKey, Environment env)
191192
ConfiguredTarget associatedTarget = state.initialValues.associatedTarget;
192193
Target target = state.initialValues.target;
193194

195+
// If the target is incompatible, then there's not much to do. The intent here is to create an
196+
// AspectValue that doesn't trigger any of the associated target's dependencies to be evaluated
197+
// against this aspect.
198+
if (associatedTarget.get(IncompatiblePlatformProvider.PROVIDER) != null) {
199+
return new AspectValue(
200+
key,
201+
aspect,
202+
target.getLocation(),
203+
ConfiguredAspect.forNonapplicableTarget(),
204+
state.transitivePackagesForPackageRootResolution.build());
205+
}
206+
194207
if (AliasProvider.isAlias(associatedTarget)) {
195208
return createAliasAspect(
196209
env,

‎src/main/java/com/google/devtools/build/lib/skyframe/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ java_library(
261261
"//src/main/java/com/google/devtools/build/lib/analysis:dependency_kind",
262262
"//src/main/java/com/google/devtools/build/lib/analysis:duplicate_exception",
263263
"//src/main/java/com/google/devtools/build/lib/analysis:exec_group_collection",
264+
"//src/main/java/com/google/devtools/build/lib/analysis:incompatible_platform_provider",
264265
"//src/main/java/com/google/devtools/build/lib/analysis:inconsistent_aspect_order_exception",
265266
"//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
266267
"//src/main/java/com/google/devtools/build/lib/analysis:platform_options",

‎src/test/shell/integration/target_compatible_with_test.sh‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1082,4 +1082,163 @@ function test_aquery_incompatible_target() {
10821082
expect_log "target platform (//target_skipping:foo3_platform) didn't satisfy constraint //target_skipping:foo1"
10831083
}
10841084
1085+
# Use aspects to interact with incompatible targets and validate the behaviour.
1086+
function test_aspect_skipping() {
1087+
cat >> target_skipping/BUILD <<EOF
1088+
load(":defs.bzl", "basic_rule", "rule_with_aspect")
1089+
# This target is compatible with all platforms and configurations. This target
1090+
# exists to validate the behaviour of aspects running against incompatible
1091+
# targets. The expectation is that the aspect should _not_ propagate to this
1092+
# compatible target from an incomaptible target. I.e. an aspect should _not_
1093+
# evaluate this target if "basic_foo3_target" is incompatible.
1094+
basic_rule(
1095+
name = "basic_universal_target",
1096+
)
1097+
# An alias to validate that incompatible target skipping works as expected with
1098+
# aliases and aspects.
1099+
alias(
1100+
name = "aliased_basic_universal_target",
1101+
actual = ":basic_universal_target",
1102+
)
1103+
basic_rule(
1104+
name = "basic_foo3_target",
1105+
deps = [
1106+
":aliased_basic_universal_target",
1107+
],
1108+
target_compatible_with = [
1109+
":foo3",
1110+
],
1111+
)
1112+
# This target is only compatible when "basic_foo3_target" is compatible. This
1113+
# target exists to validate the behaviour of aspects running against
1114+
# incompatible targets. The expectation is that the aspect should _not_
1115+
# evaluate this target when "basic_foo3_target" is incompatible.
1116+
basic_rule(
1117+
name = "other_basic_target",
1118+
deps = [
1119+
":basic_foo3_target",
1120+
],
1121+
)
1122+
alias(
1123+
name = "aliased_other_basic_target",
1124+
actual = ":other_basic_target",
1125+
)
1126+
rule_with_aspect(
1127+
name = "inspected_foo3_target",
1128+
inspect = ":aliased_other_basic_target",
1129+
)
1130+
basic_rule(
1131+
name = "previously_inspected_basic_target",
1132+
deps = [
1133+
":inspected_foo3_target",
1134+
],
1135+
)
1136+
rule_with_aspect(
1137+
name = "twice_inspected_foo3_target",
1138+
inspect = ":previously_inspected_basic_target",
1139+
)
1140+
genrule(
1141+
name = "generated_file",
1142+
outs = ["generated_file.txt"],
1143+
cmd = "echo '' > \$(OUTS)",
1144+
target_compatible_with = [
1145+
":foo1",
1146+
],
1147+
)
1148+
rule_with_aspect(
1149+
name = "inspected_generated_file",
1150+
inspect = ":generated_file",
1151+
)
1152+
EOF
1153+
cat > target_skipping/defs.bzl <<EOF
1154+
BasicProvider = provider()
1155+
def _basic_rule_impl(ctx):
1156+
return [DefaultInfo(), BasicProvider()]
1157+
basic_rule = rule(
1158+
implementation = _basic_rule_impl,
1159+
attrs = {
1160+
"deps": attr.label_list(
1161+
providers = [BasicProvider],
1162+
),
1163+
},
1164+
)
1165+
def _inspecting_aspect_impl(target, ctx):
1166+
print("Running aspect on " + str(target))
1167+
return []
1168+
_inspecting_aspect = aspect(
1169+
implementation = _inspecting_aspect_impl,
1170+
attr_aspects = ["deps"],
1171+
)
1172+
def _rule_with_aspect_impl(ctx):
1173+
out = ctx.actions.declare_file(ctx.label.name)
1174+
ctx.actions.write(out, "")
1175+
return [
1176+
DefaultInfo(files=depset([out])),
1177+
BasicProvider(),
1178+
]
1179+
rule_with_aspect = rule(
1180+
implementation = _rule_with_aspect_impl,
1181+
attrs = {
1182+
"inspect": attr.label(
1183+
aspects = [_inspecting_aspect],
1184+
),
1185+
},
1186+
)
1187+
EOF
1188+
cd target_skipping || fail "couldn't cd into workspace"
1189+
local debug_message1="Running aspect on <target //target_skipping:basic_universal_target>"
1190+
local debug_message2="Running aspect on <target //target_skipping:basic_foo3_target>"
1191+
local debug_message3="Running aspect on <target //target_skipping:other_basic_target>"
1192+
local debug_message4="Running aspect on <target //target_skipping:previously_inspected_basic_target>"
1193+
local debug_message5="Running aspect on <target //target_skipping:generated_file>"
1194+
# Validate that aspects run against compatible targets.
1195+
bazel build \
1196+
--show_result=10 \
1197+
--host_platform=@//target_skipping:foo3_platform \
1198+
--platforms=@//target_skipping:foo3_platform \
1199+
//target_skipping:all &> "${TEST_log}" \
1200+
|| fail "Bazel failed unexpectedly."
1201+
expect_log "${debug_message1}"
1202+
expect_log "${debug_message2}"
1203+
expect_log "${debug_message3}"
1204+
expect_log "${debug_message4}"
1205+
expect_not_log "${debug_message5}"
1206+
# Invert the compatibility and validate that aspects run on the other targets
1207+
# now.
1208+
bazel build \
1209+
--show_result=10 \
1210+
--host_platform=@//target_skipping:foo1_bar1_platform \
1211+
--platforms=@//target_skipping:foo1_bar1_platform \
1212+
//target_skipping:all &> "${TEST_log}" \
1213+
|| fail "Bazel failed unexpectedly."
1214+
expect_not_log "${debug_message1}"
1215+
expect_not_log "${debug_message2}"
1216+
expect_not_log "${debug_message3}"
1217+
expect_not_log "${debug_message4}"
1218+
expect_log "${debug_message5}"
1219+
# Validate that explicitly trying to build a target with an aspect against an
1220+
# incompatible target produces the normal error message.
1221+
bazel build \
1222+
--show_result=10 \
1223+
--host_platform=@//target_skipping:foo1_bar1_platform \
1224+
--platforms=@//target_skipping:foo1_bar1_platform \
1225+
//target_skipping:twice_inspected_foo3_target &> "${TEST_log}" \
1226+
&& fail "Bazel passed unexpectedly."
1227+
# TODO(#15427): Should use expect_log_once here when the issue is fixed.
1228+
expect_log 'ERROR: Target //target_skipping:twice_inspected_foo3_target is incompatible and cannot be built, but was explicitly requested.'
1229+
expect_log '^Dependency chain:$'
1230+
expect_log '^ //target_skipping:twice_inspected_foo3_target '
1231+
expect_log '^ //target_skipping:previously_inspected_basic_target '
1232+
expect_log '^ //target_skipping:inspected_foo3_target '
1233+
expect_log '^ //target_skipping:aliased_other_basic_target '
1234+
expect_log '^ //target_skipping:other_basic_target '
1235+
expect_log " //target_skipping:basic_foo3_target .* <-- target platform (//target_skipping:foo1_bar1_platform) didn't satisfy constraint //target_skipping:foo3:"
1236+
expect_log 'FAILED: Build did NOT complete successfully'
1237+
expect_not_log "${debug_message1}"
1238+
expect_not_log "${debug_message2}"
1239+
expect_not_log "${debug_message3}"
1240+
expect_not_log "${debug_message4}"
1241+
expect_not_log "${debug_message5}"
1242+
}
1243+
10851244
run_suite "target_compatible_with tests"

0 commit comments

Comments
 (0)