Skip to content

Commit

Permalink
Remove native required_providers and required_aspect_providers fr…
Browse files Browse the repository at this point in the history 
…om aspects

There were no remaining usages of native `aspect_requires_providers` except in tests.

The remaining uses of `required_providers` list providers that are not actually advertised by rules.

PiperOrigin-RevId: 599928895
Change-Id: Ic7f42ecfc0934b89fea283685b43a632d69fb845
  • Loading branch information
@mai93 @copybara-github
mai93 authored and copybara-github committed Jan 19, 2024
1 parent a3b0aee commit d32ea79
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 207 deletions.
47 changes: 1 addition & 46 deletions src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
import com.google.devtools.build.lib.analysis.config.Fragment;
import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
import com.google.devtools.build.lib.cmdline.Label;
Expand Down Expand Up @@ -277,37 +276,9 @@ public Builder(AspectClass aspectClass) {
this.aspectClass = aspectClass;
}

/**
* Asserts that this aspect can only be evaluated for rules that supply all of the providers
* from at least one set of required providers.
*/
@CanIgnoreReturnValue
public Builder requireProviderSets(
Iterable<ImmutableSet<Class<? extends TransitiveInfoProvider>>> providerSets) {
for (ImmutableSet<Class<? extends TransitiveInfoProvider>> providerSet : providerSets) {
requiredProviders.addBuiltinSet(providerSet);
}
return this;
}

/**
* Asserts that this aspect can only be evaluated for rules that supply all of the specified
* providers.
*/
@CanIgnoreReturnValue
public Builder requireProviders(Class<? extends TransitiveInfoProvider>... providers) {
requiredProviders.addBuiltinSet(ImmutableSet.copyOf(providers));
return this;
}

/**
* Same as the equivalent calls to {@link #requireProviderSets} and {@link
* #requireStarlarkProviderSets} for specifying the required providers conveyed by {@code
* requiredProviders}.
*/
@CanIgnoreReturnValue
public Builder requireProviders(RequiredProviders requiredProviders) {
requiredProviders.addToAspectDefinitionBuilder(this);
this.requireStarlarkProviderSets(requiredProviders.getStarlarkProviders());
return this;
}

Expand Down Expand Up @@ -357,22 +328,6 @@ public Builder requireAspectsWithProviders(
return this;
}

@CanIgnoreReturnValue
public Builder requireAspectsWithBuiltinProviders(
Class<? extends TransitiveInfoProvider>... providers) {
requiredAspectProviders.addBuiltinSet(ImmutableSet.copyOf(providers));
return this;
}

/** State that the aspect being built provides given providers. */
@CanIgnoreReturnValue
public Builder advertiseProvider(Class<?>... providers) {
for (Class<?> provider : providers) {
advertisedProviders.addBuiltin(provider);
}
return this;
}

/** State that the aspect being built provides given providers. */
@CanIgnoreReturnValue
public Builder advertiseProvider(ImmutableList<StarlarkProviderIdentifier> providers) {
Expand Down
12 changes: 0 additions & 12 deletions src/main/java/com/google/devtools/build/lib/packages/RequiredProviders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,18 +342,6 @@ public static Builder acceptAnyBuilder() {
return new Builder(false);
}

/**
* Mutates the given {@link AspectDefinition.Builder}, adding in the required providers specified
* by this {@link RequiredProviders}.
*
* <p>We use this indirection for the sake of encapsulating the internal representation of {@link
* RequiredProviders}.
*/
void addToAspectDefinitionBuilder(AspectDefinition.Builder aspectDefinitionBuilder) {
aspectDefinitionBuilder.requireProviderSets(builtinProviders);
aspectDefinitionBuilder.requireStarlarkProviderSets(starlarkProviders);
}

/**
* A builder for {@link RequiredProviders} that accepts no dependency
* unless restriction provider sets are added.
Expand Down
63 changes: 2 additions & 61 deletions src/test/java/com/google/devtools/build/lib/analysis/AspectDefinitionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,6 @@ public class AspectDefinitionTest {

private static final class P1 implements TransitiveInfoProvider {}

private static final class P2 implements TransitiveInfoProvider {}

private static final class P3 implements TransitiveInfoProvider {}

private static final class P4 implements TransitiveInfoProvider {}

private static final Label FAKE_LABEL = Label.parseCanonicalUnchecked("//fake/label.bzl");

private static final StarlarkProviderIdentifier STARLARK_P1 =
Expand Down Expand Up @@ -165,59 +159,6 @@ public void testAttributeAspect_allAttributes() throws Exception {
assertThat(withAspects.propagateAlong("deps")).isTrue();
}

@Test
public void testRequireBuiltinProviders_addsToSetOfRequiredProvidersAndNames() throws Exception {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireProviders(P1.class, P2.class)
.build();
AdvertisedProviderSet expectedOkSet =
AdvertisedProviderSet.builder()
.addBuiltin(P1.class)
.addBuiltin(P2.class)
.addBuiltin(P3.class)
.build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet))
.isTrue();

AdvertisedProviderSet expectedFailSet =
AdvertisedProviderSet.builder().addBuiltin(P1.class).build();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet))
.isFalse();

assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY))
.isFalse();
}

@Test
public void testRequireBuiltinProviders_addsTwoSetsOfRequiredProvidersAndNames() {
AspectDefinition requiresProviders =
new AspectDefinition.Builder(TEST_ASPECT_CLASS)
.requireProviderSets(
ImmutableList.of(ImmutableSet.of(P1.class, P2.class), ImmutableSet.of(P3.class)))
.build();

AdvertisedProviderSet expectedOkSet1 =
AdvertisedProviderSet.builder().addBuiltin(P1.class).addBuiltin(P2.class).build();

AdvertisedProviderSet expectedOkSet2 =
AdvertisedProviderSet.builder().addBuiltin(P3.class).build();

AdvertisedProviderSet expectedFailSet =
AdvertisedProviderSet.builder().addBuiltin(P4.class).build();

assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet1)).isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet2)).isTrue();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(expectedFailSet)).isFalse();
assertThat(requiresProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.EMPTY))
.isFalse();

}

@Test
public void testRequireStarlarkProviders_addsFlatSetOfRequiredProviders() throws Exception {
AspectDefinition requiresProviders =
Expand Down Expand Up @@ -275,7 +216,7 @@ public void testRequireProviders_defaultAcceptsEverything() {
AspectDefinition noRequiredProviders = new AspectDefinition.Builder(TEST_ASPECT_CLASS).build();

AdvertisedProviderSet expectedOkSet =
AdvertisedProviderSet.builder().addBuiltin(P4.class).addStarlark(STARLARK_P4).build();
AdvertisedProviderSet.builder().addBuiltin(P1.class).addStarlark(STARLARK_P4).build();
assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(expectedOkSet)).isTrue();

assertThat(noRequiredProviders.getRequiredProviders().isSatisfiedBy(AdvertisedProviderSet.ANY))
Expand All @@ -291,7 +232,7 @@ public void testRequireAspectClass_defaultAcceptsNothing() {
.build();

AdvertisedProviderSet expectedFailSet =
AdvertisedProviderSet.builder().addBuiltin(P4.class).build();
AdvertisedProviderSet.builder().addBuiltin(P1.class).build();

assertThat(noAspects.getRequiredProvidersForAspects().isSatisfiedBy(AdvertisedProviderSet.ANY))
.isFalse();
Expand Down
34 changes: 20 additions & 14 deletions src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@
import com.google.devtools.build.lib.packages.Attribute.LateBoundDefault;
import com.google.devtools.build.lib.packages.NativeAspectClass;
import com.google.devtools.build.lib.packages.Provider;
import com.google.devtools.build.lib.packages.StarlarkInfo;
import com.google.devtools.build.lib.packages.StarlarkProvider;
import com.google.devtools.build.lib.packages.StarlarkProviderIdentifier;
import com.google.devtools.build.lib.packages.StructImpl;
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
import com.google.devtools.build.lib.testutil.MoreAsserts;
Expand Down Expand Up @@ -961,7 +963,7 @@ public void aspectWithExtraAttribute_ignoredForOutputFile() throws Exception {

ConfiguredAspect configuredAspect =
Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
assertThat(configuredAspect.getProvider(ExtraAttributeAspect.Provider.class)).isNull();
assertThat(configuredAspect.get(ExtraAttributeAspect.PROVIDER.getKey())).isNull();
}

@Test
Expand All @@ -978,9 +980,9 @@ public void aspectWithExtraAttributeApplyToFiles_outputFile_hasResolvedAttribute

ConfiguredAspect configuredAspect =
Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
ExtraAttributeAspect.Provider provider =
configuredAspect.getProvider(ExtraAttributeAspect.Provider.class);
assertThat(provider.label()).isEqualTo("//extra:extra");
StarlarkInfo provider =
(StarlarkInfo) configuredAspect.get(ExtraAttributeAspect.PROVIDER.getKey());
assertThat(provider.getValue("label")).isEqualTo("//extra:extra");
}

@Test
Expand All @@ -995,7 +997,7 @@ public void aspectWithExtraAttributeApplyToFiles_ignoredForSourceFile() throws E

ConfiguredAspect configuredAspect =
Iterables.getOnlyElement(analysisResult.getAspectsMap().values());
assertThat(configuredAspect.getProvider(ExtraAttributeAspect.Provider.class)).isNull();
assertThat(configuredAspect.get(ExtraAttributeAspect.PROVIDER.getKey())).isNull();
}

@Test
Expand All @@ -1019,10 +1021,11 @@ public void aspectWithExtraAttributeApplyToFilesAndNot_outputFile_onlyApplyToFil
"//a");

assertThat(analysisResult.getAspectsMap()).hasSize(2);
ExtraAttributeAspect.Provider provider =
getAspectByName(analysisResult.getAspectsMap(), aspectApplies.getName())
.getProvider(ExtraAttributeAspect.Provider.class);
assertThat(provider.label()).isEqualTo("//extra:extra");
StarlarkInfo provider =
(StarlarkInfo)
getAspectByName(analysisResult.getAspectsMap(), aspectApplies.getName())
.get(ExtraAttributeAspect.PROVIDER.getKey());
assertThat(provider.getValue("label")).isEqualTo("//extra:extra");
assertThat(
getAspectByName(analysisResult.getAspectsMap(), aspectDoesNotApply.getName())
.getProviders()
Expand All @@ -1035,7 +1038,9 @@ public void aspectWithExtraAttributeDependsOnNotApplicable_usesItsOwnAttribute()
throws Exception {
ExtraAttributeAspect aspectApplies =
new ExtraAttributeAspect(
"//extra", /*applyToFiles=*/ true, ExtraAttributeAspect.Provider.class);
"//extra",
/* applyToFiles= */ true,
StarlarkProviderIdentifier.forKey(ExtraAttributeAspect.PROVIDER.getKey()));
ExtraAttributeAspect aspectDoesNotApply =
new ExtraAttributeAspect("//extra:extra2", /*applyToFiles=*/ false);
setRulesAndAspectsAvailableInTests(
Expand All @@ -1053,10 +1058,11 @@ public void aspectWithExtraAttributeDependsOnNotApplicable_usesItsOwnAttribute()
"//a");

assertThat(analysisResult.getAspectsMap()).hasSize(2);
ExtraAttributeAspect.Provider provider =
getAspectByName(analysisResult.getAspectsMap(), aspectApplies.getName())
.getProvider(ExtraAttributeAspect.Provider.class);
assertThat(provider.label()).isEqualTo("//extra:extra");
StarlarkInfo provider =
(StarlarkInfo)
getAspectByName(analysisResult.getAspectsMap(), aspectApplies.getName())
.get(ExtraAttributeAspect.PROVIDER.getKey());
assertThat(provider.getValue("label")).isEqualTo("//extra:extra");
assertThat(
getAspectByName(analysisResult.getAspectsMap(), aspectDoesNotApply.getName())
.getProviders()
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/analysis/util/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ java_library(
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//src/main/java/com/google/devtools/common/options",
"//src/main/java/net/starlark/java/eval",
"//src/main/java/net/starlark/java/syntax",
"//src/test/java/com/google/devtools/build/lib/actions/util",
"//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util",
"//src/test/java/com/google/devtools/build/lib/packages:testutil",
Expand Down
Loading

0 comments on commit d32ea79

Please sign in to comment.