Add `AspectOnRuleContext` to represent the analysis context during the evaluation of aspects. Also correct the order of merging attribute dependencies of rule and aspects to prepare for isolating main aspect dependencies from the underlying rule and base aspects dependencies. PiperOrigin-RevId: 575344066 Change-Id: I8e32a826d6852259f107233c4a2c0f2f7f621b38
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectOnRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectOnRuleContext.java new file mode 100644 index 0000000..76d9e05 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectOnRuleContext.java
@@ -0,0 +1,114 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.analysis; + +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.packages.Aspect; +import com.google.devtools.build.lib.packages.AspectDescriptor; +import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.AttributeMap; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import java.util.LinkedHashMap; +import java.util.Map; + +/** Extends {@link RuleContext} to provide all data available during the analysis of an aspect. */ +public final class AspectOnRuleContext extends RuleContext { + + /** + * A list of all aspects applied to the target. + * + * <p>The last aspect in the list is the main aspect that this context is for. + */ + private final ImmutableList<Aspect> aspects; + + private final ImmutableList<AspectDescriptor> aspectDescriptors; + + AspectOnRuleContext( + RuleContext.Builder builder, + AttributeMap ruleAttributes, + ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> prerequisitesCollection, + ExecGroupCollection execGroupCollection) { + super( + builder, + new AspectAwareAttributeMapper( + ruleAttributes, mergeAspectsAttributes(builder.getAspects())), + prerequisitesCollection, + execGroupCollection); + + this.aspects = builder.getAspects(); + this.aspectDescriptors = aspects.stream().map(Aspect::getDescriptor).collect(toImmutableList()); + } + + /** + * Merge the attributes of the aspects in the aspects path. + * + * <p>For attributes with the same name, the one that is first encountered takes precedence. + */ + private static ImmutableMap<String, Attribute> mergeAspectsAttributes( + ImmutableList<Aspect> aspects) { + if (aspects.isEmpty()) { + return ImmutableMap.of(); + } else if (aspects.size() == 1) { + return aspects.get(0).getDefinition().getAttributes(); + } else { + LinkedHashMap<String, Attribute> aspectAttributes = new LinkedHashMap<>(); + for (Aspect aspect : aspects) { + ImmutableMap<String, Attribute> currentAttributes = aspect.getDefinition().getAttributes(); + for (Map.Entry<String, Attribute> kv : currentAttributes.entrySet()) { + aspectAttributes.putIfAbsent(kv.getKey(), kv.getValue()); + } + } + return ImmutableMap.copyOf(aspectAttributes); + } + } + + @Override + public ImmutableList<Aspect> getAspects() { + return aspects; + } + + /** + * Return the main aspect of this context. + * + * <p>It is the last aspect in the list of aspects applied to a target; all other aspects are the + * ones main aspect sees as specified by its "required_aspect_providers"). + */ + @Override + public Aspect getMainAspect() { + return Iterables.getLast(aspects); + } + + /** All aspects applied to the rule. */ + @Override + public ImmutableList<AspectDescriptor> getAspectDescriptors() { + return aspectDescriptors; + } + + @Override + public boolean useAutoExecGroups() { + ImmutableMap<String, Attribute> aspectAttributes = + getMainAspect().getDefinition().getAttributes(); + if (aspectAttributes.containsKey("$use_auto_exec_groups")) { + return (boolean) aspectAttributes.get("$use_auto_exec_groups").getDefaultValueUnchecked(); + } else { + return getConfiguration().useAutoExecGroups(); + } + } +}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 1920c27..892b9bcf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -165,6 +165,7 @@ "AnalysisRootCauseEvent.java", "AnalysisUtils.java", "AspectCompleteEvent.java", + "AspectOnRuleContext.java", "AspectResolutionHelpers.java", "AspectValue.java", "BaseRuleClasses.java",
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java index 93e06af..f350906 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -51,7 +51,6 @@ import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.AdvertisedProviderSet; import com.google.devtools.build.lib.packages.Aspect; -import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy; import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy; import com.google.devtools.build.lib.packages.EnvironmentGroup; @@ -75,7 +74,6 @@ import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.lib.util.OrderedSetMultimap; -import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -308,7 +306,7 @@ .setActionOwnerSymbol(configuredTargetKey) .setMutability(Mutability.create("configured target")) .setVisibility(convertVisibility(prerequisiteMap, env.getEventHandler(), rule)) - .setPrerequisites(transformPrerequisiteMap(prerequisiteMap)) + .setPrerequisites(removeToolchainDeps(prerequisiteMap)) .setConfigConditions(configConditions) .setToolchainContexts(toolchainContexts) .setExecGroupCollectionBuilder(execGroupCollectionBuilder) @@ -529,15 +527,16 @@ } @VisibleForTesting - public static OrderedSetMultimap<Attribute, ConfiguredTargetAndData> transformPrerequisiteMap( + public static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> removeToolchainDeps( OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> map) { - OrderedSetMultimap<Attribute, ConfiguredTargetAndData> result = OrderedSetMultimap.create(); + OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> result = + OrderedSetMultimap.create(); + for (Map.Entry<DependencyKind, ConfiguredTargetAndData> entry : map.entries()) { if (DependencyKind.isToolchain(entry.getKey())) { continue; } - Attribute attribute = entry.getKey().getAttribute(); - result.put(attribute, entry.getValue()); + result.put(entry.getKey(), entry.getValue()); } return result; @@ -573,8 +572,7 @@ .setMutability(Mutability.create("aspect")) .setVisibility( convertVisibility(prerequisiteMap, env.getEventHandler(), associatedTarget)) - .setPrerequisites(transformPrerequisiteMap(prerequisiteMap)) - .setAspectAttributes(mergeAspectAttributes(aspectPath)) + .setPrerequisites(removeToolchainDeps(prerequisiteMap)) .setConfigConditions(configConditions) .setToolchainContexts(toolchainContexts) .setExecGroupCollectionBuilder(execGroupCollectionBuilder) @@ -687,25 +685,6 @@ } } - private static ImmutableMap<String, Attribute> mergeAspectAttributes( - ImmutableList<Aspect> aspectPath) { - if (aspectPath.isEmpty()) { - return ImmutableMap.of(); - } else if (aspectPath.size() == 1) { - return aspectPath.get(0).getDefinition().getAttributes(); - } else { - LinkedHashMap<String, Attribute> aspectAttributes = new LinkedHashMap<>(); - for (Aspect underlyingAspect : aspectPath) { - ImmutableMap<String, Attribute> currentAttributes = - underlyingAspect.getDefinition().getAttributes(); - for (Map.Entry<String, Attribute> kv : currentAttributes.entrySet()) { - aspectAttributes.putIfAbsent(kv.getKey(), kv.getValue()); - } - } - return ImmutableMap.copyOf(aspectAttributes); - } - } - private static void validateAdvertisedProviders( ConfiguredAspect configuredAspect, AspectKeyCreator.AspectKey aspectKey,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolutionHelpers.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolutionHelpers.java index 22e0287..21476a6 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolutionHelpers.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolutionHelpers.java
@@ -89,7 +89,7 @@ public static DependencyLabels computeDependencyLabels( TargetAndConfiguration node, - Iterable<Aspect> aspects, + ImmutableList<Aspect> aspects, ImmutableMap<Label, ConfigMatchingProvider> configConditions, @Nullable ToolchainCollection<ToolchainContext> toolchainContexts) throws Failure { @@ -241,7 +241,7 @@ private static void visitRule( TargetAndConfiguration node, - Iterable<Aspect> aspects, + ImmutableList<Aspect> aspects, ConfiguredAttributeMapper attributeMap, @Nullable ToolchainCollection<ToolchainContext> toolchainContexts, OrderedSetMultimap<DependencyKind, Label> outgoingLabels) @@ -448,36 +448,56 @@ /** Returns the attributes that should be visited for this rule/aspect combination. */ private static ImmutableList<AttributeDependencyKind> getAttributes( - Rule rule, Iterable<Aspect> aspects) { + Rule rule, ImmutableList<Aspect> aspects) { ImmutableList.Builder<AttributeDependencyKind> result = ImmutableList.builder(); - // If processing aspects, aspect attribute names may conflict with the attribute names of - // rules they attach to. If this occurs, the highest-level aspect attribute takes precedence. - HashSet<String> aspectProcessedAttributes = new HashSet<>(); + HashSet<String> ruleAndBaseAspectsProcessedAttributes = new HashSet<>(); - addAspectAttributes(aspects, aspectProcessedAttributes, result); - List<Attribute> ruleDefs = rule.getRuleClassObject().getAttributes(); - for (Attribute attribute : ruleDefs) { - if (!aspectProcessedAttributes.contains(attribute.getName())) { + // For aspects evaluation, all attributes of the main aspect (last aspect in {@code aspects} + // should be added, even if they have the same name as an attribute in the rule or a base aspect + // because main aspect attributes are separated and retrieved from `ctx.attr`. + + // Attributes of the underlying rule and base aspects are merged and retrieved from + // `ctx.rule.attr` with rule attributes taking precedence then aspects' attributes based on the + // aspect order in the aspects path (lowest order to highest). + + List<Attribute> ruleAttributes = rule.getRuleClassObject().getAttributes(); + for (Attribute attribute : ruleAttributes) { result.add(AttributeDependencyKind.forRule(attribute)); - } + ruleAndBaseAspectsProcessedAttributes.add(attribute.getName()); } + + addAspectAttributes(aspects, ruleAndBaseAspectsProcessedAttributes, result); + return result.build(); } private static ImmutableList<AttributeDependencyKind> getAspectAttributes( - Iterable<Aspect> aspects) { + ImmutableList<Aspect> aspects) { ImmutableList.Builder<AttributeDependencyKind> result = ImmutableList.builder(); addAspectAttributes(aspects, new HashSet<>(), result); return result.build(); } private static void addAspectAttributes( - Iterable<Aspect> aspects, - Set<String> aspectProcessedAttributes, + ImmutableList<Aspect> aspects, + Set<String> processedAttributes, ImmutableList.Builder<AttributeDependencyKind> attributes) { - for (Aspect aspect : aspects) { + + if (aspects.isEmpty()) { + return; + } + + // Add all the main aspect's attributes + Aspect mainAspect = Iterables.getLast(aspects, null); + for (Attribute attribute : mainAspect.getDefinition().getAttributes().values()) { + attributes.add(AttributeDependencyKind.forAspect(attribute, mainAspect.getAspectClass())); + } + + // For base aspects, if multiple attributes have the same name, take the first encountered in + // the aspects path. + for (Aspect aspect : aspects.subList(0, aspects.size() - 1)) { for (Attribute attribute : aspect.getDefinition().getAttributes().values()) { - if (aspectProcessedAttributes.add(attribute.getName())) { + if (processedAttributes.add(attribute.getName())) { attributes.add(AttributeDependencyKind.forAspect(attribute, aspect.getAspectClass())); } }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PrerequisitesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/PrerequisitesCollection.java index e4a1a4e..642d5c3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/PrerequisitesCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/PrerequisitesCollection.java
@@ -26,6 +26,7 @@ import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue; import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap; import com.google.devtools.build.lib.collect.nestedset.NestedSet; +import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.AttributeMap; import com.google.devtools.build.lib.packages.BuiltinProvider; @@ -35,6 +36,7 @@ import com.google.devtools.build.lib.packages.StarlarkProviderWrapper; import com.google.devtools.build.lib.packages.Type.LabelClass; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import java.util.HashSet; import java.util.List; import java.util.Map; import javax.annotation.Nullable; @@ -49,7 +51,63 @@ private final Rule rule; private final String ruleClassNameForLogging; - PrerequisitesCollection( + static PrerequisitesCollection create( + AttributeMap attributes, + ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> prerequisitesMap, + ImmutableList<Aspect> aspects, + RuleErrorConsumer ruleErrorConsumer, + Rule rule, + String ruleClassNameForLogging) { + return new PrerequisitesCollection( + attributes, + mergePrerequisites(aspects, prerequisitesMap), + ruleErrorConsumer, + rule, + ruleClassNameForLogging); + } + + /** + * Merges the rule prerequisites with the aspects prerequisites giving precedence to aspects + * prerequisites if any. + * + * <p>TODO(b/293304543) merging prerequisites should be not needed after completing the solution + * to isolate main aspect dependencies from the underlying rule and aspects dependencies. + */ + private static ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> mergePrerequisites( + ImmutableList<Aspect> aspects, + ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> targetsMap) { + + ImmutableSortedKeyListMultimap.Builder<String, ConfiguredTargetAndData> + attributeNameToTargetsMap = ImmutableSortedKeyListMultimap.builder(); + HashSet<String> processedAttributes = new HashSet<>(); + + // add aspects prerequisites + for (Aspect aspect : aspects) { + for (Attribute attribute : aspect.getDefinition().getAttributes().values()) { + DependencyKind key = + DependencyKind.AttributeDependencyKind.forAspect(attribute, aspect.getAspectClass()); + if (targetsMap.containsKey(key)) { + if (processedAttributes.add(attribute.getName())) { + attributeNameToTargetsMap.putAll(attribute.getName(), targetsMap.get(key)); + } + } + } + } + + // add rule prerequisites + for (var entry : targetsMap.asMap().entrySet()) { + if (entry.getKey().getOwningAspect() == null) { + if (!processedAttributes.contains(entry.getKey().getAttribute().getName())) { + attributeNameToTargetsMap.putAll( + entry.getKey().getAttribute().getName(), entry.getValue()); + } + } + } + + return attributeNameToTargetsMap.build(); + } + + private PrerequisitesCollection( AttributeMap attributes, ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> prerequisitesMap, RuleErrorConsumer ruleErrorConsumer,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index 8f7741f..acc3808 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -14,7 +14,7 @@ package com.google.devtools.build.lib.analysis; -import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.devtools.build.lib.analysis.constraints.ConstraintConstants.OS_TO_CONSTRAINTS; import static com.google.devtools.build.lib.analysis.test.ExecutionInfo.DEFAULT_TEST_RUNNER_EXEC_GROUP; import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME; @@ -56,7 +56,6 @@ import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.RepositoryName; -import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.Aspect; @@ -118,9 +117,13 @@ * internal tests to ensure that RuleContext objects are not persisted that check the name of this * class, so update those tests if you change this class's name. * - * @see com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory + * <p>@see com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory + * + * <p>The class is intended to be sub-classed by {@link AspectOnRuleContext}, in order to share the + * code. However, it's not intended for sub-classing beyond that, and the constructor is + * intentionally package private to enforce that. */ -public final class RuleContext extends TargetContext +public class RuleContext extends TargetContext implements ActionConstructionContext, ActionRegistry, RuleErrorConsumer, AutoCloseable { /** Custom dependency validation logic. */ @@ -143,18 +146,9 @@ new Attribute.Builder<>(TOOLCHAIN_ATTR_NAME, BuildType.LABEL_LIST).build(); private final Rule rule; - /** - * A list of all aspects applied to the target. If this {@code RuleContext} is for a rule - * implementation, {@code aspects} is an empty list. - * - * <p>Otherwise, the last aspect in the list is the one that this {@code RuleContext} is for. - */ - private final ImmutableList<Aspect> aspects; - - private final ImmutableList<AspectDescriptor> aspectDescriptors; private final PrerequisitesCollection prerequisitesCollection; private final ImmutableMap<Label, ConfigMatchingProvider> configConditions; - private final AspectAwareAttributeMapper attributes; + private final AttributeMap attributes; private final FeatureSet features; private final String ruleClassNameForLogging; private final ConfigurationFragmentPolicy configurationFragmentPolicy; @@ -180,6 +174,7 @@ * partially migrated to {@code @_builtins}. */ private final StarlarkThread starlarkThread; + /** * The {@code ctx} object passed to a Starlark-defined rule's or aspect's implementation function. * This object may outlive the analysis phase, e.g. if it is returned in a provider. @@ -190,24 +185,26 @@ */ @Nullable private StarlarkRuleContext starlarkRuleContext; - private RuleContext( + /** + * The constructor is intentionally package private to be only used by {@link + * AspectOnRuleContext}. + */ + RuleContext( Builder builder, - AttributeMap ruleAttributes, - ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> targetMap, + AttributeMap attributes, + ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> targetMap, ExecGroupCollection execGroupCollection) { super( builder.env, builder.target.getAssociatedRule(), builder.configuration, - builder.prerequisiteMap.get(null), + getDirectPrerequisites(builder.prerequisiteMap), builder.visibility); this.rule = builder.target.getAssociatedRule(); - this.aspects = builder.aspects; - this.aspectDescriptors = aspects.stream().map(Aspect::getDescriptor).collect(toImmutableList()); this.configurationFragmentPolicy = builder.configurationFragmentPolicy; this.ruleClassProvider = builder.ruleClassProvider; this.configConditions = builder.configConditions.asProviders(); - this.attributes = new AspectAwareAttributeMapper(ruleAttributes, builder.aspectAttributes); + this.attributes = attributes; this.features = computeFeatures(); this.ruleClassNameForLogging = builder.getRuleClassNameForLogging(); this.actionOwnerSymbolGenerator = new SymbolGenerator<>(builder.actionOwnerSymbol); @@ -220,8 +217,13 @@ this.starlarkThread = createStarlarkThread(builder.mutability); // uses above state this.prerequisitesCollection = - new PrerequisitesCollection( - this.attributes, targetMap, this.reporter, this.rule, this.ruleClassNameForLogging); + PrerequisitesCollection.create( + attributes, + targetMap, + builder.aspects, + this.reporter, + this.rule, + this.ruleClassNameForLogging); } private FeatureSet computeFeatures() { @@ -234,6 +236,14 @@ FeatureSet.merge(pkg, rule), getConfiguration().getDefaultFeatures()); } + private static ImmutableSet<ConfiguredTargetAndData> getDirectPrerequisites( + OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap) { + return prerequisiteMap.entries().stream() + .filter(e -> e.getKey().getAttribute() == null) + .map(e -> e.getValue()) + .collect(toImmutableSet()); + } + public boolean isAllowTagsPropagation() { return getAnalysisEnvironment() .getStarlarkSemantics() @@ -284,7 +294,7 @@ } public ImmutableList<Aspect> getAspects() { - return aspects; + return ImmutableList.of(); } /** @@ -312,7 +322,7 @@ */ @Nullable public Aspect getMainAspect() { - return Streams.findLast(aspects.stream()).orElse(null); + return null; } /** @@ -334,7 +344,7 @@ /** All aspects applied to the rule. */ public ImmutableList<AspectDescriptor> getAspectDescriptors() { - return aspectDescriptors; + return ImmutableList.of(); } /** @@ -365,11 +375,6 @@ return prerequisitesCollection.getPrerequisiteConfiguredTargets(attributeName); } - @Override - public ActionOwner getActionOwner() { - return getActionOwner(DEFAULT_EXEC_GROUP_NAME); - } - /** * Returns a special action owner for test actions. Test actions should run on the target platform * rather than the host platform. Note that the value is not cached (on the assumption that this @@ -394,7 +399,11 @@ ActionOwner actionOwner = createActionOwner( - rule, aspectDescriptors, getConfiguration(), testExecProperties, testExecutionPlatform); + rule, + getAspectDescriptors(), + getConfiguration(), + testExecProperties, + testExecutionPlatform); if (actionOwner == null) { actionOwner = getActionOwner(); @@ -403,6 +412,11 @@ } @Override + public ActionOwner getActionOwner() { + return getActionOwner(DEFAULT_EXEC_GROUP_NAME); + } + + @Override @Nullable public ActionOwner getActionOwner(String execGroup) { if (actionOwners.containsKey(execGroup)) { @@ -414,7 +428,7 @@ ActionOwner actionOwner = createActionOwner( rule, - aspectDescriptors, + getAspectDescriptors(), getConfiguration(), execGroupCollection.getExecProperties(execGroup), getExecutionPlatform(execGroup)); @@ -977,9 +991,9 @@ */ public StarlarkRuleContext initStarlarkRuleContext() throws RuleErrorException { if (starlarkRuleContext == null) { - AspectDescriptor descriptor = - aspects.isEmpty() ? null : Iterables.getLast(aspects).getDescriptor(); - this.starlarkRuleContext = new StarlarkRuleContext(this, descriptor); + AspectDescriptor aspectDescriptor = + getMainAspect() == null ? null : getMainAspect().getDescriptor(); + this.starlarkRuleContext = new StarlarkRuleContext(this, aspectDescriptor); } return starlarkRuleContext; } @@ -1050,7 +1064,7 @@ return targetPlatform.label(); } - private boolean useAutoExecGroupsForRule() { + public boolean useAutoExecGroups() { if (attributes().has("$use_auto_exec_groups")) { return (boolean) attributes().get("$use_auto_exec_groups", Type.BOOLEAN); } else { @@ -1058,26 +1072,6 @@ } } - private boolean useAutoExecGroupsForAspect(Aspect aspect) { - ImmutableMap<String, Attribute> aspectAttributes = aspect.getDefinition().getAttributes(); - - if (aspectAttributes.containsKey("$use_auto_exec_groups")) { - return (boolean) aspectAttributes.get("$use_auto_exec_groups").getDefaultValueUnchecked(); - } else { - return getConfiguration().useAutoExecGroups(); - } - } - - public boolean useAutoExecGroups() { - Aspect aspect = getMainAspect(); - - if (aspect == null) { - return useAutoExecGroupsForRule(); - } else { - return useAutoExecGroupsForAspect(aspect); - } - } - /** * Returns the toolchain context from the default exec group. Important note: In case automatic * exec groups are enabled, use `getToolchainInfo(Label toolchainType)` function. @@ -1484,11 +1478,10 @@ private ConfiguredRuleClassProvider ruleClassProvider; private ConfigurationFragmentPolicy configurationFragmentPolicy; private ActionLookupKey actionOwnerSymbol; - private OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap; + private OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap; private ConfigConditions configConditions; private Mutability mutability; private NestedSet<PackageGroupContents> visibility; - private ImmutableMap<String, Attribute> aspectAttributes = ImmutableMap.of(); private ToolchainCollection<ResolvedToolchainContext> toolchainContexts; private ExecGroupCollection.Builder execGroupCollectionBuilder; private ImmutableMap<String, String> rawExecProperties; @@ -1539,17 +1532,19 @@ Preconditions.checkNotNull(configConditions); Preconditions.checkNotNull(mutability); Preconditions.checkNotNull(visibility); - ConfiguredAttributeMapper attributes = + ConfiguredAttributeMapper ruleAttributes = ConfiguredAttributeMapper.of( target.getAssociatedRule(), configConditions.asProviders(), configuration); - ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> targetMap = createTargetMap(); - validateExtraPrerequisites(attributeChecks, attributes); + ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> targetMap = createTargetMap(); + validateExtraPrerequisites(attributeChecks, ruleAttributes); - return new RuleContext( - this, - attributes, - targetMap, - createExecGroupCollection(execGroupCollectionBuilder, attributes)); + ExecGroupCollection execGroupCollection = + createExecGroupCollection(execGroupCollectionBuilder, ruleAttributes); + if (aspects.isEmpty()) { + return new RuleContext(this, ruleAttributes, targetMap, execGroupCollection); + } else { + return new AspectOnRuleContext(this, ruleAttributes, targetMap, execGroupCollection); + } } private ExecGroupCollection createExecGroupCollection( @@ -1638,18 +1633,11 @@ */ @CanIgnoreReturnValue public Builder setPrerequisites( - OrderedSetMultimap<Attribute, ConfiguredTargetAndData> prerequisiteMap) { + OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> prerequisiteMap) { this.prerequisiteMap = Preconditions.checkNotNull(prerequisiteMap); return this; } - /** Adds attributes which are defined by an Aspect (and not by RuleClass). */ - @CanIgnoreReturnValue - public Builder setAspectAttributes(Map<String, Attribute> aspectAttributes) { - this.aspectAttributes = ImmutableMap.copyOf(aspectAttributes); - return this; - } - /** * Sets the configuration conditions needed to determine which paths to follow for this rule's * configurable attributes. @@ -1703,14 +1691,17 @@ return this; } - /** Determines and returns a map from attribute name to list of configured targets. */ - private ImmutableSortedKeyListMultimap<String, ConfiguredTargetAndData> createTargetMap() { - ImmutableSortedKeyListMultimap.Builder<String, ConfiguredTargetAndData> mapBuilder = - ImmutableSortedKeyListMultimap.builder(); + /** + * Filter only attribute-based prerequisites, validate them and return them in a map from {@link + * DependencyKind} to list of configured targets. + */ + private ImmutableListMultimap<DependencyKind, ConfiguredTargetAndData> createTargetMap() { + ImmutableListMultimap.Builder<DependencyKind, ConfiguredTargetAndData> mapBuilder = + ImmutableListMultimap.builder(); - for (Map.Entry<Attribute, Collection<ConfiguredTargetAndData>> entry : + for (Map.Entry<DependencyKind, Collection<ConfiguredTargetAndData>> entry : prerequisiteMap.asMap().entrySet()) { - Attribute attribute = entry.getKey(); + Attribute attribute = entry.getKey().getAttribute(); if (attribute == null) { continue; } @@ -1725,13 +1716,13 @@ for (ConfiguredTargetAndData configuredTarget : entry.getValue()) { if (filter.apply(configuredTarget.getRuleClass())) { validateDirectPrerequisite(attribute, configuredTarget); - mapBuilder.put(attribute.getName(), configuredTarget); + mapBuilder.put(entry.getKey(), configuredTarget); } } } else { for (ConfiguredTargetAndData configuredTarget : entry.getValue()) { validateDirectPrerequisite(attribute, configuredTarget); - mapBuilder.put(attribute.getName(), configuredTarget); + mapBuilder.put(entry.getKey(), configuredTarget); } } } @@ -1881,6 +1872,10 @@ return Streams.findLast(aspects.stream()).orElse(null); } + ImmutableList<Aspect> getAspects() { + return aspects; + } + boolean isStarlarkRuleOrAspect() { Aspect mainAspect = getMainAspect(); if (mainAspect != null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/DependencyResolver.java index b4a1c24..7a500d8 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/DependencyResolver.java
@@ -669,7 +669,7 @@ public static OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> computeDependencies( State state, ConfiguredTargetKey configuredTargetKey, - Iterable<Aspect> aspects, + ImmutableList<Aspect> aspects, StarlarkTransitionCache transitionCache, @Nullable StarlarkAttributeTransitionProvider starlarkTransitionProvider, LookupEnvironment env,
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index ae8a327..d16f4b3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -418,7 +418,7 @@ NestedSetBuilder.create( Order.STABLE_ORDER, PackageGroupContents.create(ImmutableList.of(PackageSpecification.everything())))) - .setPrerequisites(ConfiguredTargetFactory.transformPrerequisiteMap(prerequisiteMap)) + .setPrerequisites(ConfiguredTargetFactory.removeToolchainDeps(prerequisiteMap)) .setConfigConditions(ConfigConditions.EMPTY) .setToolchainContexts(resolvedToolchainContext.build()) .setExecGroupCollectionBuilder(state.execGroupCollectionBuilder)
diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh index 14a07e8..a3d4f6f 100755 --- a/src/test/shell/integration/aspect_test.sh +++ b/src/test/shell/integration/aspect_test.sh
@@ -1583,9 +1583,8 @@ expect_log "aspect_b on target @@\?//test:t3" expect_log "aspect_b on target @@\?//test:t2" - # the underlying aspect (aspect_b) cannot propagate to the dependencies of - # its main aspect (aspect_a) - expect_not_log "aspect_b on target @@\?//test:tool" + # the underlying aspect (aspect_b) can propagate to its main aspect dependencies + expect_log "aspect_b on target @@\?//test:tool_a" # the main aspect (aspect_a) can propagate to its underlying aspects # dependencies expect_log "aspect_a on target @@\?//test:tool_b" @@ -1676,4 +1675,96 @@ expect_log "aspect on @@\?//test:t1 can see its dep flag val = v2" } +#TODO(b/293304543): the main aspect attributes should not overwrite the underlying +# rule and base aspects attributes that have the same name. +function test_merge_of_aspects_and_rule_conflicting_attributes() { + local package="test" + mkdir -p "${package}" + + cat > "${package}/defs.bzl" <<EOF +prov_b = provider() + +def _aspect_a_impl(target, ctx): + prefix = 'aspect_a on target {}'.format(target.label) + same_attr_message = 'aspect_a _tool={} and merged_rule_and_base_aspects _tool={}'.format(ctx.attr._tool.label, ctx.rule.attr._tool.label) + diff_attr_message = '_tool_a={}, _tool_r1={}, _tool_b={}'.format(ctx.attr._tool_a.label, ctx.rule.attr._tool_r1.label, ctx.rule.attr._tool_b.label) + + print('{}: {}'.format(prefix, same_attr_message)) + print('{}: {}'.format(prefix, diff_attr_message)) + + return [] + +aspect_a = aspect( + implementation = _aspect_a_impl, + attr_aspects = ['dep'], + required_aspect_providers = [prov_b], + attrs = { + '_tool' : attr.label(default = "//${package}:aspect_a_tool"), + '_tool_a' : attr.label(default = "//${package}:aspect_a_diff_tool"), + }, +) + +def _aspect_b_impl(target, ctx): + prefix = 'aspect_b on target {}'.format(target.label) + same_attr_message = 'aspect_b _tool={} and rule _tool={}'.format(ctx.attr._tool.label, ctx.rule.attr._tool.label) + diff_attr_message = '_tool_b={} and _tool_r1={}'.format(ctx.attr._tool_b.label, ctx.rule.attr._tool_r1.label) + + print('{}: {}'.format(prefix, same_attr_message)) + print('{}: {}'.format(prefix, diff_attr_message)) + + return [prov_b()] + +aspect_b = aspect( + implementation = _aspect_b_impl, + attr_aspects = ['dep'], + attrs = { + '_tool' : attr.label(default = "//${package}:aspect_b_tool"), + '_tool_b' : attr.label(default = "//${package}:aspect_b_diff_tool"), + }, + provides = [prov_b], +) + +def _rule_impl(ctx): + pass + +r1 = rule( + implementation = _rule_impl, + attrs = { + '_tool' : attr.label(default = "//${package}:r1_tool"), + '_tool_r1' : attr.label(default = "//${package}:r1_diff_tool"), + }, +) + +EOF + + cat > "${package}/tool.sh" <<EOF +EOF + + cat > "${package}/BUILD" <<EOF +load('//test:defs.bzl', 'r1') +r1( + name = 't1', +) + +sh_binary(name = "aspect_a_tool", srcs = ["tool.sh"]) +sh_binary(name = "aspect_b_tool", srcs = ["tool.sh"]) +sh_binary(name = "r1_tool", srcs = ["tool.sh"]) + +sh_binary(name = "aspect_a_diff_tool", srcs = ["tool.sh"]) +sh_binary(name = "aspect_b_diff_tool", srcs = ["tool.sh"]) +sh_binary(name = "r1_diff_tool", srcs = ["tool.sh"]) +EOF + + bazel build "//${package}:t1" \ + --aspects="//${package}:defs.bzl%aspect_b,//${package}:defs.bzl%aspect_a" \ + &> $TEST_log || fail "Build failed" + + expect_log "aspect_b on target @@\?//test:t1: aspect_b _tool=@@\?//test:aspect_b_tool and rule _tool=@@\?//test:aspect_b_tool" + expect_log "aspect_b on target @@\?//test:t1: _tool_b=@@\?//test:aspect_b_diff_tool and _tool_r1=@@\?//test:r1_diff_tool" + + expect_log "aspect_a on target @@\?//test:t1: aspect_a _tool=@@\?//test:aspect_a_tool and merged_rule_and_base_aspects _tool=@@\?//test:aspect_a_tool" + expect_log "aspect_a on target @@\?//test:t1: _tool_a=@@\?//test:aspect_a_diff_tool, _tool_r1=@@\?//test:r1_diff_tool, _tool_b=@@\?//test:aspect_b_diff_tool" + +} + run_suite "Tests for aspects"