Skip to content

Commit

Permalink
Ignore base target toolchain dependencies if aspect is applied on out…
Browse files Browse the repository at this point in the history
…put file

when aspect is applied to an output file it does not propagate to its generating rule attribute dependencies and it should not propagate to its toolchain dependencies. The type of the target was mistakenly checked using `getAssociatedRule` which is not null if the target is `OutputFile`.

PiperOrigin-RevId: 684923928
Change-Id: Idb0ba4dec8b646f2f8aa09002ea75817eb4d6f81
  • Loading branch information
mai93 authored and copybara-github committed Oct 11, 2024
1 parent 8c7bedf commit 00fdb67
Show file tree
Hide file tree
Showing 3 changed files with 234 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -613,8 +613,7 @@ private ToolchainCollection<UnloadedToolchainContext> getBaseTargetUnloadedToolc
*/
private static boolean canAspectsPropagateToToolchains(
ImmutableList<Aspect> topologicalAspectPath, Target baseTarget) {
Rule rule = baseTarget.getAssociatedRule();
if (rule == null) {
if (!baseTarget.isRule()) {
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,62 @@ public AspectDefinition getDefinition(AspectParameters aspectParameters) {
}
}

/**
* An aspect that applies to output files and propagates to toolchain dependencies and attribute
* dependencies.
*/
public static class DepsVisitingFileAspect extends BaseAspect {

/** Test provider which includes the base target label. */
@SerializationConstant
public static final StarlarkProvider PROVIDER =
StarlarkProvider.builder(Location.BUILTIN)
.buildExported(new StarlarkProvider.Key(keyForBuild(FAKE_LABEL), "AspectProvider"));

private String depAttr;
private Label toolChainType;

public DepsVisitingFileAspect(String depAttr, String toolChainType) {
this.depAttr = depAttr;
this.toolChainType = Label.parseCanonicalUnchecked(toolChainType);
}

@Override
public ConfiguredAspect create(
Label targetLabel,
ConfiguredTarget ct,
RuleContext ruleContext,
AspectParameters parameters,
RepositoryName toolsRepository)
throws ActionConflictException, InterruptedException {
try {
return ConfiguredAspect.builder(ruleContext)
.addStarlarkDeclaredProvider(
StarlarkInfo.create(
PROVIDER, ImmutableMap.of("val", ct.getLabel().getCanonicalForm()), null))
.build();
} catch (EvalException e) {
throw new IllegalStateException(e);
}
}

@Override
public String getName() {
return String.format("%s_%s_%s", super.getName(), depAttr, toolChainType.getCanonicalForm());
}

@Override
public AspectDefinition getDefinition(AspectParameters aspectParameters) {
AspectDefinition.Builder aspectDefinition =
new AspectDefinition.Builder(this)
.applyToFiles(true)
.propagateAlongAttribute(depAttr)
.propagateToToolchainsTypes(ImmutableSet.of(toolChainType));

return aspectDefinition.build();
}
}

/** An aspect that defines its own implicit attribute, requiring PackageSpecificationProvider. */
public static class PackageGroupAttributeAspect extends BaseAspect {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.common.collect.Sets;
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.util.AnalysisTestCase;
import com.google.devtools.build.lib.analysis.util.TestAspects.DepsVisitingFileAspect;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.packages.StarlarkInfo;
import com.google.devtools.build.lib.packages.StarlarkProvider;
Expand Down Expand Up @@ -1457,7 +1458,7 @@ def _rule_2_impl(ctx):
""");
useConfiguration("--extra_toolchains=//toolchain:foo_toolchain_with_dep");

var analysisResult = update(ImmutableList.of("//test:defs.bzl%toolchain_aspect"), "//test:t1");
var analysisResult = update("//test:t1");

var configuredTarget = Iterables.getOnlyElement(analysisResult.getTargetsToBuild());

Expand All @@ -1473,6 +1474,181 @@ def _rule_2_impl(ctx):
"toolchain_aspect on @@//toolchain:toolchain_dep");
}

@Test
public void toolchainAspectOnOutputFile_notPropagatedToDeps() throws Exception {
scratch.file(
"test/defs.bzl",
"""
AspectProvider = provider()
def _impl(target, ctx):
return [AspectProvider(val="hi")]
toolchain_aspect = aspect(
implementation = _impl,
toolchains_aspects = ['//rule:toolchain_type_1'],
attr_aspects = ['dep'],
)
def _rule_1_impl(ctx):
if ctx.outputs.out:
ctx.actions.write(ctx.outputs.out, 'hi')
return []
r1 = rule(
implementation = _rule_1_impl,
attrs = {
"out": attr.output(),
"dep": attr.label(),
},
toolchains = ['//rule:toolchain_type_1'],
)
""");
scratch.file(
"test/BUILD",
"""
load('//test:defs.bzl', 'r1')
r1(name = 't1', out = 'my_out.txt', dep = ':t2')
r1(name = 't2')
""");
useConfiguration("--extra_toolchains=//toolchain:foo_toolchain");

var unused = update(ImmutableList.of("//test:defs.bzl%toolchain_aspect"), "//test:my_out.txt");

// {@link AspectKey} is created for toolchain_aspect on the output file //test:my_out.txt but
// the aspect is not applied (no returned providers) because the aspect cannot be applied to
// output files. The aspect does not propagate to any of the generating rule dependencies.
var nodes =
skyframeExecutor.getEvaluator().getDoneValues().entrySet().stream()
.filter(
entry ->
entry.getKey() instanceof AspectKey
&& ((AspectKey) entry.getKey())
.getAspectClass()
.getName()
.equals("//test:defs.bzl%toolchain_aspect"))
.collect(toImmutableList());
assertThat(nodes).hasSize(1);

AspectKey aspectKey = (AspectKey) Iterables.getOnlyElement(nodes).getKey();
assertThat(aspectKey.getLabel().toString()).isEqualTo("//test:my_out.txt");

ConfiguredAspect aspectValue = (ConfiguredAspect) Iterables.getOnlyElement(nodes).getValue();
assertThat(aspectValue.getProviders().getProviderCount()).isEqualTo(0);
}

@Test
public void toolchainAspectApplyToGeneratingRule_propagateToDeps() throws Exception {
scratch.file(
"test/defs.bzl",
"""
def _impl(target, ctx):
return []
toolchain_aspect = aspect(
implementation = _impl,
toolchains_aspects = ['//rule:toolchain_type_1'],
attr_aspects = ['dep'],
apply_to_generating_rules = True,
)
def _rule_1_impl(ctx):
if ctx.outputs.out:
ctx.actions.write(ctx.outputs.out, 'hi')
return []
r1 = rule(
implementation = _rule_1_impl,
attrs = {
"out": attr.output(),
"dep": attr.label(),
},
toolchains = ['//rule:toolchain_type_1'],
)
""");
scratch.file(
"test/BUILD",
"""
load('//test:defs.bzl', 'r1')
r1(name = 't1', out = 'my_out.txt', dep = ':t2')
r1(name = 't2')
""");
useConfiguration("--extra_toolchains=//toolchain:foo_toolchain");

var unused = update(ImmutableList.of("//test:defs.bzl%toolchain_aspect"), "//test:my_out.txt");

var visitedTargets =
skyframeExecutor.getEvaluator().getDoneValues().entrySet().stream()
.filter(
entry ->
entry.getKey() instanceof AspectKey
&& ((AspectKey) entry.getKey())
.getAspectClass()
.getName()
.equals("//test:defs.bzl%toolchain_aspect"))
.map(e -> ((AspectKey) e.getKey()).getLabel().toString())
.collect(toImmutableList());

// toolchain_aspect is applied to the generating rule of the output file and propagated to its
// attribute dependency and toolchain dependency.
assertThat(visitedTargets)
.containsExactly("//test:my_out.txt", "//test:t1", "//test:t2", "//toolchain:foo");
}

@Test
public void toolchainAspectApplyToFiles_notPropagatedToDeps() throws Exception {
DepsVisitingFileAspect aspect = new DepsVisitingFileAspect("dep", "//rule:toolchain_type_1");
setRulesAndAspectsAvailableInTests(ImmutableList.of(aspect), ImmutableList.of());
scratch.file(
"test/defs.bzl",
"""
def _rule_1_impl(ctx):
if ctx.outputs.out:
ctx.actions.write(ctx.outputs.out, 'hi')
return []
r1 = rule(
implementation = _rule_1_impl,
attrs = {
"out": attr.output(),
"dep": attr.label(),
},
toolchains = ['//rule:toolchain_type_1'],
)
""");
scratch.file(
"test/BUILD",
"""
load('//test:defs.bzl', 'r1')
r1(name = 't1', out = 'my_out.txt', dep = ':t2')
r1(name = 't2')
""");
useConfiguration("--extra_toolchains=//toolchain:foo_toolchain");

var unused = update(ImmutableList.of(aspect.getName()), "//test:my_out.txt");

// {@link DepsVisitingFileAspect} is only applied to //test:my_out.txt file therefore it does
// not propagate to the dependencies of its generating rule.
var nodes =
skyframeExecutor.getEvaluator().getDoneValues().entrySet().stream()
.filter(
entry ->
entry.getKey() instanceof AspectKey
&& ((AspectKey) entry.getKey())
.getAspectClass()
.getName()
.equals(aspect.getName()))
.collect(toImmutableList());
assertThat(nodes).hasSize(1);

AspectKey aspectKey = (AspectKey) Iterables.getOnlyElement(nodes).getKey();
assertThat(aspectKey.getLabel().toString()).isEqualTo("//test:my_out.txt");

ConfiguredAspect aspectValue = (ConfiguredAspect) Iterables.getOnlyElement(nodes).getValue();
StarlarkInfo provider =
(StarlarkInfo) aspectValue.get(DepsVisitingFileAspect.PROVIDER.getKey());
assertThat(provider.getValue("val")).isEqualTo("//test:my_out.txt");
}

private ImmutableList<AspectKey> getAspectKeys(String targetLabel, String aspectLabel) {
return skyframeExecutor.getEvaluator().getDoneValues().entrySet().stream()
.filter(
Expand Down

0 comments on commit 00fdb67

Please sign in to comment.