From f64f071f44394a33a1be40cb7642e2c881d1e9bb 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 2a0f67134185a0..da87d5ffafad67 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 @@ -521,6 +521,7 @@ public StarlarkAspect aspect( StarlarkFunction implementation, Sequence attributeAspects, Object attrs, + Sequence requiredProvidersArg, Sequence requiredAspectProvidersArg, Sequence providesArg, Sequence fragments, @@ -610,6 +611,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 7fe802dee7a949..19fe5566e4664a 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 @@ -283,6 +283,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 6c2efaba5c5fa2..6c26e62133dfaf 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 @@ -36,6 +36,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; @@ -52,6 +53,7 @@ public StarlarkDefinedAspect( StarlarkCallable implementation, ImmutableList attributeAspects, ImmutableList attributes, + ImmutableList> requiredProviders, ImmutableList> requiredAspectProviders, ImmutableSet provides, ImmutableSet paramAttributes, @@ -65,6 +67,7 @@ public StarlarkDefinedAspect( this.implementation = implementation; this.attributeAspects = attributeAspects; this.attributes = attributes; + this.requiredProviders = requiredProviders; this.requiredAspectProviders = requiredAspectProviders; this.provides = provides; this.paramAttributes = paramAttributes; @@ -83,6 +86,7 @@ public StarlarkDefinedAspect( StarlarkCallable implementation, ImmutableList attributeAspects, ImmutableList attributes, + ImmutableList> requiredProviders, ImmutableList> requiredAspectProviders, ImmutableSet provides, ImmutableSet paramAttributes, @@ -98,6 +102,7 @@ public StarlarkDefinedAspect( implementation, attributeAspects, attributes, + requiredProviders, requiredAspectProviders, provides, paramAttributes, @@ -165,7 +170,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()) { @@ -182,6 +187,7 @@ public AspectDefinition getDefinition(AspectParameters aspectParams) { } builder.add(attr); } + builder.requireStarlarkProviderSets(requiredProviders); builder.requireAspectsWithProviders(requiredAspectProviders); ImmutableList.Builder advertisedStarlarkProviders = ImmutableList.builder(); @@ -272,6 +278,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) @@ -289,6 +296,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 621b3e3d39dd7e..b281479fb446da 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 @@ -586,6 +586,21 @@ public class BuildLanguageOptions extends OptionsBase implements Serializable { + " (zero means no limit).") public long maxComputationSteps; + @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 @@ -656,6 +671,9 @@ public StarlarkSemantics toStarlarkSemantics() { INCOMPATIBLE_OBJC_PROVIDER_REMOVE_COMPILE_INFO, incompatibleObjcProviderRemoveCompileInfo) .set(MAX_COMPUTATION_STEPS, maxComputationSteps) + .setBool( + INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS, + incompatibleTopLevelAspectsRequireProviders) .build(); return INTERNER.intern(semantics); } @@ -729,6 +747,8 @@ public StarlarkSemantics toStarlarkSemantics() { "-incompatible_visibility_private_attributes_at_definition"; public static final String RECORD_RULE_INSTANTIATION_CALLSTACK = "+record_rule_instantiation_callstack"; + 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 e9e4bff1739ced..a4cc98a73101b0 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. @@ -324,6 +327,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 8b3dc0f9a1fb15..4eb469892213d1 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 746dd6c88c04aa..1df73f2b49f1c9 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 fb8086a0708f1f..1a1d7eab2f58da 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 @@ -51,9 +51,13 @@ import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; import com.google.devtools.build.lib.skyframe.AspectValueKey.AspectKey; import com.google.devtools.build.lib.vfs.FileSystemUtils; +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 org.junit.Before; +import net.starlark.java.eval.StarlarkList; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -3111,6 +3115,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 502ba79385135f..db3336e5a1d2df 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 @@ -1630,6 +1630,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 70c2b441c9f945..56c72280e6c63d 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"