Remove incompatible_top_level_aspects_dependency flag PiperOrigin-RevId: 403924623
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java index db5a0fb..816c30a 100644 --- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java +++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildRequestOptions.java
@@ -305,15 +305,13 @@ effectTags = {OptionEffectTag.UNKNOWN}, allowMultiple = true, help = - "Comma-separated list of aspects to be applied to top-level targets. All aspects are" - + " applied independently to all top-level targets except if" - + " <code>incompatible_top_level_aspects_dependency</code> is used. In this case, if" + "Comma-separated list of aspects to be applied to top-level targets. In the list, if" + " aspect <code>some_aspect</code> specifies required aspect providers via" + " <code>required_aspect_providers</code>, <code>some_aspect</code> will run after" + " every aspect that was mentioned before it in the aspects list whose advertised" + " providers satisfy <code>some_aspect</code> required aspect providers. Moreover," + " <code>some_aspect</code> will run after all its required aspects specified by" - + " <code>requires</code> attribute which otherwise will be ignored." + + " <code>requires</code> attribute." + " <code>some_aspect</code> will then have access to the values of those aspects'" + " providers." + " <bzl-file-label>%<aspect_name>, for example '//tools:my_def.bzl%my_aspect', where"
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 8ac702a..b599d0e 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
@@ -557,19 +557,6 @@ + " providers of the aspect.") public boolean incompatibleTopLevelAspectsRequireProviders; - @Option( - name = "incompatible_top_level_aspects_dependency", - defaultValue = "true", - documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, - metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, - effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, - help = - "If set to true, a dependency between the top level aspects will be built based on their" - + " required aspect providers, advertised providers and required aspects. Otherwise," - + " each aspect in the list will run independently and its required aspects will be" - + " ignored.") - public boolean incompatibleTopLevelAspectsDependOnAspects; - /** * 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 @@ -639,9 +626,6 @@ .setBool( INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS, incompatibleTopLevelAspectsRequireProviders) - .setBool( - INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY, - incompatibleTopLevelAspectsDependOnAspects) .build(); return INTERNER.intern(semantics); } @@ -714,8 +698,6 @@ "-incompatible_visibility_private_attributes_at_definition"; public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS = "-incompatible_top_level_aspects_require_providers"; - public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY = - "+incompatible_top_level_aspects_dependency"; // non-booleans public static final StarlarkSemantics.Key<String> EXPERIMENTAL_BUILTINS_BZL_PATH =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BuildTopLevelAspectsDetailsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BuildTopLevelAspectsDetailsFunction.java index 0cec99f..4ef1e56 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BuildTopLevelAspectsDetailsFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BuildTopLevelAspectsDetailsFunction.java
@@ -16,7 +16,6 @@ import com.google.common.base.Objects; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Interner; import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException; @@ -30,7 +29,6 @@ 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.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.server.FailureDetails.Analysis; import com.google.devtools.build.lib.server.FailureDetails.Analysis.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; @@ -47,7 +45,6 @@ import java.util.Map; import javax.annotation.Nullable; import net.starlark.java.eval.EvalException; -import net.starlark.java.eval.StarlarkSemantics; /** * SkyFunction to load top level aspects, build the dependency relation between them based on the @@ -73,30 +70,9 @@ BuildTopLevelAspectsDetailsKey topLevelAspectsDetailsKey = (BuildTopLevelAspectsDetailsKey) skyKey.argument(); - ImmutableList<AspectClass> topLevelAspectsClasses = - topLevelAspectsDetailsKey.getTopLevelAspectsClasses(); - StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env); - if (starlarkSemantics == null) { - return null; - } - boolean buildTopLevelAspectsDependency = - starlarkSemantics.getBool(BuildLanguageOptions.INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY); - if (!buildTopLevelAspectsDependency) { - // If building a relation between top-level aspects is not required, then we can remove - // duplicate aspects by keeping the first occurrence of each aspect. - topLevelAspectsClasses = ImmutableSet.copyOf(topLevelAspectsClasses).asList(); - - // Then return a list of indenpendent aspects to be applied on the top-level targets - ImmutableList<AspectDetails> aspectsDetailsList = - getIndependentTopLevelAspects(env, topLevelAspectsClasses); - if (aspectsDetailsList == null) { - return null; // some aspects are not loaded - } - return new BuildTopLevelAspectsDetailsValue(aspectsDetailsList); - } - - ImmutableList<Aspect> topLevelAspects = getTopLevelAspects(env, topLevelAspectsClasses); + ImmutableList<Aspect> topLevelAspects = + getTopLevelAspects(env, topLevelAspectsDetailsKey.getTopLevelAspectsClasses()); if (topLevelAspects == null) { return null; // some aspects are not loaded @@ -155,31 +131,6 @@ } @Nullable - private static ImmutableList<AspectDetails> getIndependentTopLevelAspects( - Environment env, ImmutableList<AspectClass> topLevelAspectsClasses) - throws InterruptedException, BuildTopLevelAspectsDetailsFunctionException { - - ImmutableList.Builder<AspectDetails> aspectDetailsList = ImmutableList.builder(); - - for (AspectClass aspectClass : topLevelAspectsClasses) { - if (aspectClass instanceof StarlarkAspectClass) { - StarlarkAspect starlarkAspect = loadStarlarkAspect(env, (StarlarkAspectClass) aspectClass); - if (starlarkAspect == null) { - return null; - } - aspectDetailsList.add( - new AspectDetails( - ImmutableList.of(), new AspectDescriptor(starlarkAspect.getAspectClass()))); - } else { - aspectDetailsList.add( - new AspectDetails(ImmutableList.of(), new AspectDescriptor(aspectClass))); - } - } - - return aspectDetailsList.build(); - } - - @Nullable private static ImmutableList<Aspect> getTopLevelAspects( Environment env, ImmutableList<AspectClass> topLevelAspectsClasses) throws InterruptedException, BuildTopLevelAspectsDetailsFunctionException {
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java index 9a15bc4..28bfbb2 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java
@@ -881,26 +881,6 @@ } @Test - public void duplicateTopLevelAspects_duplicateAspectsIgnored() throws Exception { - AspectApplyingToFiles aspectApplyingToFiles = new AspectApplyingToFiles(); - setRulesAndAspectsAvailableInTests(ImmutableList.of(aspectApplyingToFiles), ImmutableList.of()); - pkg("a", "java_binary(name = 'x', main_class = 'x.FooBar', srcs = ['x.java'])"); - useConfiguration("--noincompatible_top_level_aspects_dependency"); - - AnalysisResult analysisResult = - update( - new EventBus(), - defaultFlags(), - ImmutableList.of(aspectApplyingToFiles.getName(), aspectApplyingToFiles.getName()), - "//a:x_deploy.jar"); - - ConfiguredAspect aspect = Iterables.getOnlyElement(analysisResult.getAspectsMap().values()); - AspectApplyingToFiles.Provider provider = - aspect.getProvider(AspectApplyingToFiles.Provider.class); - assertThat(provider.getLabel()).isEqualTo(Label.parseAbsoluteUnchecked("//a:x_deploy.jar")); - } - - @Test public void duplicateTopLevelAspects_duplicateAspectsNotAllowed() throws Exception { AspectApplyingToFiles aspectApplyingToFiles = new AspectApplyingToFiles(); setRulesAndAspectsAvailableInTests(ImmutableList.of(aspectApplyingToFiles), ImmutableList.of());
diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index d83c731..9fe547e 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java
@@ -146,7 +146,6 @@ "--incompatible_no_rule_outputs_param=" + rand.nextBoolean(), "--incompatible_run_shell_command_string=" + rand.nextBoolean(), "--incompatible_struct_has_no_methods=" + rand.nextBoolean(), - "--incompatible_top_level_aspects_dependency=" + rand.nextBoolean(), "--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(), "--incompatible_require_linker_input_cc_api=" + rand.nextBoolean(), "--incompatible_restrict_string_escapes=" + rand.nextBoolean(), @@ -196,7 +195,6 @@ .setBool(BuildLanguageOptions.INCOMPATIBLE_NO_RULE_OUTPUTS_PARAM, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_RUN_SHELL_COMMAND_STRING, rand.nextBoolean()) .setBool(BuildLanguageOptions.INCOMPATIBLE_STRUCT_HAS_NO_METHODS, rand.nextBoolean()) - .setBool(BuildLanguageOptions.INCOMPATIBLE_TOP_LEVEL_ASPECTS_DEPENDENCY, rand.nextBoolean()) .setBool( BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION, rand.nextBoolean())
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 6a4921f..5bf954d 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
@@ -5411,83 +5411,6 @@ "aspect a3 on target //test:main sees a1p = a1p_val and sees a2p = a2p_val"); } - /** - * --aspects = a1, a2, a1: aspect a1 requires a2p, a2 provides a2p. - * - * <p>top level aspects list is deduplicated when --incompatible_top_level_aspects_dependency is - * disabled and only the first occurrence of a1 will be there so a1 won't get the value of a2p. - */ - @Test - public void testTopLevelAspectOnAspect_duplicateAspectsIgnored() throws Exception { - scratch.file( - "test/defs.bzl", - "a2p = provider()", - "a1_result = provider()", - "", - "def _a1_impl(target, ctx):", - " result = 'aspect a1 on target {}'.format(target.label)", - " if a2p in target:", - " result += ' sees a2p = {}'.format(target[a2p].value)", - " else:", - " result += ' cannot see a2p'", - " complete_result = []", - " if ctx.rule.attr.deps:", - " for dep in ctx.rule.attr.deps:", - " complete_result.extend(dep[a1_result].value)", - " complete_result.append(result)", - " return [a1_result(value = complete_result)]", - "a1 = aspect(", - " implementation = _a1_impl,", - " attr_aspects = ['deps'],", - " required_aspect_providers = [a2p]", - ")", - "", - "def _a2_impl(target, ctx):", - " return [a2p(value = 'a2p_val')]", - "a2 = aspect(", - " implementation = _a2_impl,", - " attr_aspects = ['deps'],", - " provides = [a2p]", - ")", - "", - "def _simple_rule_impl(ctx):", - " pass", - "simple_rule = rule(", - " implementation = _simple_rule_impl,", - " attrs = {", - " 'deps': attr.label_list(),", - " },", - ")"); - scratch.file( - "test/BUILD", - "load('//test:defs.bzl', 'simple_rule')", - "simple_rule(", - " name = 'main',", - " deps = [':dep_target'],", - ")", - "simple_rule(", - " name = 'dep_target',", - ")"); - useConfiguration("--noincompatible_top_level_aspects_dependency"); - - AnalysisResult analysisResult = - update( - ImmutableList.of("test/defs.bzl%a1", "test/defs.bzl%a2", "test/defs.bzl%a1"), - "//test:main"); - - Map<AspectKey, ConfiguredAspect> configuredAspects = analysisResult.getAspectsMap(); - ConfiguredAspect a1 = getConfiguredAspect(configuredAspects, "a1"); - assertThat(a1).isNotNull(); - StarlarkProvider.Key a1Result = - new StarlarkProvider.Key( - Label.parseAbsolute("//test:defs.bzl", ImmutableMap.of()), "a1_result"); - StructImpl a1ResultProvider = (StructImpl) a1.get(a1Result); - assertThat((Sequence<?>) a1ResultProvider.getValue("value")) - .containsExactly( - "aspect a1 on target //test:main cannot see a2p", - "aspect a1 on target //test:dep_target cannot see a2p"); - } - @Test public void testTopLevelAspectOnAspect_duplicateAspectsNotAllowed() throws Exception { scratch.file( @@ -5949,63 +5872,6 @@ assertContainsEvent("Provider a1p provided twice"); } - /** - * aspects = a1, a2; aspect a1 provides a1p provider and aspect a2 requires a1p provider. These - * top-level aspects are applied on top-level target `main` whose rule also provides a1p. - * - * <p>If the incompatible_top_level_aspects_dependency flag is disabled, aspects a1 and a2 will - * run independently and the build will succeed. a2 will only see the value of a1p provided by - * my_rule. - */ - @Test - public void testTopLevelAspects_duplicateRuleProviderPassed() throws Exception { - scratch.file( - "test/defs.bzl", - "a1p = provider()", - "a2p = provider()", - "", - "def _a1_impl(target, ctx):", - " return [a1p(value = 'aspect_a1p_val')]", - "a1 = aspect(", - " implementation = _a1_impl,", - " provides = [a1p],", - ")", - "", - "def _a2_impl(target, ctx):", - " result = 'aspect a2 on target {}'.format(target.label)", - " if a1p in target:", - " result += ' sees a1p = {}'.format(target[a1p].value)", - " else:", - " result += ' cannot see a1p'", - " return [a2p(value = result)]", - "a2 = aspect(", - " implementation = _a2_impl,", - " provides = [a2p],", - " required_aspect_providers = [a1p]", - ")", - "", - "def _my_rule_impl(ctx):", - " return [a1p(value = 'rule_a1p_val')]", - "my_rule = rule(", - " implementation = _my_rule_impl,", - ")"); - scratch.file("test/BUILD", "load('//test:defs.bzl', 'my_rule')", "my_rule(name = 'main')"); - useConfiguration("--noincompatible_top_level_aspects_dependency"); - reporter.removeHandler(failFastHandler); - - AnalysisResult analysisResult = - update(ImmutableList.of("test/defs.bzl%a1", "test/defs.bzl%a2"), "//test:main"); - - Map<AspectKey, ConfiguredAspect> configuredAspects = analysisResult.getAspectsMap(); - ConfiguredAspect a2 = getConfiguredAspect(configuredAspects, "a2"); - assertThat(a2).isNotNull(); - StarlarkProvider.Key a2Result = - new StarlarkProvider.Key(Label.parseAbsolute("//test:defs.bzl", ImmutableMap.of()), "a2p"); - StructImpl a2ResultProvider = (StructImpl) a2.get(a2Result); - assertThat((String) a2ResultProvider.getValue("value")) - .isEqualTo("aspect a2 on target //test:main sees a1p = rule_a1p_val"); - } - @Test public void testTopLevelAspectRequiresAspect_stackOfRequiredAspects() throws Exception { scratch.file(