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(
