diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectContext.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectContext.java new file mode 100644 index 00000000000000..acfaf989f8ef50 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectContext.java @@ -0,0 +1,278 @@ +// 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.common.collect.Streams; +import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap; +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.packages.semantics.BuildLanguageOptions; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; +import java.util.Collection; +import java.util.HashSet; +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 AspectContext extends RuleContext { + + /** + * A list of all aspects applied to the target. + * + *

The last aspect in the list is the main aspect that this context is for. + */ + private final ImmutableList aspects; + + private final ImmutableList aspectDescriptors; + + // If (separate_aspect_deps) flag is disabled, holds all merged prerequisites otherwise it will + // only have the main aspect's prerequisites. + private final PrerequisitesCollection mainAspectPrerequisites; + + AspectContext( + RuleContext.Builder builder, + AspectAwareAttributeMapper aspectAwareAttributeMapper, + PrerequisitesCollection ruleAndBaseAspectsPrerequisites, + PrerequisitesCollection mainAspectPrerequisites, + ExecGroupCollection execGroupCollection) { + super( + builder, aspectAwareAttributeMapper, ruleAndBaseAspectsPrerequisites, execGroupCollection); + + this.aspects = builder.getAspects(); + this.aspectDescriptors = aspects.stream().map(Aspect::getDescriptor).collect(toImmutableList()); + this.mainAspectPrerequisites = mainAspectPrerequisites; + } + + /** + * Merge the attributes of the aspects in the aspects path. + * + *

For attributes with the same name, the one that is first encountered takes precedence. + */ + private static ImmutableMap mergeAspectsAttributes( + ImmutableList aspects) { + if (aspects.isEmpty()) { + return ImmutableMap.of(); + } else if (aspects.size() == 1) { + return aspects.get(0).getDefinition().getAttributes(); + } else { + LinkedHashMap aspectAttributes = new LinkedHashMap<>(); + for (Aspect aspect : aspects) { + ImmutableMap currentAttributes = aspect.getDefinition().getAttributes(); + for (Map.Entry kv : currentAttributes.entrySet()) { + aspectAttributes.putIfAbsent(kv.getKey(), kv.getValue()); + } + } + return ImmutableMap.copyOf(aspectAttributes); + } + } + + static AspectContext create( + Builder builder, + AttributeMap ruleAttributes, + ImmutableListMultimap targetsMap, + ExecGroupCollection execGroupCollection) { + + if (builder.separateAspectDeps()) { + return createAspectContextWithSeparatedPrerequisites( + builder, ruleAttributes, targetsMap, execGroupCollection); + } else { + return createAspectContextWithMergedPrerequisites( + builder, ruleAttributes, targetsMap, execGroupCollection); + } + } + + /** + * Merges the rule prerequisites with the aspects prerequisites giving precedence to aspects + * prerequisites if any. + * + *

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 AspectContext createAspectContextWithMergedPrerequisites( + RuleContext.Builder builder, + AttributeMap ruleAttributes, + ImmutableListMultimap targetsMap, + ExecGroupCollection execGroupCollection) { + + ImmutableSortedKeyListMultimap.Builder + attributeNameToTargetsMap = ImmutableSortedKeyListMultimap.builder(); + HashSet processedAttributes = new HashSet<>(); + + // add aspects prerequisites + for (Aspect aspect : builder.getAspects()) { + 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()); + } + } + } + AspectAwareAttributeMapper ruleAndAspectsAttributes = + new AspectAwareAttributeMapper( + ruleAttributes, mergeAspectsAttributes(builder.getAspects())); + PrerequisitesCollection mergedPrerequisitesCollection = + new PrerequisitesCollection( + attributeNameToTargetsMap.build(), + ruleAndAspectsAttributes, + builder.getErrorConsumer(), + builder.getRule(), + builder.getRuleClassNameForLogging()); + return new AspectContext( + builder, + ruleAndAspectsAttributes, + /* ruleAndBaseAspectsPrerequisites= */ mergedPrerequisitesCollection, + /* mainAspectPrerequisites= */ mergedPrerequisitesCollection, + execGroupCollection); + } + + /** + * Create prerequisites collection for aspect evaluation separating the main aspect prerequisites + * from the underlying rule and base aspects prerequisites. + */ + private static AspectContext createAspectContextWithSeparatedPrerequisites( + RuleContext.Builder builder, + AttributeMap ruleAttributes, + ImmutableListMultimap prerequisitesMap, + ExecGroupCollection execGroupCollection) { + ImmutableSortedKeyListMultimap.Builder + mainAspectPrerequisites = ImmutableSortedKeyListMultimap.builder(); + ImmutableSortedKeyListMultimap.Builder + ruleAndBaseAspectsPrerequisites = ImmutableSortedKeyListMultimap.builder(); + + Aspect mainAspect = Iterables.getLast(builder.getAspects()); + + for (Map.Entry> entry : + prerequisitesMap.asMap().entrySet()) { + String attributeName = entry.getKey().getAttribute().getName(); + + if (mainAspect.getAspectClass().equals(entry.getKey().getOwningAspect())) { + mainAspectPrerequisites.putAll(attributeName, entry.getValue()); + } else { + ruleAndBaseAspectsPrerequisites.putAll(attributeName, entry.getValue()); + } + } + + return new AspectContext( + builder, + new AspectAwareAttributeMapper( + ruleAttributes, mergeAspectsAttributes(builder.getAspects())), + new PrerequisitesCollection( + ruleAndBaseAspectsPrerequisites.build(), + mergeRuleAndBaseAspectsAttributes(ruleAttributes, builder.getAspects()), + builder.getErrorConsumer(), + builder.getRule(), + builder.getRuleClassNameForLogging()), + new PrerequisitesCollection( + mainAspectPrerequisites.build(), + mainAspect.getDefinition().getAttributes(), + builder.getErrorConsumer(), + builder.getRule(), + builder.getRuleClassNameForLogging()), + execGroupCollection); + } + + private static AspectAwareAttributeMapper mergeRuleAndBaseAspectsAttributes( + AttributeMap ruleAttributes, ImmutableList aspects) { + LinkedHashMap mergedBaseAspectsAttributes = new LinkedHashMap<>(); + for (int i = 0; i < aspects.size() - 1; i++) { + for (Attribute attribute : aspects.get(i).getDefinition().getAttributes().values()) { + mergedBaseAspectsAttributes.putIfAbsent(attribute.getName(), attribute); + } + } + return new AspectAwareAttributeMapper( + ruleAttributes, ImmutableMap.copyOf(mergedBaseAspectsAttributes)); + } + + @Override + PrerequisitesCollection getOwningPrerequisitesCollection(String attributeName) { + if (mainAspectPrerequisites.has(attributeName)) { + return mainAspectPrerequisites; + } + return getRulePrerequisitesCollection(); + } + + public PrerequisitesCollection getMainAspectPrerequisitesCollection() { + return mainAspectPrerequisites; + } + + @Override + public ImmutableList getAspects() { + return aspects; + } + + /** + * Return the main aspect of this context. + * + *

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 getAspectDescriptors() { + return aspectDescriptors; + } + + @Override + public boolean useAutoExecGroups() { + ImmutableMap aspectAttributes = + getMainAspect().getDefinition().getAttributes(); + if (aspectAttributes.containsKey("$use_auto_exec_groups")) { + return (boolean) aspectAttributes.get("$use_auto_exec_groups").getDefaultValueUnchecked(); + } else { + return getConfiguration().useAutoExecGroups(); + } + } + + @Override + public ImmutableList getAllPrerequisites() { + boolean separateAspectDeps = + getAnalysisEnvironment() + .getStarlarkSemantics() + .getBool(BuildLanguageOptions.SEPARATE_ASPECT_DEPS); + if (separateAspectDeps) { + return Streams.concat( + mainAspectPrerequisites.getAllPrerequisites().stream(), + getRulePrerequisitesCollection().getAllPrerequisites().stream()) + .collect(toImmutableList()); + } + return super.getAllPrerequisites(); + } +} 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 deleted file mode 100644 index 76d9e05df637d1..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectOnRuleContext.java +++ /dev/null @@ -1,114 +0,0 @@ -// 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. - * - *

The last aspect in the list is the main aspect that this context is for. - */ - private final ImmutableList aspects; - - private final ImmutableList aspectDescriptors; - - AspectOnRuleContext( - RuleContext.Builder builder, - AttributeMap ruleAttributes, - ImmutableListMultimap 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. - * - *

For attributes with the same name, the one that is first encountered takes precedence. - */ - private static ImmutableMap mergeAspectsAttributes( - ImmutableList aspects) { - if (aspects.isEmpty()) { - return ImmutableMap.of(); - } else if (aspects.size() == 1) { - return aspects.get(0).getDefinition().getAttributes(); - } else { - LinkedHashMap aspectAttributes = new LinkedHashMap<>(); - for (Aspect aspect : aspects) { - ImmutableMap currentAttributes = aspect.getDefinition().getAttributes(); - for (Map.Entry kv : currentAttributes.entrySet()) { - aspectAttributes.putIfAbsent(kv.getKey(), kv.getValue()); - } - } - return ImmutableMap.copyOf(aspectAttributes); - } - } - - @Override - public ImmutableList getAspects() { - return aspects; - } - - /** - * Return the main aspect of this context. - * - *

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 getAspectDescriptors() { - return aspectDescriptors; - } - - @Override - public boolean useAutoExecGroups() { - ImmutableMap 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 50912f4befe5fb..208da8fe11db81 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -165,7 +165,7 @@ java_library( "AnalysisRootCauseEvent.java", "AnalysisUtils.java", "AspectCompleteEvent.java", - "AspectOnRuleContext.java", + "AspectContext.java", "AspectResolutionHelpers.java", "AspectValue.java", "BaseRuleClasses.java", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PrerequisiteArtifacts.java b/src/main/java/com/google/devtools/build/lib/analysis/PrerequisiteArtifacts.java index c584db114d8cc4..ebcfaaeaf1c0c1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/PrerequisiteArtifacts.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/PrerequisiteArtifacts.java @@ -62,8 +62,14 @@ static PrerequisiteArtifacts get(RuleContext ruleContext, String attributeName) } public static NestedSet nestedSet(RuleContext ruleContext, String attributeName) { + return nestedSet(ruleContext.getOwningPrerequisitesCollection(attributeName), attributeName); + } + + public static NestedSet nestedSet( + PrerequisitesCollection prerequisitesCollection, String attributeName) { NestedSetBuilder result = NestedSetBuilder.stableOrder(); - for (FileProvider target : ruleContext.getPrerequisites(attributeName, FileProvider.class)) { + for (FileProvider target : + prerequisitesCollection.getPrerequisites(attributeName, FileProvider.class)) { result.addTransitive(target.getFilesToBuild()); } return result.build(); 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 642d5c336ea348..4a611c93ef2118 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,7 +26,6 @@ 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; @@ -36,7 +35,6 @@ 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; @@ -44,82 +42,63 @@ /** Collection of the attributes dependencies available in a {@link RuleContext}. */ public final class PrerequisitesCollection { + private interface AttributesMapper { + Attribute getAttribute(String attributeName); + } + private final ImmutableSortedKeyListMultimap attributeToPrerequisitesMap; - private final AttributeMap attributes; + private final AttributesMapper attributesMapper; + private final RuleErrorConsumer ruleErrorConsumer; private final Rule rule; private final String ruleClassNameForLogging; - static PrerequisitesCollection create( - AttributeMap attributes, - ImmutableListMultimap prerequisitesMap, - ImmutableList aspects, + PrerequisitesCollection( + ImmutableSortedKeyListMultimap attributeToPrerequisitesMap, + ImmutableMap attributes, RuleErrorConsumer ruleErrorConsumer, Rule rule, String ruleClassNameForLogging) { - return new PrerequisitesCollection( - attributes, - mergePrerequisites(aspects, prerequisitesMap), + this( + attributeToPrerequisitesMap, + /* attributesMapper= */ attributes::get, ruleErrorConsumer, rule, ruleClassNameForLogging); } - /** - * Merges the rule prerequisites with the aspects prerequisites giving precedence to aspects - * prerequisites if any. - * - *

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 mergePrerequisites( - ImmutableList aspects, - ImmutableListMultimap targetsMap) { - - ImmutableSortedKeyListMultimap.Builder - attributeNameToTargetsMap = ImmutableSortedKeyListMultimap.builder(); - HashSet 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(); + PrerequisitesCollection( + ImmutableSortedKeyListMultimap attributeToPrerequisitesMap, + AttributeMap attributes, + RuleErrorConsumer ruleErrorConsumer, + Rule rule, + String ruleClassNameForLogging) { + this( + attributeToPrerequisitesMap, + /* attributesMapper= */ attributes::getAttributeDefinition, + ruleErrorConsumer, + rule, + ruleClassNameForLogging); } private PrerequisitesCollection( - AttributeMap attributes, - ImmutableSortedKeyListMultimap prerequisitesMap, + ImmutableSortedKeyListMultimap attributeToPrerequisitesMap, + AttributesMapper attributesMapper, RuleErrorConsumer ruleErrorConsumer, Rule rule, String ruleClassNameForLogging) { - this.attributes = attributes; - this.attributeToPrerequisitesMap = prerequisitesMap; + this.attributeToPrerequisitesMap = attributeToPrerequisitesMap; + this.attributesMapper = attributesMapper; this.ruleErrorConsumer = ruleErrorConsumer; this.rule = rule; this.ruleClassNameForLogging = ruleClassNameForLogging; } + boolean has(String attributeName) { + return attributesMapper.getAttribute(attributeName) != null; + } + /** Returns a list of all prerequisites as {@code ConfiguredTarget} objects. */ public ImmutableList getAllPrerequisites() { return attributeToPrerequisitesMap.values().stream() @@ -137,7 +116,7 @@ public List getPrerequisiteConfiguredTargets(String att * specified attribute. */ public List getPrerequisites(String attributeName) { - Attribute attribute = attributes.getAttributeDefinition(attributeName); + Attribute attribute = attributesMapper.getAttribute(attributeName); if (attribute == null) { return ImmutableList.of(); } @@ -301,7 +280,7 @@ private Artifact transitiveInfoCollectionToArtifact( */ @Nullable public FilesToRunProvider getExecutablePrerequisite(String attributeName) { - Attribute ruleDefinition = attributes.getAttributeDefinition(attributeName); + Attribute ruleDefinition = attributesMapper.getAttribute(attributeName); Preconditions.checkNotNull( ruleDefinition, "%s attribute %s is not defined", ruleClassNameForLogging, attributeName); @@ -366,7 +345,7 @@ ImmutableListMultimap getPrerequisitesByConfiguratio } private void checkAttributeIsDependency(String attributeName) { - Attribute attributeDefinition = attributes.getAttributeDefinition(attributeName); + Attribute attributeDefinition = attributesMapper.getAttribute(attributeName); Preconditions.checkNotNull( attributeDefinition, "%s: %s attribute %s is not defined", 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 dd8c19e8fb2da3..23a98e61787ac4 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 @@ -56,6 +56,7 @@ 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; @@ -119,9 +120,9 @@ * *

@see com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory * - *

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. + *

The class is intended to be sub-classed by {@link AspectContext}, 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 class RuleContext extends TargetContext implements ActionConstructionContext, ActionRegistry, RuleErrorConsumer, AutoCloseable { @@ -146,7 +147,14 @@ void validate( new Attribute.Builder<>(TOOLCHAIN_ATTR_NAME, BuildType.LABEL_LIST).build(); private final Rule rule; + + /** + * If this {@code RuleContext} is for rule evaluation, this holds the attribute-based + * prerequisites of the rule and if it is for aspect evaluation, it will contain the merged + * prerequisites of the rule and the base aspects (rule attributes take precedence). + */ private final PrerequisitesCollection prerequisitesCollection; + private final ImmutableMap configConditions; private final AttributeMap attributes; private final FeatureSet features; @@ -185,14 +193,11 @@ void validate( */ @Nullable private StarlarkRuleContext starlarkRuleContext; - /** - * The constructor is intentionally package private to be only used by {@link - * AspectOnRuleContext}. - */ + /** The constructor is intentionally package private to be only used by {@link AspectContext}. */ RuleContext( Builder builder, AttributeMap attributes, - ImmutableListMultimap targetMap, + PrerequisitesCollection prerequisitesCollection, ExecGroupCollection execGroupCollection) { super( builder.env, @@ -215,15 +220,32 @@ void validate( this.transitivePackagesForRunfileRepoMappingManifest = builder.transitivePackagesForRunfileRepoMappingManifest; this.starlarkThread = createStarlarkThread(builder.mutability); // uses above state + this.prerequisitesCollection = prerequisitesCollection; + } - this.prerequisitesCollection = - PrerequisitesCollection.create( - attributes, - targetMap, - builder.aspects, - this.reporter, - this.rule, - this.ruleClassNameForLogging); + static RuleContext create( + Builder builder, + AttributeMap ruleAttributes, + ImmutableListMultimap targetsMap, + ExecGroupCollection execGroupCollection) { + + ImmutableSortedKeyListMultimap.Builder attrNameToTargets = + ImmutableSortedKeyListMultimap.builder(); + for (Map.Entry> entry : + targetsMap.asMap().entrySet()) { + attrNameToTargets.putAll(entry.getKey().getAttribute().getName(), entry.getValue()); + } + + return new RuleContext( + builder, + ruleAttributes, + new PrerequisitesCollection( + attrNameToTargets.build(), + ruleAttributes, + builder.getErrorConsumer(), + builder.getRule(), + builder.getRuleClassNameForLogging()), + execGroupCollection); } private FeatureSet computeFeatures() { @@ -250,6 +272,28 @@ public boolean isAllowTagsPropagation() { .getBool(BuildLanguageOptions.INCOMPATIBLE_ALLOW_TAGS_PROPAGATION); } + /** + * If this {@code RuleContext} is for rule evaluation, returns the attribute-based prerequisites + * of the rule and if it is for aspect evaluation, it returns the merged prerequisites of the rule + * and the base aspects (rule attributes take precedence). + */ + public PrerequisitesCollection getRulePrerequisitesCollection() { + return prerequisitesCollection; + } + + /** + * Prerequisites lookup methods in {@code RuleContext} such as {@link + * RuleContext#getExecutablePrerequisite} use this method to find the {@code + * PrerquisitesCollection} owning an attribute with the given name. + * + *

For aspect evaluation, {@link AspectContext} overrides this to select the correct owning + * {@code PrerequisitesCollection} for the given {@code attributeName} whether it is owned by the + * main aspect or the underlying rule and base aspects. + */ + PrerequisitesCollection getOwningPrerequisitesCollection(String attributeName) { + return prerequisitesCollection; + } + public RepositoryName getRepository() { return rule.getRepository(); } @@ -372,7 +416,8 @@ public ImmutableList getAllPrerequisites() { /** Returns the {@link ConfiguredTargetAndData} the given attribute. */ public List getPrerequisiteConfiguredTargets(String attributeName) { - return prerequisitesCollection.getPrerequisiteConfiguredTargets(attributeName); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisiteConfiguredTargets(attributeName); } /** @@ -812,7 +857,7 @@ public boolean isAttrDefined(String attrName, Type type) { */ public Map, List> getSplitPrerequisites( String attributeName) { - return prerequisitesCollection.getSplitPrerequisites(attributeName); + return getOwningPrerequisitesCollection(attributeName).getSplitPrerequisites(attributeName); } /** @@ -822,7 +867,7 @@ public Map, List> getSplitPrerequisite @Nullable public C getPrerequisite( String attributeName, Class provider) { - return prerequisitesCollection.getPrerequisite(attributeName, provider); + return getOwningPrerequisitesCollection(attributeName).getPrerequisite(attributeName, provider); } /** @@ -831,7 +876,7 @@ public C getPrerequisite( */ @Nullable public TransitiveInfoCollection getPrerequisite(String attributeName) { - return prerequisitesCollection.getPrerequisite(attributeName); + return getOwningPrerequisitesCollection(attributeName).getPrerequisite(attributeName); } /** @@ -842,13 +887,14 @@ public TransitiveInfoCollection getPrerequisite(String attributeName) { @Nullable public T getPrerequisite( String attributeName, BuiltinProvider builtinProvider) { - return prerequisitesCollection.getPrerequisite(attributeName, builtinProvider); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisite(attributeName, builtinProvider); } @Nullable public T getPrerequisite(String attributeName, StarlarkProviderWrapper key) throws RuleErrorException { - return prerequisitesCollection.getPrerequisite(attributeName, key); + return getOwningPrerequisitesCollection(attributeName).getPrerequisite(attributeName, key); } /** @@ -859,7 +905,8 @@ public T getPrerequisite(String attributeName, StarlarkProviderWrapper ke public ImmutableListMultimap getPrerequisitesByConfiguration( String attributeName, BuiltinProvider provider) { - return prerequisitesCollection.getPrerequisitesByConfiguration(attributeName, provider); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisitesByConfiguration(attributeName, provider); } /** @@ -867,7 +914,7 @@ ImmutableListMultimap getPrerequisitesByConfiguratio * specified attribute. */ public List getPrerequisites(String attributeName) { - return prerequisitesCollection.getPrerequisites(attributeName); + return getOwningPrerequisitesCollection(attributeName).getPrerequisites(attributeName); } /** @@ -876,7 +923,8 @@ public List getPrerequisites(String attribut */ public List getPrerequisites( String attributeName, Class classType) { - return prerequisitesCollection.getPrerequisites(attributeName, classType); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisites(attributeName, classType); } /** @@ -885,7 +933,8 @@ public List getPrerequisites( */ public ImmutableList getPrerequisites( String attributeName, StarlarkProviderWrapper starlarkKey) throws RuleErrorException { - return prerequisitesCollection.getPrerequisites(attributeName, starlarkKey); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisites(attributeName, starlarkKey); } /** @@ -894,7 +943,8 @@ public ImmutableList getPrerequisites( */ public List getPrerequisites( String attributeName, BuiltinProvider starlarkKey) { - return prerequisitesCollection.getPrerequisites(attributeName, starlarkKey); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisites(attributeName, starlarkKey); } /** @@ -904,7 +954,8 @@ public List getPrerequisites( public Iterable getPrerequisitesIf( String attributeName, Class classType) { - return prerequisitesCollection.getPrerequisitesIf(attributeName, classType); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisitesIf(attributeName, classType); } /** @@ -913,7 +964,8 @@ Iterable getPrerequisitesIf( */ public Iterable getPrerequisitesIf( String attributeName, BuiltinProvider classType) { - return prerequisitesCollection.getPrerequisitesIf(attributeName, classType); + return getOwningPrerequisitesCollection(attributeName) + .getPrerequisitesIf(attributeName, classType); } /** @@ -925,7 +977,7 @@ public Iterable getPrerequi */ @Nullable public FilesToRunProvider getExecutablePrerequisite(String attributeName) { - return prerequisitesCollection.getExecutablePrerequisite(attributeName); + return getOwningPrerequisitesCollection(attributeName).getExecutablePrerequisite(attributeName); } public void initConfigurationMakeVariableContext( @@ -1541,9 +1593,9 @@ private RuleContext build(boolean attributeChecks) throws InvalidExecGroupExcept ExecGroupCollection execGroupCollection = createExecGroupCollection(execGroupCollectionBuilder, ruleAttributes); if (aspects.isEmpty()) { - return new RuleContext(this, ruleAttributes, targetMap, execGroupCollection); + return RuleContext.create(this, ruleAttributes, targetMap, execGroupCollection); } else { - return new AspectOnRuleContext(this, ruleAttributes, targetMap, execGroupCollection); + return AspectContext.create(this, ruleAttributes, targetMap, execGroupCollection); } } @@ -1844,7 +1896,7 @@ public StarlarkSemantics getStarlarkSemantics() { /** * Returns a rule class name suitable for log messages, including an aspect name if applicable. */ - private String getRuleClassNameForLogging() { + String getRuleClassNameForLogging() { if (aspects.isEmpty()) { return target.getAssociatedRule().getRuleClass(); } @@ -1855,6 +1907,14 @@ private String getRuleClassNameForLogging() { + target.getAssociatedRule().getRuleClass(); } + RuleErrorConsumer getErrorConsumer() { + return reporter; + } + + boolean separateAspectDeps() { + return env.getStarlarkSemantics().getBool(BuildLanguageOptions.SEPARATE_ASPECT_DEPS); + } + public BuildConfigurationValue getConfiguration() { return configuration; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java index 1cdcd516dfbee3..0166e5853cc37e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttributesCollection.java @@ -18,6 +18,7 @@ import com.google.devtools.build.lib.analysis.AliasProvider; import com.google.devtools.build.lib.analysis.FilesToRunProvider; import com.google.devtools.build.lib.analysis.PrerequisiteArtifacts; +import com.google.devtools.build.lib.analysis.PrerequisitesCollection; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -129,12 +130,15 @@ public void repr(Printer printer) { printer.append(""); } - public static Builder builder(StarlarkRuleContext ruleContext) { - return new Builder(ruleContext); + public static Builder builder( + StarlarkRuleContext ruleContext, PrerequisitesCollection prerequisitesCollection) { + return new Builder(ruleContext, prerequisitesCollection); } public static class Builder { private final StarlarkRuleContext context; + private final PrerequisitesCollection prerequisites; + private final LinkedHashMap attrBuilder = new LinkedHashMap<>(); private final LinkedHashMap executableBuilder = new LinkedHashMap<>(); private final ImmutableMap.Builder executableRunfilesbuilder = @@ -143,8 +147,10 @@ public static class Builder { private final LinkedHashMap filesBuilder = new LinkedHashMap<>(); private final HashSet seenExecutables = new HashSet<>(); - private Builder(StarlarkRuleContext ruleContext) { + private Builder( + StarlarkRuleContext ruleContext, PrerequisitesCollection prerequisitesCollection) { this.context = ruleContext; + this.prerequisites = prerequisitesCollection; } public void addAttribute(Attribute a, Object val) { @@ -183,8 +189,7 @@ public void addAttribute(Attribute a, Object val) { // for a native rule for builtins injection, in which case we may see an executable // LABEL_LIST. In that case omit the attribute as if it weren't executable. if (type == BuildType.LABEL) { - FilesToRunProvider provider = - context.getRuleContext().getExecutablePrerequisite(a.getName()); + FilesToRunProvider provider = prerequisites.getExecutablePrerequisite(a.getName()); if (provider != null && provider.getExecutable() != null) { Artifact executable = provider.getExecutable(); executableBuilder.put(skyname, executable); @@ -205,34 +210,33 @@ public void addAttribute(Attribute a, Object val) { } if (a.isSingleArtifact()) { // In Starlark only label (not label list) type attributes can have the SingleArtifact flag. - Artifact artifact = context.getRuleContext().getPrerequisiteArtifact(a.getName()); + Artifact artifact = prerequisites.getPrerequisiteArtifact(a.getName()); if (artifact != null) { fileBuilder.put(skyname, artifact); } else { fileBuilder.put(skyname, Starlark.NONE); } } - NestedSet files = - PrerequisiteArtifacts.nestedSet(context.getRuleContext(), a.getName()); + NestedSet files = PrerequisiteArtifacts.nestedSet(prerequisites, a.getName()); filesBuilder.put( skyname, files.isEmpty() ? StarlarkList.empty() : StarlarkList.lazyImmutable(files::toList)); if (type == BuildType.LABEL && !a.getTransitionFactory().isSplit()) { - Object prereq = context.getRuleContext().getPrerequisite(a.getName()); + Object prereq = prerequisites.getPrerequisite(a.getName()); if (prereq == null) { prereq = Starlark.NONE; } attrBuilder.put(skyname, prereq); } else if (type == BuildType.LABEL_LIST || (type == BuildType.LABEL && a.getTransitionFactory().isSplit())) { - List allPrereq = context.getRuleContext().getPrerequisites(a.getName()); + List allPrereq = prerequisites.getPrerequisites(a.getName()); attrBuilder.put(skyname, StarlarkList.immutableCopyOf(allPrereq)); } else if (type == BuildType.LABEL_KEYED_STRING_DICT) { Dict.Builder builder = Dict.builder(); Map original = BuildType.LABEL_KEYED_STRING_DICT.cast(val); List allPrereq = - context.getRuleContext().getPrerequisites(a.getName()); + prerequisites.getPrerequisites(a.getName()); for (TransitiveInfoCollection prereq : allPrereq) { builder.put(prereq, original.get(AliasProvider.getDependencyLabel(prereq))); } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java index 305742817d873a..fde561439dc9bf 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleContext.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.analysis.ActionsProvider; import com.google.devtools.build.lib.analysis.AliasProvider; +import com.google.devtools.build.lib.analysis.AspectContext; import com.google.devtools.build.lib.analysis.BashCommandConstructor; import com.google.devtools.build.lib.analysis.CommandHelper; import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext; @@ -269,7 +270,8 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a this.outputsObject = outputs; // Populate ctx.attr. - StarlarkAttributesCollection.Builder builder = StarlarkAttributesCollection.builder(this); + StarlarkAttributesCollection.Builder builder = + StarlarkAttributesCollection.builder(this, ruleContext.getRulePrerequisitesCollection()); for (Attribute attribute : attributes) { Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType()); builder.addAttribute(attribute, value); @@ -284,7 +286,8 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a ruleContext.getMainAspect().getDefinition().getAttributes().values(); StarlarkAttributesCollection.Builder aspectBuilder = - StarlarkAttributesCollection.builder(this); + StarlarkAttributesCollection.builder( + this, ((AspectContext) ruleContext).getMainAspectPrerequisitesCollection()); for (Attribute attribute : attributes) { Object defaultValue = attribute.getDefaultValue(null); if (defaultValue instanceof ComputedDefault) { @@ -295,7 +298,8 @@ public StarlarkRuleContext(RuleContext ruleContext, @Nullable AspectDescriptor a this.attributesCollection = aspectBuilder.build(); this.splitAttributes = null; - StarlarkAttributesCollection.Builder ruleBuilder = StarlarkAttributesCollection.builder(this); + StarlarkAttributesCollection.Builder ruleBuilder = + StarlarkAttributesCollection.builder(this, ruleContext.getRulePrerequisitesCollection()); for (Attribute attribute : rule.getAttributes()) { Object value = ruleContext.attributes().get(attribute.getName(), attribute.getType()); 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 15872a959b3a54..33097e36e90a8e 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 @@ -690,6 +690,20 @@ public final class BuildLanguageOptions extends OptionsBase { help = "Enable experimental rule extension API and subrule APIs") public boolean experimentalRuleExtensionApi; + @Option( + name = "separate_aspect_deps", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + help = + "If enabled, the dependencies of the main aspect in an aspect path will be" + + " separated from those of the target and the base aspects. ctx.attr.{attr_name}" + + " will always get the attribute value from the main aspect and" + + " ctx.rule.attr.{attr_name} will get the value from the rule if it has an attribute" + + " with that name or from the base aspects attributes (first one in" + + " the aspects path wins).") + public boolean separateAspectDeps; + /** * 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 @@ -792,6 +806,7 @@ public StarlarkSemantics toStarlarkSemantics() { INCOMPATIBLE_DISABLE_NON_EXECUTABLE_JAVA_BINARY, incompatibleDisableNonExecutableJavaBinary) .setBool(EXPERIMENTAL_RULE_EXTENSION_API, experimentalRuleExtensionApi) + .setBool(SEPARATE_ASPECT_DEPS, separateAspectDeps) .build(); return INTERNER.intern(semantics); } @@ -884,6 +899,7 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String INCOMPATIBLE_DISABLE_NON_EXECUTABLE_JAVA_BINARY = "-incompatible_disable_non_executable_java_binary"; public static final String EXPERIMENTAL_RULE_EXTENSION_API = "-experimental_rule_extension_api"; + public static final String SEPARATE_ASPECT_DEPS = "-separate_aspect_deps"; // non-booleans public static final StarlarkSemantics.Key EXPERIMENTAL_BUILTINS_BZL_PATH = 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 2a48601449afe1..0a6dc9d06d1f29 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 @@ -1031,7 +1031,7 @@ public void aspectWithExtraAttributeApplyToFilesAndNot_outputFile_onlyApplyToFil } @Test - public void aspectWithExtraAttributeDependsOnNotApplicable_usesAttributeFromDependentAspect() + public void aspectWithExtraAttributeDependsOnNotApplicable_usesItsOwnAttribute() throws Exception { ExtraAttributeAspect aspectApplies = new ExtraAttributeAspect( @@ -1043,6 +1043,7 @@ public void aspectWithExtraAttributeDependsOnNotApplicable_usesAttributeFromDepe ImmutableList.of(TestAspects.BASE_RULE, TestAspects.SIMPLE_RULE)); scratch.file("extra/BUILD", "simple(name='extra')", "simple(name='extra2')"); scratch.file("a/BUILD", "genrule(name='gen_a', outs=['a'], cmd='touch $@')"); + useConfiguration("--separate_aspect_deps"); AnalysisResult analysisResult = update( @@ -1055,7 +1056,7 @@ public void aspectWithExtraAttributeDependsOnNotApplicable_usesAttributeFromDepe ExtraAttributeAspect.Provider provider = getAspectByName(analysisResult.getAspectsMap(), aspectApplies.getName()) .getProvider(ExtraAttributeAspect.Provider.class); - assertThat(provider.label()).isEqualTo("//extra:extra2"); + assertThat(provider.label()).isEqualTo("//extra:extra"); assertThat( getAspectByName(analysisResult.getAspectsMap(), aspectDoesNotApply.getName()) .getProviders() diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java index c654ae39454f0d..97b1d9f96c758f 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/TestAspects.java @@ -26,6 +26,7 @@ import com.google.common.collect.Iterables; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; +import com.google.devtools.build.lib.analysis.AspectContext; import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -145,7 +146,9 @@ private static NestedSet collectAspectData(String me, RuleContext ruleCo continue; } Iterable prerequisites = - ruleContext.getPrerequisites(attributeName, AspectInfo.class); + ruleContext + .getRulePrerequisitesCollection() + .getPrerequisites(attributeName, AspectInfo.class); for (AspectInfo prerequisite : prerequisites) { result.addTransitive(prerequisite.getData()); } @@ -205,8 +208,10 @@ public static class MultiAspectRuleFactory implements RuleConfiguredTargetFactor @Override public ConfiguredTarget create(RuleContext ruleContext) throws InterruptedException, RuleErrorException, ActionConflictException { - TransitiveInfoCollection fooAttribute = ruleContext.getPrerequisite("foo"); - TransitiveInfoCollection barAttribute = ruleContext.getPrerequisite("bar"); + TransitiveInfoCollection fooAttribute = + ruleContext.getRulePrerequisitesCollection().getPrerequisite("foo"); + TransitiveInfoCollection barAttribute = + ruleContext.getRulePrerequisitesCollection().getPrerequisite("bar"); NestedSetBuilder infoBuilder = NestedSetBuilder.stableOrder(); @@ -389,7 +394,10 @@ public ConfiguredAspect create( AspectParameters parameters, RepositoryName toolsRepository) throws ActionConflictException, InterruptedException { - TransitiveInfoCollection dep = ruleContext.getPrerequisite("$dep"); + TransitiveInfoCollection dep = + ((AspectContext) ruleContext) + .getMainAspectPrerequisitesCollection() + .getPrerequisite("$dep"); if (dep == null) { ruleContext.attributeError("$dep", "$dep attribute not resolved"); return ConfiguredAspect.builder(ruleContext).build(); @@ -641,7 +649,10 @@ public ConfiguredAspect create( information.append(" data " + Iterables.getFirst(parameters.getAttribute("baz"), null)); information.append(" "); } - List deps = ruleContext.getPrerequisites("$dep"); + List deps = + ((AspectContext) ruleContext) + .getMainAspectPrerequisitesCollection() + .getPrerequisites("$dep"); information.append("$dep:["); for (TransitiveInfoCollection dep : deps) { information.append(" "); 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 e64dd68d9b46c3..4fe5853dc4ea41 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 @@ -64,7 +64,6 @@ import net.starlark.java.eval.Starlark; import net.starlark.java.eval.StarlarkInt; import net.starlark.java.eval.StarlarkList; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -2810,7 +2809,7 @@ public void aspectOnAspectAttrConflict() throws Exception { "def _r2_impl(ctx):", " return struct(result = ctx.attr.dep[MyInfo].hidden_attr_label)", "r1 = rule(_r1_impl, attrs = { 'dep' : attr.label(aspects = [a1])})", - "r2 = rule(_r2_impl, attrs = { 'dep' : attr.label(aspects = [a2])})"); + "r2 = rule(_r2_impl, attrs = { 'dep' : attr.label(aspects = [a1, a2])})"); scratch.file( "test/BUILD", "load(':aspect.bzl', 'r1', 'r2')", @@ -2823,6 +2822,8 @@ public void aspectOnAspectAttrConflict() throws Exception { "cc_library(", " name = 'zzz',", ")"); + useConfiguration("--separate_aspect_deps"); + AnalysisResult analysisResult = update("//test:r2"); ConfiguredTarget target = Iterables.getOnlyElement(analysisResult.getTargetsToBuild()); String result = (String) target.get("result"); @@ -6618,31 +6619,56 @@ public void testTopLevelAspectRequiresAspect_withRequiredAspectProvidersNotFound .isEqualTo("aspect_b on target @//test:main_target cannot find prov_b"); } - @Ignore("TODO(b/206127051): Fix the crash, rename the test and add a check the error message.") @Test public void testDependentAspectWithNonExecutableTool_doesNotCrash() throws Exception { scratch.file("test/BUILD", "sh_binary(name='bin', srcs=['bin.sh'])", "sh_library(name='lib')"); scratch.file( "test/defs.bzl", - "AInfo = provider(fields={})", - "def _aspect_a(target, ctx): return AInfo()", + "AInfo = provider()", + "BInfo = provider()", + "def _aspect_a(target, ctx):", + " return [AInfo(value=str(ctx.attr._attr.label))]", "aspect_a = aspect(", " implementation = _aspect_a,", " provides=[AInfo],", " attrs = {'_attr':" + " attr.label(default=':lib')},", ")", "def _aspect_b(target, ctx):", - " print(str(ctx.executable._attr))", - " return []", + " return [BInfo(value=str(ctx.executable._attr.path.split('/')[-1]))]", "aspect_b = aspect(", " implementation = _aspect_b,", " required_aspect_providers = [AInfo],", " attrs = {'_attr': attr.label(default=':bin', executable=True, cfg='exec')},", ")"); scratch.file("test/bin.sh").setExecutable(true); + useConfiguration("--separate_aspect_deps"); + + AnalysisResult result = + update(ImmutableList.of("test/defs.bzl%aspect_a", "test/defs.bzl%aspect_b"), "//test:bin"); + + ConfiguredAspect aspectB = + result.getAspectsMap().entrySet().stream() + .filter(a -> a.getKey().getAspectName().endsWith("aspect_b")) + .map(Map.Entry::getValue) + .findFirst() + .orElse(null); + assertThat(aspectB).isNotNull(); + + StarlarkProvider.Key provB = + new StarlarkProvider.Key(Label.parseCanonical("//test:defs.bzl"), "BInfo"); + assertThat(((StructImpl) aspectB.get(provB)).getValue("value")).isEqualTo("bin"); + + ConfiguredAspect aspectA = + result.getAspectsMap().entrySet().stream() + .filter(a -> a.getKey().getAspectName().endsWith("aspect_a")) + .map(Map.Entry::getValue) + .findFirst() + .orElse(null); + assertThat(aspectA).isNotNull(); - // TODO(b/206127051): This currently crashes with an IllegalStateException. - update(ImmutableList.of("test/defs.bzl%aspect_a", "test/defs.bzl%aspect_b"), "//test:bin"); + StarlarkProvider.Key provA = + new StarlarkProvider.Key(Label.parseCanonical("//test:defs.bzl"), "AInfo"); + assertThat(((StructImpl) aspectA.get(provA)).getValue("value")).isEqualTo("@//test:lib"); } @Test diff --git a/src/test/shell/integration/aspect_test.sh b/src/test/shell/integration/aspect_test.sh index a3d4f6ff5b4a24..915b33e89cb5ea 100755 --- a/src/test/shell/integration/aspect_test.sh +++ b/src/test/shell/integration/aspect_test.sh @@ -1675,22 +1675,29 @@ EOF 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" < $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_c on target @@\?//test:t1: aspect_c _tool=@@\?//test:aspect_c_tool, rule _tool=@@\?//test:r1_tool" + expect_log "aspect_c on target @@\?//test:t1: _tool_c=@@\?//test:aspect_c_diff_tool, _tool_r1=@@\?//test:r1_diff_tool" + + expect_log "aspect_b on target @@\?//test:t1: aspect_b _tool=@@\?//test:aspect_b_tool, rule _tool=@@\?//test:r1_tool" + expect_log "aspect_b on target @@\?//test:t1: _tool_b=@@\?//test:aspect_b_diff_tool, _tool_r1=@@\?//test:r1_diff_tool, _tool_c=@@\?//test:aspect_c_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" + # in aspect_a, `ctx.attr._tool` gets its value from the main aspect (aspect_a) + # `ctx.rule.attr._tool` gets its value from the rule (r1) attribute with that name + # `ctx.rule.attr._base_aspects_tool` is not there in the rule (r1) attributes so it + # gets its value from the first aspect that has it in the aspects path + # which is aspect_c. + expect_log "aspect_a on target @@\?//test:t1: aspect_a _tool=@@\?//test:aspect_a_tool and merged_rule_and_base_aspects _tool=@@\?//test:r1_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, _tool_c=@@\?//test:aspect_c_diff_tool" + expect_log "aspect_a on target @@\?//test:t1: _base_aspects_tool=@@\?//test:aspect_c_tool" }