From 1bca1bde7db06340559878a7d54b1f5698e7ddd6 Mon Sep 17 00:00:00 2001 From: messa Date: Fri, 14 May 2021 00:48:19 -0700 Subject: [PATCH] Add `required_providers` attribute to Starlark defined aspects. `required_providers` attribute allows the aspect to limit its propagation to only the targets whose rules advertise the required providers. It accepts a list of either providers or providers lists. To make some rule targets visible to an aspect, the rule must advertise all providers from at least one of the lists specified in the aspect `required_providers`. This CL also adds incompatible flag `incompatible_top_level_aspects_require_providers` which when set allows the top level aspects to only run on top level targets that advertise its required providers. It is needed to avoid breaking existing usages on command line aspects on targets not advertising its required providers. PiperOrigin-RevId: 373738497 --- .../starlark/StarlarkRuleClassFunctions.java | 2 + .../build/lib/packages/AspectDefinition.java | 14 + .../lib/packages/StarlarkDefinedAspect.java | 10 +- .../semantics/BuildLanguageOptions.java | 20 + .../build/lib/skyframe/AspectFunction.java | 31 ++ .../StarlarkRuleFunctionsApi.java | 26 ++ .../FakeStarlarkRuleFunctionsApi.java | 1 + .../lib/analysis/AspectDefinitionTest.java | 88 ++++- .../starlark/StarlarkDefinedAspectsTest.java | 334 +++++++++++++++++ .../StarlarkRuleClassFunctionsTest.java | 85 +++++ src/test/shell/integration/aspect_test.sh | 352 ++++++++++++++++++ 11 files changed, 960 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index a905d015e1ea0d..29c4febdb671ea 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -518,6 +518,7 @@ public StarlarkAspect aspect( StarlarkFunction implementation, Sequence attributeAspects, Object attrs, + Sequence requiredProvidersArg, Sequence requiredAspectProvidersArg, Sequence providesArg, Sequence fragments, @@ -607,6 +608,7 @@ public StarlarkAspect aspect( implementation, attrAspects.build(), attributes.build(), + StarlarkAttrModule.buildProviderPredicate(requiredProvidersArg, "required_providers"), StarlarkAttrModule.buildProviderPredicate( requiredAspectProvidersArg, "required_aspect_providers"), StarlarkAttrModule.getStarlarkProviderIdentifiers(providesArg), diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java index 9c50c80d7631a7..30ec83cb11c544 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java @@ -305,6 +305,20 @@ public Builder requireProviders(Class... provi return this; } + /** + * Asserts that this aspect can only be evaluated for rules that supply all of the providers + * from at least one set of required providers. + */ + public Builder requireStarlarkProviderSets( + Iterable> providerSets) { + for (ImmutableSet providerSet : providerSets) { + if (!providerSet.isEmpty()) { + requiredProviders.addStarlarkSet(providerSet); + } + } + return this; + } + /** * Asserts that this aspect can only be evaluated for rules that supply all of the specified * Starlark providers. diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java index 3991894e154c05..959d6ec967bf55 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkDefinedAspect.java @@ -37,6 +37,7 @@ public class StarlarkDefinedAspect implements StarlarkExportable, StarlarkAspect private final StarlarkCallable implementation; private final ImmutableList attributeAspects; private final ImmutableList attributes; + private final ImmutableList> requiredProviders; private final ImmutableList> requiredAspectProviders; private final ImmutableSet provides; private final ImmutableSet paramAttributes; @@ -53,6 +54,7 @@ public StarlarkDefinedAspect( StarlarkCallable implementation, ImmutableList attributeAspects, ImmutableList attributes, + ImmutableList> requiredProviders, ImmutableList> requiredAspectProviders, ImmutableSet provides, ImmutableSet paramAttributes, @@ -66,6 +68,7 @@ public StarlarkDefinedAspect( this.implementation = implementation; this.attributeAspects = attributeAspects; this.attributes = attributes; + this.requiredProviders = requiredProviders; this.requiredAspectProviders = requiredAspectProviders; this.provides = provides; this.paramAttributes = paramAttributes; @@ -84,6 +87,7 @@ public StarlarkDefinedAspect( StarlarkCallable implementation, ImmutableList attributeAspects, ImmutableList attributes, + ImmutableList> requiredProviders, ImmutableList> requiredAspectProviders, ImmutableSet provides, ImmutableSet paramAttributes, @@ -99,6 +103,7 @@ public StarlarkDefinedAspect( implementation, attributeAspects, attributes, + requiredProviders, requiredAspectProviders, provides, paramAttributes, @@ -166,7 +171,7 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) { builder.propagateAlongAttribute(attributeAspect); } } - + for (Attribute attribute : attributes) { Attribute attr = attribute; // Might be reassigned. if (!aspectParams.getAttribute(attr.getName()).isEmpty()) { @@ -183,6 +188,7 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) { } builder.add(attr); } + builder.requireStarlarkProviderSets(requiredProviders); builder.requireAspectsWithProviders(requiredAspectProviders); ImmutableList.Builder advertisedStarlarkProviders = ImmutableList.builder(); @@ -273,6 +279,7 @@ public boolean equals(Object o) { return Objects.equals(implementation, that.implementation) && Objects.equals(attributeAspects, that.attributeAspects) && Objects.equals(attributes, that.attributes) + && Objects.equals(requiredProviders, that.requiredProviders) && Objects.equals(requiredAspectProviders, that.requiredAspectProviders) && Objects.equals(provides, that.provides) && Objects.equals(paramAttributes, that.paramAttributes) @@ -290,6 +297,7 @@ public int hashCode() { implementation, attributeAspects, attributes, + requiredProviders, requiredAspectProviders, provides, paramAttributes, diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index d42e7fff2117a4..d3b47f4cd0591c 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -627,6 +627,21 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable { + " environment and inputs during execution.") public boolean experimentalShadowedAction; + @Option( + name = "incompatible_top_level_aspects_require_providers", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "If set to true, the top level aspect will honor its required providers and only run on" + + " top level targets whose rules' advertised providers satisfy the required" + + " providers of the aspect.") + public boolean incompatibleTopLevelAspectsRequireProviders; + /** * An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should * never accumulate a large number of these and being able to shortcut on object identity makes a @@ -694,6 +709,9 @@ public StarlarkSemantics toStarlarkSemantics() { .set(MAX_COMPUTATION_STEPS, maxComputationSteps) .set(NESTED_SET_DEPTH_LIMIT, nestedSetDepthLimit) .setBool(EXPERIMENTAL_SHADOWED_ACTION, experimentalShadowedAction) + .setBool( + INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS, + incompatibleTopLevelAspectsRequireProviders) .build(); return INTERNER.intern(semantics); } @@ -765,6 +783,8 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION = "-incompatible_visibility_private_attributes_at_definition"; public static final String EXPERIMENTAL_SHADOWED_ACTION = "-experimental_shadowed_action"; + public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS = + "-incompatible_top_level_aspects_require_providers"; // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = 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 44ab8a796da506..aead10e5076850 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 @@ -58,12 +58,14 @@ import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.OutputFile; import com.google.devtools.build.lib.packages.Package; +import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.StarlarkAspect; import com.google.devtools.build.lib.packages.StarlarkAspectClass; import com.google.devtools.build.lib.packages.StarlarkDefinedAspect; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.Type.ConversionException; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.profiler.memory.CurrentRuleTracker; import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey; import com.google.devtools.build.lib.skyframe.BzlLoadFunction.BzlLoadFailedException; @@ -78,6 +80,7 @@ import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; +import net.starlark.java.eval.StarlarkSemantics; /** * The Skyframe function that generates aspects. @@ -314,6 +317,34 @@ public SkyValue compute(SkyKey skyKey, Environment env) baseConfiguredTargetValue.getConfiguredTarget()); } + // If the incompatible flag is set, the top-level aspect should not be applied on top-level + // targets whose rules do not advertise the aspect's required providers. The aspect should not + // also propagate to these targets dependencies. + StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); + if (starlarkSemantics == null) { + return null; + } + boolean checkRuleAdvertisedProviders = + starlarkSemantics.getBool( + BuildLanguageOptions.INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS); + if (checkRuleAdvertisedProviders) { + Target target = associatedConfiguredTargetAndData.getTarget(); + if (target instanceof Rule) { + if (!aspect + .getDefinition() + .getRequiredProviders() + .isSatisfiedBy(((Rule) target).getRuleClassObject().getAdvertisedProviders())) { + return new AspectValue( + key, + aspect, + target.getLocation(), + ConfiguredAspect.forNonapplicableTarget(), + /*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder + .stableOrder() + .build()); + } + } + } ImmutableList.Builder aspectPathBuilder = ImmutableList.builder(); diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java index b268a76782b5dc..2a82a6ee45632b 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkRuleFunctionsApi.java @@ -416,6 +416,31 @@ StarlarkCallable rule( + "the values restriction. Explicit attributes restrict the " + "aspect to only be used with rules that have attributes of the same " + "name, type, and valid values according to the restriction."), + @Param( + name = "required_providers", + named = true, + defaultValue = "[]", + doc = + "This attribute allows the aspect to limit its propagation to only the targets " + + "whose rules advertise its required providers. The value must be a " + + "list containing either individual providers or lists of providers but not " + + "both. For example, [[FooInfo], [BarInfo], [BazInfo, QuxInfo]] " + + "is a valid value while [FooInfo, BarInfo, [BazInfo, QuxInfo]] " + + "is not valid." + + "" + + "

An unnested list of providers will automatically be converted to a list " + + "containing one list of providers. That is, [FooInfo, BarInfo] " + + "will automatically be converted to [[FooInfo, BarInfo]]." + + "" + + "

To make some rule (e.g. some_rule) targets visible to an " + + "aspect, some_rule must advertise all providers from at least " + + "one of the required providers lists. For example, if the " + + "required_providers of an aspect are " + + "[[FooInfo], [BarInfo], [BazInfo, QuxInfo]], this aspect can " + + "only see some_rule targets if and only if " + + "some_rule provides FooInfo *or* " + + "BarInfo *or* both BazInfo *and* " + + "QuxInfo."), @Param( name = "required_aspect_providers", named = true, @@ -500,6 +525,7 @@ StarlarkAspectApi aspect( StarlarkFunction implementation, Sequence attributeAspects, Object attrs, + Sequence requiredProvidersArg, Sequence requiredAspectProvidersArg, Sequence providesArg, Sequence fragments, diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java index daad2b721f9499..9b163e2455da4e 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeStarlarkRuleFunctionsApi.java @@ -180,6 +180,7 @@ public StarlarkAspectApi aspect( StarlarkFunction implementation, Sequence attributeAspects, Object attrs, + Sequence requiredProvidersArg, Sequence requiredAspectProvidersArg, Sequence providesArg, Sequence fragments, diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java index 2711dfc1c8b102..741da4564579d3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java @@ -33,6 +33,8 @@ import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy; import com.google.devtools.build.lib.packages.NativeAspectClass; +import com.google.devtools.build.lib.packages.StarlarkProvider; +import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.util.FileTypeSet; import net.starlark.java.annot.StarlarkBuiltin; @@ -55,6 +57,20 @@ private static final class P3 implements TransitiveInfoProvider {} private static final class P4 implements TransitiveInfoProvider {} + private static final Label FAKE_LABEL = Label.parseAbsoluteUnchecked("//fake/label.bzl"); + + private static final StarlarkProviderIdentifier STARLARK_P1 = + StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P1")); + + private static final StarlarkProviderIdentifier STARLARK_P2 = + StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P2")); + + private static final StarlarkProviderIdentifier STARLARK_P3 = + StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P3")); + + private static final StarlarkProviderIdentifier STARLARK_P4 = + StarlarkProviderIdentifier.forKey(new StarlarkProvider.Key(FAKE_LABEL, "STARLARK_P4")); + /** * A dummy aspect factory. Is there to demonstrate how to define aspects and so that we can test * {@code attributeAspect}. @@ -150,7 +166,7 @@ public void testAttributeAspect_allAttributes() throws Exception { } @Test - public void testRequireProvider_addsToSetOfRequiredProvidersAndNames() throws Exception { + public void testRequireBuiltinProviders_addsToSetOfRequiredProvidersAndNames() throws Exception { AspectDefinition requiresProviders = new AspectDefinition.Builder(TEST_ASPECT_CLASS) .requireProviders(P1.class, P2.class) @@ -176,7 +192,8 @@ public void testRequireProvider_addsToSetOfRequiredProvidersAndNames() throws Ex } @Test - public void testRequireProvider_addsTwoSetsOfRequiredProvidersAndNames() throws Exception { + public void testRequireBuiltinProviders_addsTwoSetsOfRequiredProvidersAndNames() + throws Exception { AspectDefinition requiresProviders = new AspectDefinition.Builder(TEST_ASPECT_CLASS) .requireProviderSets( @@ -202,6 +219,73 @@ public void testRequireProvider_addsTwoSetsOfRequiredProvidersAndNames() throws } + @Test + public void testRequireStarlarkProviders_addsFlatSetOfRequiredProviders() throws Exception { + AspectDefinition requiresProviders = + new AspectDefinition.Builder(TEST_ASPECT_CLASS) + .requireStarlarkProviders(STARLARK_P1, STARLARK_P2) + .build(); + + AdvertisedProviderSet expectedOkSet = + AdvertisedProviderSet.builder() + .addStarlark(STARLARK_P1) + .addStarlark(STARLARK_P2) + .addStarlark(STARLARK_P3) + .build(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet)).isTrue(); + + AdvertisedProviderSet expectedFailSet = + AdvertisedProviderSet.builder().addStarlark(STARLARK_P1).build(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet)).isFalse(); + + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY)) + .isTrue(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY)) + .isFalse(); + } + + @Test + public void testRequireStarlarkProviders_addsTwoSetsOfRequiredProviders() throws Exception { + AspectDefinition requiresProviders = + new AspectDefinition.Builder(TEST_ASPECT_CLASS) + .requireStarlarkProviderSets( + ImmutableList.of( + ImmutableSet.of(STARLARK_P1, STARLARK_P2), ImmutableSet.of(STARLARK_P3))) + .build(); + + AdvertisedProviderSet expectedOkSet1 = + AdvertisedProviderSet.builder().addStarlark(STARLARK_P1).addStarlark(STARLARK_P2).build(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet1)).isTrue(); + + AdvertisedProviderSet expectedOkSet2 = + AdvertisedProviderSet.builder().addStarlark(STARLARK_P3).build(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet2)).isTrue(); + + AdvertisedProviderSet expectedFailSet = + AdvertisedProviderSet.builder().addStarlark(STARLARK_P4).build(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet)).isFalse(); + + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY)) + .isTrue(); + assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY)) + .isFalse(); + } + + @Test + public void testRequireProviders_defaultAcceptsEverything() { + AspectDefinition noRequiredProviders = new AspectDefinition.Builder(TEST_ASPECT_CLASS).build(); + + AdvertisedProviderSet expectedOkSet = + AdvertisedProviderSet.builder().addBuiltin(P4.class).addStarlark(STARLARK_P4).build(); + assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet)).isTrue(); + + assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY)) + .isTrue(); + assertThat( + noRequiredProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY)) + .isTrue(); + } + @Test public void testRequireAspectClass_defaultAcceptsNothing() { AspectDefinition noAspects = new AspectDefinition.Builder(TEST_ASPECT_CLASS) diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java index 92800aebec496a..326e56d1ce30dd 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkDefinedAspectsTest.java @@ -50,8 +50,12 @@ import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; +import java.util.ArrayList; +import java.util.List; import net.starlark.java.eval.Sequence; +import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkInt; +import net.starlark.java.eval.StarlarkList; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -3086,6 +3090,336 @@ public void testAspectActionsDontInheritExecProperties() throws Exception { assertThat(owner.getExecProperties()).isEmpty(); } + @Test + public void testAspectRequiredProviders_defaultNoRequiredProviders() throws Exception { + scratch.file( + "test/defs.bzl", + "prov_a = provider()", + "prov_b = provider()", + "", + "def _my_aspect_impl(target, ctx):", + " targets_labels = [\"//test:defs.bzl%my_aspect({})\".format(target.label)]", + " for dep in ctx.rule.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " targets_labels.extend(dep.target_labels)", + " return struct(target_labels = targets_labels)", + "", + "my_aspect = aspect(", + " implementation = _my_aspect_impl,", + " attr_aspects = ['deps'],", + ")", + "", + "def _rule_without_providers_impl(ctx):", + " s = []", + " for dep in ctx.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " s.extend(dep.target_labels)", + " return struct(rule_deps = s)", + "", + "rule_without_providers = rule(", + " implementation = _rule_without_providers_impl,", + " attrs = {", + " 'deps': attr.label_list(aspects = [my_aspect])", + " },", + ")", + "", + "def _rule_with_providers_impl(ctx):", + " return [prov_a(), prov_b()]", + "", + "rule_with_providers = rule(", + " implementation = _rule_with_providers_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_a, prov_b]", + ")", + "", + "rule_with_providers_not_advertised = rule(", + " implementation = _rule_with_providers_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = []", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'rule_with_providers', 'rule_without_providers',", + " 'rule_with_providers_not_advertised')", + "rule_without_providers(", + " name = 'main',", + " deps = [':target_without_providers', ':target_with_providers',", + " ':target_with_providers_not_advertised'],", + ")", + "rule_without_providers(", + " name = 'target_without_providers',", + ")", + "rule_with_providers(", + " name = 'target_with_providers',", + ")", + "rule_with_providers(", + " name = 'target_with_providers_indeps',", + ")", + "rule_with_providers_not_advertised(", + " name = 'target_with_providers_not_advertised',", + " deps = [':target_with_providers_indeps'],", + ")"); + + AnalysisResult analysisResult = update("//test:main"); + + // my_aspect does not require any providers so it will be applied to all the dependencies of + // main target + List expected = new ArrayList<>(); + expected.add("//test:defs.bzl%my_aspect(//test:target_without_providers)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_providers)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_providers_not_advertised)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_providers_indeps)"); + assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:main"); + ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next(); + Object ruleDepsUnchecked = target.get("rule_deps"); + assertThat(ruleDepsUnchecked).isInstanceOf(StarlarkList.class); + StarlarkList ruleDeps = (StarlarkList) ruleDepsUnchecked; + assertThat(Starlark.toIterable(ruleDeps)).containsExactlyElementsIn(expected); + } + + @Test + public void testAspectRequiredProviders_flatSetOfRequiredProviders() throws Exception { + scratch.file( + "test/defs.bzl", + "prov_a = provider()", + "prov_b = provider()", + "", + "def _my_aspect_impl(target, ctx):", + " targets_labels = [\"//test:defs.bzl%my_aspect({})\".format(target.label)]", + " for dep in ctx.rule.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " targets_labels.extend(dep.target_labels)", + " return struct(target_labels = targets_labels)", + "", + "my_aspect = aspect(", + " implementation = _my_aspect_impl,", + " attr_aspects = ['deps'],", + " required_providers = [prov_a, prov_b],", + ")", + "", + "def _rule_without_providers_impl(ctx):", + " s = []", + " for dep in ctx.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " s.extend(dep.target_labels)", + " return struct(rule_deps = s)", + "", + "rule_without_providers = rule(", + " implementation = _rule_without_providers_impl,", + " attrs = {", + " 'deps': attr.label_list(aspects=[my_aspect])", + " },", + " provides = []", + ")", + "", + "def _rule_with_a_impl(ctx):", + " return [prov_a()]", + "", + "rule_with_a = rule(", + " implementation = _rule_with_a_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_a]", + ")", + "", + "def _rule_with_ab_impl(ctx):", + " return [prov_a(), prov_b()]", + "", + "rule_with_ab = rule(", + " implementation = _rule_with_ab_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_a, prov_b]", + ")", + "", + "rule_with_ab_not_advertised = rule(", + " implementation = _rule_with_ab_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = []", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'rule_without_providers', 'rule_with_a', 'rule_with_ab',", + " 'rule_with_ab_not_advertised')", + "rule_without_providers(", + " name = 'main',", + " deps = [':target_without_providers', ':target_with_a', ':target_with_ab',", + " ':target_with_ab_not_advertised'],", + ")", + "rule_without_providers(", + " name = 'target_without_providers',", + ")", + "rule_with_a(", + " name = 'target_with_a',", + " deps = [':target_with_ab_indeps_not_reached']", + ")", + "rule_with_ab(", + " name = 'target_with_ab',", + " deps = [':target_with_ab_indeps_reached']", + ")", + "rule_with_ab(", + " name = 'target_with_ab_indeps_not_reached',", + ")", + "rule_with_ab(", + " name = 'target_with_ab_indeps_reached',", + ")", + "rule_with_ab_not_advertised(", + " name = 'target_with_ab_not_advertised',", + ")"); + + AnalysisResult analysisResult = update("//test:main"); + + // my_aspect will only be applied on target_with_ab and target_with_ab_indeps_reached since + // their rule (rule_with_ab) is the only rule that advertises the aspect required providers. + // However, my_aspect cannot be propagated to target_with_ab_indeps_not_reached because it was + // not applied to its parent (target_with_a) + List expected = new ArrayList<>(); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_ab)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_ab_indeps_reached)"); + assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:main"); + ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next(); + Object ruleDepsUnchecked = target.get("rule_deps"); + assertThat(ruleDepsUnchecked).isInstanceOf(StarlarkList.class); + StarlarkList ruleDeps = (StarlarkList) ruleDepsUnchecked; + assertThat(Starlark.toIterable(ruleDeps)).containsExactlyElementsIn(expected); + } + + @Test + public void testAspectRequiredProviders_listOfRequiredProvidersLists() throws Exception { + scratch.file( + "test/defs.bzl", + "prov_a = provider()", + "prov_b = provider()", + "prov_c = provider()", + "", + "def _my_aspect_impl(target, ctx):", + " targets_labels = [\"//test:defs.bzl%my_aspect({})\".format(target.label)]", + " for dep in ctx.rule.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " targets_labels.extend(dep.target_labels)", + " return struct(target_labels = targets_labels)", + "", + "my_aspect = aspect(", + " implementation = _my_aspect_impl,", + " attr_aspects = ['deps'],", + " required_providers = [[prov_a, prov_b], [prov_c]],", + ")", + "", + "def _rule_without_providers_impl(ctx):", + " s = []", + " for dep in ctx.attr.deps:", + " if hasattr(dep, 'target_labels'):", + " s.extend(dep.target_labels)", + " return struct(rule_deps = s)", + "", + "rule_without_providers = rule(", + " implementation = _rule_without_providers_impl,", + " attrs = {", + " 'deps': attr.label_list(aspects=[my_aspect])", + " },", + " provides = []", + ")", + "", + "def _rule_with_a_impl(ctx):", + " return [prov_a()]", + "", + "rule_with_a = rule(", + " implementation = _rule_with_a_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_a]", + ")", + "", + "def _rule_with_c_impl(ctx):", + " return [prov_c()]", + "", + "rule_with_c = rule(", + " implementation = _rule_with_c_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_c]", + ")", + "", + "def _rule_with_ab_impl(ctx):", + " return [prov_a(), prov_b()]", + "", + "rule_with_ab = rule(", + " implementation = _rule_with_ab_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = [prov_a, prov_b]", + ")", + "", + "rule_with_ab_not_advertised = rule(", + " implementation = _rule_with_ab_impl,", + " attrs = {", + " 'deps': attr.label_list()", + " },", + " provides = []", + ")"); + scratch.file( + "test/BUILD", + "load('//test:defs.bzl', 'rule_without_providers', 'rule_with_a', 'rule_with_c',", + " 'rule_with_ab', 'rule_with_ab_not_advertised')", + "rule_without_providers(", + " name = 'main',", + " deps = [':target_without_providers', ':target_with_a', ':target_with_c',", + " ':target_with_ab', 'target_with_ab_not_advertised'],", + ")", + "rule_without_providers(", + " name = 'target_without_providers',", + ")", + "rule_with_a(", + " name = 'target_with_a',", + " deps = [':target_with_c_indeps_not_reached'],", + ")", + "rule_with_c(", + " name = 'target_with_c',", + ")", + "rule_with_c(", + " name = 'target_with_c_indeps_reached',", + ")", + "rule_with_c(", + " name = 'target_with_c_indeps_not_reached',", + ")", + "rule_with_ab(", + " name = 'target_with_ab',", + " deps = [':target_with_c_indeps_reached'],", + ")", + "rule_with_ab_not_advertised(", + " name = 'target_with_ab_not_advertised',", + ")"); + + AnalysisResult analysisResult = update("//test:main"); + + // my_aspect will only be applied on target_with_ab, target_wtih_c and + // target_with_c_indeps_reached because their rules (rule_with_ab and rule_with_c) are the only + // rules advertising the aspect required providers + // However, my_aspect cannot be propagated to target_with_c_indeps_not_reached because it was + // not applied to its parent (target_with_a) + List expected = new ArrayList<>(); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_ab)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_c)"); + expected.add("//test:defs.bzl%my_aspect(//test:target_with_c_indeps_reached)"); + assertThat(getLabelsToBuild(analysisResult)).containsExactly("//test:main"); + ConfiguredTarget target = analysisResult.getTargetsToBuild().iterator().next(); + Object ruleDepsUnchecked = target.get("rule_deps"); + assertThat(ruleDepsUnchecked).isInstanceOf(StarlarkList.class); + StarlarkList ruleDeps = (StarlarkList) ruleDepsUnchecked; + assertThat(Starlark.toIterable(ruleDeps)).containsExactlyElementsIn(expected); + } + /** StarlarkAspectTest with "keep going" flag */ @RunWith(JUnit4.class) public static final class WithKeepGoing extends StarlarkDefinedAspectsTest { diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java index 826c37d50e9d20..b43b362558b2f8 100644 --- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleClassFunctionsTest.java @@ -1632,6 +1632,91 @@ public void aspectRequiredAspectProvidersDefault() throws Exception { assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isFalse(); } + @Test + public void aspectRequiredProvidersSingle() throws Exception { + evalAndExport( + ev, + "def _impl(target, ctx):", + " pass", + "cc = provider()", + "my_aspect = aspect(_impl, required_providers=['java', cc])"); + StarlarkDefinedAspect myAspect = (StarlarkDefinedAspect) ev.lookup("my_aspect"); + RequiredProviders requiredProviders = + myAspect.getDefinition(AspectParameters.EMPTY).getRequiredProviders(); + + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.ANY)).isTrue(); + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isFalse(); + assertThat( + requiredProviders.isSatisfiedBy( + AdvertisedProviderSet.builder() + .addStarlark(declared("cc")) + .addStarlark("java") + .build())) + .isTrue(); + assertThat( + requiredProviders.isSatisfiedBy( + AdvertisedProviderSet.builder().addStarlark(declared("cc")).build())) + .isFalse(); + } + + @Test + public void aspectRequiredProvidersAlternatives() throws Exception { + evalAndExport( + ev, + "def _impl(target, ctx):", + " pass", + "cc = provider()", + "my_aspect = aspect(_impl, required_providers=[['java'], [cc]])"); + StarlarkDefinedAspect myAspect = (StarlarkDefinedAspect) ev.lookup("my_aspect"); + RequiredProviders requiredProviders = + myAspect.getDefinition(AspectParameters.EMPTY).getRequiredProviders(); + + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.ANY)).isTrue(); + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isFalse(); + assertThat( + requiredProviders.isSatisfiedBy( + AdvertisedProviderSet.builder().addStarlark("java").build())) + .isTrue(); + assertThat( + requiredProviders.isSatisfiedBy( + AdvertisedProviderSet.builder().addStarlark(declared("cc")).build())) + .isTrue(); + assertThat( + requiredProviders.isSatisfiedBy( + AdvertisedProviderSet.builder().addStarlark("prolog").build())) + .isFalse(); + } + + @Test + public void aspectRequiredProvidersEmpty() throws Exception { + evalAndExport( + ev, + "def _impl(target, ctx):", + " pass", + "my_aspect = aspect(_impl, required_providers=[])"); + StarlarkDefinedAspect myAspect = (StarlarkDefinedAspect) ev.lookup("my_aspect"); + RequiredProviders requiredProviders = + myAspect.getDefinition(AspectParameters.EMPTY).getRequiredProviders(); + + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.ANY)).isTrue(); + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isTrue(); + } + + @Test + public void aspectRequiredProvidersDefault() throws Exception { + evalAndExport( + ev, + "def _impl(target, ctx):", // + " pass", + "my_aspect = aspect(_impl)"); + StarlarkDefinedAspect myAspect = (StarlarkDefinedAspect) ev.lookup("my_aspect"); + RequiredProviders requiredProviders = + myAspect.getDefinition(AspectParameters.EMPTY).getRequiredProviders(); + + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.ANY)).isTrue(); + assertThat(requiredProviders.isSatisfiedBy(AdvertisedProviderSet.EMPTY)).isTrue(); + } + @Test public void aspectProvides() throws Exception { evalAndExport( diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh index 0d960f7b94c9bf..63f0c978e88165 100755 --- a/src/test/shell/integration/aspect_test.sh +++ b/src/test/shell/integration/aspect_test.sh @@ -89,4 +89,356 @@ EOF expect_not_log "IllegalStateException" } +function test_aspect_required_providers_with_toplevel_aspects() { + local package="a" + mkdir -p "${package}" + + cat > "${package}/lib.bzl" < "${package}/BUILD" <"$TEST_log" \ + || fail "Build failed but should have succeeded" + + # Only aspect_a is applied on target_with_a because its "provided" providers + # do not macth aspect_b required providers. + expect_log "aspect_a runs on target //${package}:target_with_a" + expect_not_log "aspect_b runs on target //${package}:target_with_a" + + # Only aspect_a can run on target_with_a_indeps + expect_log "aspect_a runs on target //${package}:target_with_a_indeps" + expect_not_log "aspect_b runs on target //${package}:target_with_a_indeps" + + # Only aspect_b can run on target_with_bc + expect_not_log "aspect_a runs on target //${package}:target_with_bc" + expect_log "aspect_b runs on target //${package}:target_with_bc" + + # using --incompatible_top_level_aspects_require_providers, the top level + # target rule's advertised providers will be checked and only aspect_a will be + # applied on target_with_a and propgated to its dependencies. + bazel build "${package}:target_with_a" \ + --aspects="//${package}:lib.bzl%aspect_a" \ + --aspects="//${package}:lib.bzl%aspect_b" &>"$TEST_log" \ + --incompatible_top_level_aspects_require_providers \ + || fail "Build failed but should have succeeded" + + # Only aspect_a is applied on target_with_a + expect_log "aspect_a runs on target //${package}:target_with_a" + expect_not_log "aspect_b runs on target //${package}:target_with_a" + + # Only aspect_a can run on target_with_a_indeps + expect_log "aspect_a runs on target //${package}:target_with_a_indeps" + expect_not_log "aspect_b runs on target //${package}:target_with_a_indeps" + + # rule_with_bc advertised provides only match the required providers for + # aspect_b, but aspect_b is not propagated from target_with_a + expect_not_log "aspect_a runs on target //${package}:target_with_bc" + expect_not_log "aspect_b runs on target //${package}:target_with_bc" +} + +function test_aspect_required_providers_default_no_required_providers() { + local package="a" + mkdir -p "${package}" + + cat > "${package}/lib.bzl" < "${package}/BUILD" <"$TEST_log" \ + || fail "Build failed but should have succeeded" + + # my_aspect does not require any providers so it will be applied to all the + # dependencies of main target + expect_log "my_aspect runs on target //${package}:target_without_providers" + expect_log "my_aspect runs on target //${package}:target_with_providers" + expect_log "my_aspect runs on target //${package}:target_with_providers_not_advertised" + expect_log "my_aspect runs on target //${package}:target_with_providers_indeps" +} + +function test_aspect_required_providers_flat_set_of_required_providers() { + local package="a" + mkdir -p "${package}" + + cat > "${package}/lib.bzl" < "${package}/BUILD" <"$TEST_log" \ + || fail "Build failed but should have succeeded" + + # my_aspect will only be applied on target_with_ab and + # target_with_ab_indeps_reached since their rule (rule_with_ab) is the only + # rule that advertises the aspect required providers. + expect_log "my_aspect runs on target //${package}:target_with_ab" + expect_log "my_aspect runs on target //${package}:target_with_ab_indeps_reached" + expect_not_log "/^my_aspect runs on target //${package}:target_with_a$/" + expect_not_log "my_aspect runs on target //${package}:target_without_providers" + expect_not_log "my_aspect runs on target //${package}:target_with_ab_not_advertised" + + # my_aspect cannot be propagated to target_with_ab_indeps_not_reached + # because it was not applied to its parent (target_with_a) + expect_not_log "my_aspect runs on target //${package}:target_with_ab_indeps_not_reached" +} + +function test_aspect_required_providers_with_list_of_required_providers_lists() { + local package="a" + mkdir -p "${package}" + + cat > "${package}/lib.bzl" < "${package}/BUILD" <"$TEST_log" \ + || fail "Build failed but should have succeeded" + + # my_aspect will only be applied on target_with_ab, target_wtih_c and + # target_with_c_indeps_reached because their rules (rule_with_ab and + # rule_with_c) are the only rules advertising the aspect required providers + expect_log "my_aspect runs on target //${package}:target_with_ab" + expect_log "my_aspect runs on target //${package}:target_with_c" + expect_log "my_aspect runs on target //${package}:target_with_c_indeps_reached" + expect_not_log "my_aspect runs on target //${package}:target_without_providers" + expect_not_log "/^my_aspect runs on target //${package}:target_with_a$/" + expect_not_log "my_aspect runs on target //${package}:target_with_ab_not_advertised" + + # my_aspect cannot be propagated to target_with_c_indeps_not_reached because it was + # not applied to its parent (target_with_a) + expect_not_log "my_aspect runs on target //${package}:target_with_c_indeps_not_reached" +} + run_suite "Tests for aspects"