Fix check for exec groups in DependencyResolver
In `DependencyResolver`, if the aspects list passed to [`dependentNodeMap()`](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java#L197) is not empty, the `toolchainContexts` will contain only the exec groups of the main aspect (placed as the last aspect in aspects list) ([from AspectFunction](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java#L596)). Otherwise, `toolchainContexts` will contain the exec group of the target's rule ([from ConfiguredTargetFunction](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java#L587)). Therefore, [this check](https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java#L349) needs to be updated so that if the aspects list is not empty and the current dependency is not a dependency of the main aspect, no need to check as the exec group will always not be in `toolchainContexts`.
PiperOrigin-RevId: 499270890
Change-Id: Ibe657eb7f3ec3721e816cf4a9b4b86af58957730
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index 87156b9..3667b5e 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -271,6 +271,7 @@
throws Failure {
OrderedSetMultimap<DependencyKind, PartiallyResolvedDependency> partiallyResolvedDeps =
OrderedSetMultimap.create();
+ ImmutableList<Aspect> aspectsList = ImmutableList.copyOf(aspects);
for (Map.Entry<DependencyKind, Label> entry : outgoingLabels.entries()) {
Label toLabel = entry.getValue();
@@ -334,11 +335,8 @@
Attribute attribute = entry.getKey().getAttribute();
ImmutableList.Builder<Aspect> propagatingAspects = ImmutableList.builder();
propagatingAspects.addAll(attribute.getAspects(fromRule));
- collectPropagatingAspects(
- ImmutableList.copyOf(aspects),
- attribute.getName(),
- entry.getKey().getOwningAspect(),
- propagatingAspects);
+ AspectClass owningAspect = entry.getKey().getOwningAspect();
+ collectPropagatingAspects(aspectsList, attribute.getName(), owningAspect, propagatingAspects);
Label executionPlatformLabel = null;
// TODO(jcater): refactor this nested if structure into something simpler.
@@ -347,11 +345,25 @@
String execGroup =
((ExecutionTransitionFactory) attribute.getTransitionFactory()).getExecGroup();
if (!toolchainContexts.hasToolchainContext(execGroup)) {
- throw new Failure(
- fromRule != null ? fromRule.getLocation() : null,
- String.format(
- "Attr '%s' declares a transition for non-existent exec group '%s'",
- attribute.getName(), execGroup));
+ // If {@code aspectsList} is not empty, {@code toolchainContexts} contains only the exec
+ // groups of the main aspect (placed as the last aspect in {@code aspectsList}).
+ // Otherwise, {@code toolchainContexts} contains the exec group of the target's rule.
+ // Therefore, if the {@aspectsList} is not empty and the current entry is not a
+ // dependency of the main aspect, {@code execGroup} will never exist in {@code
+ // toolchainContexts} and this dependency will be skipped.
+ // TODO(b/256617733): Make a decision on whether the exec groups of the target
+ // and the base aspects should be merged in {@code toolchainContexts}.
+ if (aspectsList.isEmpty()
+ || (owningAspect != null
+ && owningAspect.equals(Iterables.getLast(aspects).getAspectClass()))) {
+ throw new Failure(
+ fromRule != null ? fromRule.getLocation() : null,
+ String.format(
+ "Attr '%s' declares a transition for non-existent exec group '%s'",
+ attribute.getName(), execGroup));
+ } else {
+ continue;
+ }
}
if (toolchainContexts.getToolchainContext(execGroup).executionPlatform() != null) {
executionPlatformLabel =
diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh
index 83eeb16..39833ff 100755
--- a/src/test/shell/integration/aspect_test.sh
+++ b/src/test/shell/integration/aspect_test.sh
@@ -1280,4 +1280,202 @@
expect_not_log 'aspect_a on target @//test:t3, aspects_path = \["//test:defs.bzl%aspect_b\[p=\\\"4\\\"\]", "//test:defs.bzl%aspect_b\[p=\\\"3\\\"\]", "//test:defs.bzl%aspect_a"\]'
}
+function test_aspect_on_target_with_exec_gp() {
+ local package="test"
+ mkdir -p "${package}"
+
+ cat > "${package}/defs.bzl" <<EOF
+def _aspect_a_impl(target, ctx):
+ print('aspect_a on target {}'.format(target.label))
+ return []
+
+aspect_a = aspect(
+ implementation = _aspect_a_impl,
+ attr_aspects = ['deps'],
+)
+
+def _rule_impl(ctx):
+ pass
+
+r1 = rule(
+ implementation = _rule_impl,
+ attrs = {
+ 'deps' : attr.label_list(aspects=[aspect_a]),
+ },
+)
+
+r2 = rule(
+ implementation = _rule_impl,
+ attrs = {
+ '_tool' : attr.label(
+ default = "//${package}:tool",
+ cfg = config.exec(exec_group = "exec_gp")),
+ },
+ exec_groups = {"exec_gp": exec_group()},
+)
+EOF
+
+ cat > "${package}/tool.sh" <<EOF
+EOF
+
+ cat > "${package}/BUILD" <<EOF
+load('//test:defs.bzl', 'r1', 'r2')
+r1(
+ name = 't1',
+ deps = [':t2'],
+)
+r2(
+ name = 't2',
+)
+
+sh_binary(name = "tool", srcs = ["tool.sh"])
+EOF
+
+ bazel build "//${package}:t1" \
+ &> $TEST_log || fail "Build failed"
+
+ expect_log 'aspect_a on target @//test:t2'
+}
+
+function test_aspect_on_aspect_with_exec_gp() {
+ local package="test"
+ mkdir -p "${package}"
+
+ cat > "${package}/defs.bzl" <<EOF
+prov_b = provider()
+
+def _aspect_a_impl(target, ctx):
+ print('aspect_a on target {}'.format(target.label))
+ return []
+
+aspect_a = aspect(
+ implementation = _aspect_a_impl,
+ attr_aspects = ['deps'],
+ required_aspect_providers = [prov_b],
+)
+
+def _aspect_b_impl(target, ctx):
+ print('aspect_b on target {}'.format(target.label))
+ return [prov_b()]
+
+aspect_b = aspect(
+ implementation = _aspect_b_impl,
+ attr_aspects = ['deps'],
+ provides = [prov_b],
+ attrs = {
+ '_tool' : attr.label(
+ default = "//${package}:tool",
+ cfg = config.exec(exec_group = "exec_gp")),
+ },
+ exec_groups = {"exec_gp": exec_group()},
+)
+
+def _rule_impl(ctx):
+ pass
+
+r1 = rule(
+ implementation = _rule_impl,
+ attrs = {
+ 'deps' : attr.label_list(aspects=[aspect_b, aspect_a]),
+ },
+)
+
+r2 = rule(
+ implementation = _rule_impl,
+)
+EOF
+
+ cat > "${package}/tool.sh" <<EOF
+EOF
+
+ cat > "${package}/BUILD" <<EOF
+load('//test:defs.bzl', 'r1', 'r2')
+r1(
+ name = 't1',
+ deps = [':t2'],
+)
+r2(
+ name = 't2',
+)
+
+sh_binary(name = "tool", srcs = ["tool.sh"])
+EOF
+
+ bazel build "//${package}:t1" \
+ &> $TEST_log || fail "Build failed"
+
+ expect_log 'aspect_a on target @//test:t2'
+ expect_log 'aspect_b on target @//test:t2'
+}
+
+function test_aspect_on_aspect_with_missing_exec_gp() {
+ local package="test"
+ mkdir -p "${package}"
+
+ cat > "${package}/defs.bzl" <<EOF
+prov_b = provider()
+
+def _aspect_a_impl(target, ctx):
+ print('aspect_a on target {}'.format(target.label))
+ return []
+
+aspect_a = aspect(
+ implementation = _aspect_a_impl,
+ attr_aspects = ['deps'],
+ required_aspect_providers = [prov_b],
+ attrs = {
+ '_tool' : attr.label(
+ default = "//${package}:tool",
+ # exec_gp not declared
+ cfg = config.exec(exec_group = "exec_gp")),
+ },
+)
+
+def _aspect_b_impl(target, ctx):
+ print('aspect_b on target {}'.format(target.label))
+ return [prov_b()]
+
+aspect_b = aspect(
+ implementation = _aspect_b_impl,
+ attr_aspects = ['deps'],
+ provides = [prov_b],
+)
+
+def _rule_impl(ctx):
+ pass
+
+r1 = rule(
+ implementation = _rule_impl,
+ attrs = {
+ 'deps' : attr.label_list(aspects=[aspect_b, aspect_a]),
+ },
+)
+
+r2 = rule(
+ implementation = _rule_impl,
+)
+EOF
+
+ cat > "${package}/tool.sh" <<EOF
+EOF
+
+ cat > "${package}/BUILD" <<EOF
+load('//test:defs.bzl', 'r1', 'r2')
+r1(
+ name = 't1',
+ deps = [':t2'],
+)
+r2(
+ name = 't2',
+)
+
+sh_binary(name = "tool", srcs = ["tool.sh"])
+EOF
+
+ bazel build "//${package}:t1" \
+ &> $TEST_log && fail "Build succeeded, expected to fail"
+
+ expect_log "Attr '\$tool' declares a transition for non-existent exec group 'exec_gp'"
+}
+
run_suite "Tests for aspects"
