diff options
Diffstat (limited to 'src/main/java/com')
14 files changed, 253 insertions, 153 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java index 0018525784..6427014a85 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectCollection.java @@ -213,6 +213,10 @@ public final class AspectCollection { } } + public static AspectCollection createForTests(AspectDescriptor... descriptors) { + return createForTests(ImmutableSet.copyOf(descriptors)); + } + public static AspectCollection createForTests(ImmutableSet<AspectDescriptor> descriptors) { ImmutableSet.Builder<AspectDeps> depsBuilder = ImmutableSet.builder(); for (AspectDescriptor descriptor : descriptors) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java index 19e972c4fe..10a9216ae5 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java @@ -857,7 +857,7 @@ public class BuildView { targetAndConfig.getLabel(), Attribute.ConfigurationTransition.NONE, // TODO(bazel-team): support top-level aspects - ImmutableSet.<AspectDescriptor>of())); + AspectCollection.EMPTY)); } } @@ -1003,7 +1003,7 @@ public class BuildView { Iterable<BuildOptions> buildOptions) { Preconditions.checkArgument(ct.getConfiguration().fragmentClasses().equals(fragments)); Dependency asDep = Dependency.withTransitionAndAspects(ct.getLabel(), - Attribute.ConfigurationTransition.NONE, ImmutableSet.<AspectDescriptor>of()); + Attribute.ConfigurationTransition.NONE, AspectCollection.EMPTY); ImmutableList.Builder<BuildConfiguration> builder = ImmutableList.builder(); for (BuildOptions options : buildOptions) { builder.add(Iterables.getOnlyElement( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java b/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java index d72bced27a..9b67fe8583 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Dependency.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.analysis; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.AspectDescriptor; @@ -62,7 +61,9 @@ public abstract class Dependency { */ public static Dependency withConfiguration(Label label, BuildConfiguration configuration) { return new StaticConfigurationDependency( - label, configuration, ImmutableMap.<AspectDescriptor, BuildConfiguration>of()); + label, configuration, + AspectCollection.EMPTY, + ImmutableMap.<AspectDescriptor, BuildConfiguration>of()); } /** @@ -72,13 +73,13 @@ public abstract class Dependency { * <p>The configuration and aspects must not be {@code null}. */ public static Dependency withConfigurationAndAspects( - Label label, BuildConfiguration configuration, Iterable<AspectDescriptor> aspects) { + Label label, BuildConfiguration configuration, AspectCollection aspects) { ImmutableMap.Builder<AspectDescriptor, BuildConfiguration> aspectBuilder = new ImmutableMap.Builder<>(); - for (AspectDescriptor aspect : aspects) { + for (AspectDescriptor aspect : aspects.getAllAspects()) { aspectBuilder.put(aspect, configuration); } - return new StaticConfigurationDependency(label, configuration, aspectBuilder.build()); + return new StaticConfigurationDependency(label, configuration, aspects, aspectBuilder.build()); } /** @@ -90,9 +91,10 @@ public abstract class Dependency { */ public static Dependency withConfiguredAspects( Label label, BuildConfiguration configuration, + AspectCollection aspects, Map<AspectDescriptor, BuildConfiguration> aspectConfigurations) { return new StaticConfigurationDependency( - label, configuration, ImmutableMap.copyOf(aspectConfigurations)); + label, configuration, aspects, ImmutableMap.copyOf(aspectConfigurations)); } /** @@ -100,8 +102,8 @@ public abstract class Dependency { * configuration builds. */ public static Dependency withTransitionAndAspects( - Label label, Attribute.Transition transition, Iterable<AspectDescriptor> aspects) { - return new DynamicConfigurationDependency(label, transition, ImmutableSet.copyOf(aspects)); + Label label, Attribute.Transition transition, AspectCollection aspects) { + return new DynamicConfigurationDependency(label, transition, aspects); } protected final Label label; @@ -144,18 +146,16 @@ public abstract class Dependency { * Returns the set of aspects which should be evaluated and combined with the configured target * pointed to by this dependency. * - * @see #getAspectConfigurations() + * @see #getAspectConfiguration(AspectDescriptor) */ - public abstract ImmutableSet<AspectDescriptor> getAspects(); + public abstract AspectCollection getAspects(); /** - * Returns the mapping from aspects to the static configurations they should be evaluated with. - * - * <p>The {@link Map#keySet()} of this map is equal to that returned by {@link #getAspects()}. - * + * Returns the the static configuration an aspect should be evaluated with + ** * @throws IllegalStateException if {@link #hasStaticConfiguration()} returns false. */ - public abstract ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations(); + public abstract BuildConfiguration getAspectConfiguration(AspectDescriptor aspect); /** * Implementation of a dependency with no configuration, suitable for static configuration @@ -184,13 +184,13 @@ public abstract class Dependency { } @Override - public ImmutableSet<AspectDescriptor> getAspects() { - return ImmutableSet.of(); + public AspectCollection getAspects() { + return AspectCollection.EMPTY; } @Override - public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() { - return ImmutableMap.of(); + public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) { + return null; } @Override @@ -219,14 +219,17 @@ public abstract class Dependency { */ private static final class StaticConfigurationDependency extends Dependency { private final BuildConfiguration configuration; + private final AspectCollection aspects; private final ImmutableMap<AspectDescriptor, BuildConfiguration> aspectConfigurations; public StaticConfigurationDependency( Label label, BuildConfiguration configuration, - ImmutableMap<AspectDescriptor, BuildConfiguration> aspects) { + AspectCollection aspects, + ImmutableMap<AspectDescriptor, BuildConfiguration> aspectConfigurations) { super(label); this.configuration = Preconditions.checkNotNull(configuration); - this.aspectConfigurations = Preconditions.checkNotNull(aspects); + this.aspects = Preconditions.checkNotNull(aspects); + this.aspectConfigurations = Preconditions.checkNotNull(aspectConfigurations); } @Override @@ -246,13 +249,13 @@ public abstract class Dependency { } @Override - public ImmutableSet<AspectDescriptor> getAspects() { - return aspectConfigurations.keySet(); + public AspectCollection getAspects() { + return aspects; } @Override - public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() { - return aspectConfigurations; + public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) { + return aspectConfigurations.get(aspect); } @Override @@ -268,6 +271,7 @@ public abstract class Dependency { StaticConfigurationDependency otherDep = (StaticConfigurationDependency) other; return label.equals(otherDep.label) && configuration.equals(otherDep.configuration) + && aspects.equals(otherDep.aspects) && aspectConfigurations.equals(otherDep.aspectConfigurations); } @@ -285,10 +289,10 @@ public abstract class Dependency { */ private static final class DynamicConfigurationDependency extends Dependency { private final Attribute.Transition transition; - private final ImmutableSet<AspectDescriptor> aspects; + private final AspectCollection aspects; public DynamicConfigurationDependency( - Label label, Attribute.Transition transition, ImmutableSet<AspectDescriptor> aspects) { + Label label, Attribute.Transition transition, AspectCollection aspects) { super(label); this.transition = Preconditions.checkNotNull(transition); this.aspects = Preconditions.checkNotNull(aspects); @@ -311,15 +315,15 @@ public abstract class Dependency { } @Override - public ImmutableSet<AspectDescriptor> getAspects() { + public AspectCollection getAspects() { return aspects; } @Override - public ImmutableMap<AspectDescriptor, BuildConfiguration> getAspectConfigurations() { + public BuildConfiguration getAspectConfiguration(AspectDescriptor aspect) { throw new IllegalStateException( "A dependency with a dynamic configuration transition does not have aspect " - + "configurations."); + + "configurations."); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index dc1f29278c..06d561bf48 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -18,6 +18,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.devtools.build.lib.analysis.AspectCollection.AspectCycleOnPathException; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildOptions; import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; @@ -520,36 +521,86 @@ public abstract class DependencyResolver { } - private static ImmutableSet<AspectDescriptor> requiredAspects( - Iterable<Aspect> aspects, + private static AspectCollection requiredAspects( + Iterable<Aspect> aspectPath, AttributeAndOwner attributeAndOwner, final Target target, Rule originalRule) { if (!(target instanceof Rule)) { - return ImmutableSet.of(); + return AspectCollection.EMPTY; } if (attributeAndOwner.ownerAspect != null) { // Do not propagate aspects along aspect attributes. - return ImmutableSet.of(); + return AspectCollection.EMPTY; } - Iterable<Aspect> aspectCandidates = - extractAspectCandidates(aspects, attributeAndOwner.attribute, originalRule); - RuleClass ruleClass = ((Rule) target).getRuleClassObject(); - ImmutableSet.Builder<AspectDescriptor> result = ImmutableSet.builder(); + ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder(); + ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder(); + + collectOriginatingAspects(originalRule, attributeAndOwner.attribute, (Rule) target, + filteredAspectPath, visibleAspects); + + collectPropagatingAspects(aspectPath, + attributeAndOwner.attribute, + (Rule) target, filteredAspectPath, visibleAspects); + try { + return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build()); + } catch (AspectCycleOnPathException e) { + // TODO(dslomov): propagate the error and report to user. + throw new AssertionError(e); + } + } - for (Aspect aspectCandidate : aspectCandidates) { - if (aspectCandidate.getDefinition() - .getRequiredProviders() + /** + * Collects into {@code filteredAspectPath} + * aspects from {@code aspectPath} that propagate along {@code attribute} + * and apply to a given {@code target}. + * + * The last aspect in {@code aspectPath} is (potentially) visible and recorded + * in {@code visibleAspects}. + */ + private static void collectPropagatingAspects(Iterable<Aspect> aspectPath, + Attribute attribute, Rule target, + ImmutableList.Builder<Aspect> filteredAspectPath, + ImmutableSet.Builder<AspectDescriptor> visibleAspects) { + + Aspect lastAspect = null; + for (Aspect aspect : aspectPath) { + lastAspect = aspect; + if (aspect.getDefinition().propagateAlong(attribute) + && aspect.getDefinition().getRequiredProviders() + .isSatisfiedBy(target.getRuleClassObject().getAdvertisedProviders())) { + filteredAspectPath.add(aspect); + } else { + lastAspect = null; + } + } + + if (lastAspect != null) { + visibleAspects.add(lastAspect.getDescriptor()); + } + } + + /** + * Collect all aspects that originate on {@code attribute} of {@code originalRule} + * and are applicable to a {@code target} + * + * They are appended to {@code filteredAspectPath} and registered in {@code visibleAspects} set. + */ + private static void collectOriginatingAspects( + Rule originalRule, Attribute attribute, Rule target, + ImmutableList.Builder<Aspect> filteredAspectPath, + ImmutableSet.Builder<AspectDescriptor> visibleAspects) { + ImmutableList<Aspect> baseAspects = attribute.getAspects(originalRule); + RuleClass ruleClass = target.getRuleClassObject(); + for (Aspect baseAspect : baseAspects) { + if (baseAspect.getDefinition().getRequiredProviders() .isSatisfiedBy(ruleClass.getAdvertisedProviders())) { - result.add( - new AspectDescriptor( - aspectCandidate.getAspectClass(), - aspectCandidate.getParameters())); + filteredAspectPath.add(baseAspect); + visibleAspects.add(baseAspect.getDescriptor()); } } - return result.build(); } private static Iterable<Aspect> extractAspectCandidates( @@ -686,7 +737,7 @@ public abstract class DependencyResolver { applyNullTransition = true; } - ImmutableSet<AspectDescriptor> aspects = + AspectCollection aspects = requiredAspects(this.aspects, attributeAndOwner, toTarget, rule); Dependency dep; if (config.useDynamicConfigurations() && !applyNullTransition) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 5823a52d81..b16a4a6bd4 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -34,6 +34,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.Multimap; import com.google.common.collect.MutableClassToInstanceMap; import com.google.devtools.build.lib.actions.Root; +import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.Dependency; @@ -45,7 +46,6 @@ import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; -import com.google.devtools.build.lib.packages.AspectDescriptor; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Attribute.Configurator; @@ -1612,7 +1612,7 @@ public final class BuildConfiguration { * for each configuration represented by this instance. * TODO(bazel-team): this is a really ugly reverse dependency: factor this away. */ - Iterable<Dependency> getDependencies(Label label, ImmutableSet<AspectDescriptor> aspects); + Iterable<Dependency> getDependencies(Label label, AspectCollection aspects); } /** @@ -1706,7 +1706,7 @@ public final class BuildConfiguration { @Override public Iterable<Dependency> getDependencies( - Label label, ImmutableSet<AspectDescriptor> aspects) { + Label label, AspectCollection aspects) { ImmutableList.Builder<Dependency> deps = ImmutableList.builder(); for (BuildConfiguration config : toConfigurations) { deps.add(config != null @@ -1829,7 +1829,7 @@ public final class BuildConfiguration { @Override public Iterable<Dependency> getDependencies( - Label label, ImmutableSet<AspectDescriptor> aspects) { + Label label, AspectCollection aspects) { return ImmutableList.of( isNull() // We can trivially set the final value for null-configured targets now. This saves diff --git a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java index a305e515d2..8cc374f212 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java +++ b/src/main/java/com/google/devtools/build/lib/packages/AspectDefinition.java @@ -280,10 +280,8 @@ public final class AspectDefinition { } public Builder requireAspectsWithNativeProviders( - Iterable<ImmutableSet<SkylarkProviderIdentifier>> providerSets) { - for (ImmutableSet<SkylarkProviderIdentifier> providerSet : providerSets) { - requiredAspectProviders.addSkylarkSet(providerSet); - } + Class<?>... providers) { + requiredAspectProviders.addNativeSet(ImmutableSet.copyOf(providers)); return this; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java index 5518692436..ebd734bb45 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/android/DexArchiveAspect.java @@ -136,7 +136,8 @@ public final class DexArchiveAspect extends NativeAspectClass implements Configu .allowedRuleClasses("android_sdk", "filegroup") .value(new AndroidRuleClasses.AndroidSdkLabel( Label.parseAbsoluteUnchecked(toolsRepository + AndroidRuleClasses.DEFAULT_SDK)))) - .requiresConfigurationFragments(AndroidConfiguration.class); + .requiresConfigurationFragments(AndroidConfiguration.class) + .requireAspectsWithNativeProviders(JavaCompilationArgsAspectProvider.class); if (TriState.valueOf(params.getOnlyValueOfAttribute("incremental_dexing")) != TriState.NO) { // Marginally improves "query2" precision for targets that disable incremental dexing result.add(attr(ASPECT_DEXBUILDER_PREREQ, LABEL).cfg(HOST).exec() diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiProvider.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiProvider.java index 591057e4ac..5e89e5b14b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaSkylarkApiProvider.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; import com.google.devtools.build.lib.rules.SkylarkApiProvider; import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable; import com.google.devtools.build.lib.skylarkinterface.SkylarkModule; @@ -43,7 +44,8 @@ public final class JavaSkylarkApiProvider extends SkylarkApiProvider { /** The name of the field in Skylark used to access this class. */ public static final String NAME = "java"; /** The name of the field in Skylark proto aspects used to access this class. */ - public static final String PROTO_NAME = "proto_java"; + public static final SkylarkProviderIdentifier PROTO_NAME = + SkylarkProviderIdentifier.forLegacy("proto_java"); private final JavaRuleOutputJarsProvider ruleOutputJarsProvider; @Nullable private final JavaSourceJarsProvider sourceJarsProvider; diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java index a151b3db54..9eef47a53c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaLiteProtoAspect.java @@ -118,6 +118,8 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured .propagateAlongAttribute("deps") .requiresConfigurationFragments(JavaConfiguration.class, ProtoConfiguration.class) .requireProviders(ProtoSourcesProvider.class) + .advertiseProvider(JavaCompilationArgsAspectProvider.class) + .advertiseProvider(ImmutableList.of(JavaSkylarkApiProvider.PROTO_NAME)) .add( attr(PROTO_TOOLCHAIN_ATTR, LABEL) .mandatoryNativeProviders( @@ -218,7 +220,8 @@ public class JavaLiteProtoAspect extends NativeAspectClass implements Configured skylarkApiProvider.setCompilationArgsProvider(generatedCompilationArgsProvider); aspect - .addSkylarkTransitiveInfo(JavaSkylarkApiProvider.PROTO_NAME, skylarkApiProvider.build()) + .addSkylarkTransitiveInfo( + JavaSkylarkApiProvider.PROTO_NAME.getLegacyId(), skylarkApiProvider.build()) .addProviders( new JavaProtoLibraryTransitiveFilesToBuildProvider(transitiveOutputJars.build()), new JavaCompilationArgsAspectProvider(generatedCompilationArgsProvider)); diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java index a248bbf7b6..e702f27fc9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/proto/JavaProtoAspect.java @@ -125,6 +125,8 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe .propagateAlongAttribute("deps") .requiresConfigurationFragments(JavaConfiguration.class, ProtoConfiguration.class) .requireProviders(ProtoSourcesProvider.class) + .advertiseProvider(JavaCompilationArgsAspectProvider.class) + .advertiseProvider(ImmutableList.of(JavaSkylarkApiProvider.PROTO_NAME)) .add( attr(SPEED_PROTO_TOOLCHAIN_ATTR, LABEL) // TODO(carmi): reinstate mandatoryNativeProviders(ProtoLangToolchainProvider) @@ -230,7 +232,8 @@ public class JavaProtoAspect extends NativeAspectClass implements ConfiguredAspe skylarkApiProvider.setCompilationArgsProvider(generatedCompilationArgsProvider); aspect - .addSkylarkTransitiveInfo(JavaSkylarkApiProvider.PROTO_NAME, skylarkApiProvider.build()) + .addSkylarkTransitiveInfo( + JavaSkylarkApiProvider.PROTO_NAME.getLegacyId(), skylarkApiProvider.build()) .addProviders( new JavaProtoLibraryTransitiveFilesToBuildProvider(transitiveOutputJars.build()), new JavaCompilationArgsAspectProvider(generatedCompilationArgsProvider)); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java index 4f8835befb..3d5aa88e92 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java @@ -34,6 +34,7 @@ import com.google.devtools.build.lib.collect.nestedset.Order; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.StoredEventHandler; 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.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.NativeAspectClass; @@ -60,6 +61,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.util.ArrayList; +import java.util.HashMap; import java.util.Map; import javax.annotation.Nullable; @@ -218,16 +220,18 @@ public final class AspectFunction implements SkyFunction { ImmutableList.Builder<Aspect> aspectPathBuilder = ImmutableList.builder(); - if (key.getBaseKey() != null) { - ImmutableList<SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKey()); + if (!key.getBaseKeys().isEmpty()) { + // We transitively collect all required aspects to reduce the number of restarts. + // Semantically it is enough to just request key.getBaseKeys(). + ImmutableMap<AspectDescriptor, SkyKey> aspectKeys = getSkyKeysForAspects(key.getBaseKeys()); - Map<SkyKey, SkyValue> values = env.getValues(aspectKeys); + Map<SkyKey, SkyValue> values = env.getValues(aspectKeys.values()); if (env.valuesMissing()) { return null; } try { associatedTarget = getBaseTargetAndCollectPath( - associatedTarget, aspectKeys, values, aspectPathBuilder); + associatedTarget, key.getBaseKeys(), values, aspectPathBuilder); } catch (DuplicateException e) { env.getListener().handle( Event.error(associatedTarget.getTarget().getLocation(), e.getMessage())); @@ -321,11 +325,13 @@ public final class AspectFunction implements SkyFunction { * @throws DuplicateException if there is a duplicate provider provided by aspects. */ private ConfiguredTarget getBaseTargetAndCollectPath(ConfiguredTarget target, - ImmutableList<SkyKey> aspectKeys, Map<SkyKey, SkyValue> values, + ImmutableList<AspectKey> aspectKeys, + Map<SkyKey, SkyValue> values, ImmutableList.Builder<Aspect> aspectPath) throws DuplicateException { ArrayList<ConfiguredAspect> aspectValues = new ArrayList<>(); - for (SkyKey skyAspectKey : aspectKeys) { + for (AspectKey aspectKey : aspectKeys) { + SkyKey skyAspectKey = aspectKey.getSkyKey(); AspectValue aspectValue = (AspectValue) values.get(skyAspectKey); ConfiguredAspect configuredAspect = aspectValue.getConfiguredAspect(); aspectValues.add(configuredAspect); @@ -335,19 +341,28 @@ public final class AspectFunction implements SkyFunction { } /** - * Returns a list of SkyKeys for all aspects the given aspect key depends on. - * The order corresponds to the order the aspects should be merged into a configured target. + * Collect all SkyKeys that are needed for a given list of AspectKeys, + * including transitive dependencies. */ - private ImmutableList<SkyKey> getSkyKeysForAspects(AspectKey key) { - ImmutableList.Builder<SkyKey> aspectKeysBuilder = ImmutableList.builder(); - AspectKey baseKey = key; - while (baseKey != null) { - aspectKeysBuilder.add(baseKey.getSkyKey()); - baseKey = baseKey.getBaseKey(); + private ImmutableMap<AspectDescriptor, SkyKey> getSkyKeysForAspects( + ImmutableList<AspectKey> keys) { + HashMap<AspectDescriptor, SkyKey> result = new HashMap<>(); + for (AspectKey key : keys) { + buildSkyKeys(key, result); } - return aspectKeysBuilder.build().reverse(); + return ImmutableMap.copyOf(result); } + private void buildSkyKeys(AspectKey key, HashMap<AspectDescriptor, SkyKey> result) { + if (result.containsKey(key.getAspectDescriptor())) { + return; + } + ImmutableList<AspectKey> baseKeys = key.getBaseKeys(); + result.put(key.getAspectDescriptor(), key.getSkyKey()); + for (AspectKey baseKey : baseKeys) { + buildSkyKeys(baseKey, result); + } + } private static SkyValue createAliasAspect( Environment env, Target originalTarget, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java index 7accbc1cb0..df6048df17 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectValue.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Objects; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; @@ -47,36 +48,22 @@ public final class AspectValue extends ActionLookupValue { */ public static final class AspectKey extends AspectValueKey { private final Label label; - private final AspectKey baseKey; + private final ImmutableList<AspectKey> baseKeys; private final BuildConfiguration aspectConfiguration; private final BuildConfiguration baseConfiguration; - private final AspectClass aspectClass; - private final AspectParameters parameters; + private final AspectDescriptor aspectDescriptor; private AspectKey( Label label, - BuildConfiguration aspectConfiguration, BuildConfiguration baseConfiguration, - AspectClass aspectClass, - AspectParameters parameters) { - this.baseKey = null; + ImmutableList<AspectKey> baseKeys, + AspectDescriptor aspectDescriptor, + BuildConfiguration aspectConfiguration) { + this.baseKeys = baseKeys; this.label = label; - this.aspectConfiguration = aspectConfiguration; this.baseConfiguration = baseConfiguration; - this.aspectClass = aspectClass; - this.parameters = parameters; - } - - private AspectKey( - AspectKey baseKey, - AspectClass aspectClass, AspectParameters aspectParameters, - BuildConfiguration aspectConfiguration) { - this.baseKey = baseKey; - this.label = baseKey.label; - this.baseConfiguration = baseKey.getBaseConfiguration(); this.aspectConfiguration = aspectConfiguration; - this.aspectClass = aspectClass; - this.parameters = aspectParameters; + this.aspectDescriptor = aspectDescriptor; } @Override @@ -91,25 +78,31 @@ public final class AspectValue extends ActionLookupValue { } public AspectClass getAspectClass() { - return aspectClass; + return aspectDescriptor.getAspectClass(); } @Nullable public AspectParameters getParameters() { - return parameters; + return aspectDescriptor.getParameters(); + } + + public AspectDescriptor getAspectDescriptor() { + return aspectDescriptor; } @Nullable - public AspectKey getBaseKey() { - return baseKey; + public ImmutableList<AspectKey> getBaseKeys() { + return baseKeys; } @Override public String getDescription() { - if (baseKey == null) { - return String.format("%s of %s", aspectClass.getName(), getLabel()); + if (baseKeys.isEmpty()) { + return String.format("%s of %s", + aspectDescriptor.getAspectClass().getName(), getLabel()); } else { - return String.format("%s on top of %s", aspectClass.getName(), baseKey.toString()); + return String.format("%s on top of %s", + aspectDescriptor.getAspectClass().getName(), baseKeys.toString()); } } @@ -153,11 +146,10 @@ public final class AspectValue extends ActionLookupValue { public int hashCode() { return Objects.hashCode( label, - baseKey, + baseKeys, aspectConfiguration, baseConfiguration, - aspectClass, - parameters); + aspectDescriptor); } @Override @@ -172,45 +164,52 @@ public final class AspectValue extends ActionLookupValue { AspectKey that = (AspectKey) other; return Objects.equal(label, that.label) - && Objects.equal(baseKey, that.baseKey) + && Objects.equal(baseKeys, that.baseKeys) && Objects.equal(aspectConfiguration, that.aspectConfiguration) && Objects.equal(baseConfiguration, that.baseConfiguration) - && Objects.equal(aspectClass, that.aspectClass) - && Objects.equal(parameters, that.parameters); + && Objects.equal(aspectDescriptor, that.aspectDescriptor); } public String prettyPrint() { if (label == null) { return "null"; } - return String.format("%s with aspect %s%s", - baseKey == null ? label.toString() : baseKey.prettyPrint(), - aspectClass.getName(), + + String baseKeysString = + baseKeys.isEmpty() + ? "" + : String.format(" (over %s)", baseKeys.toString()); + return String.format("%s with aspect %s%s%s", + label.toString(), + aspectDescriptor.getAspectClass().getName(), (aspectConfiguration != null && aspectConfiguration.isHostConfiguration()) - ? "(host) " : ""); + ? "(host) " : "", + baseKeysString + ); } @Override public String toString() { - return (baseKey == null ? label : baseKey.toString()) + return (baseKeys == null ? label : baseKeys.toString()) + "#" - + aspectClass.getName() + + aspectDescriptor.getAspectClass().getName() + " " + (aspectConfiguration == null ? "null" : aspectConfiguration.checksum()) + " " + (baseConfiguration == null ? "null" : baseConfiguration.checksum()) + " " - + parameters; + + aspectDescriptor.getParameters(); } public AspectKey withLabel(Label label) { - if (baseKey == null) { - return new AspectKey( - label, aspectConfiguration, baseConfiguration, aspectClass, parameters); - } else { - return new AspectKey( - baseKey.withLabel(label), aspectClass, parameters, aspectConfiguration); + ImmutableList.Builder<AspectKey> newBaseKeys = ImmutableList.builder(); + for (AspectKey baseKey : baseKeys) { + newBaseKeys.add(baseKey.withLabel(label)); } + + return new AspectKey( + label, baseConfiguration, + newBaseKeys.build(), aspectDescriptor, aspectConfiguration); } } @@ -354,10 +353,14 @@ public final class AspectValue extends ActionLookupValue { return transitivePackages; } - public static AspectKey createAspectKey(AspectKey baseKey, AspectDescriptor aspectDescriptor, + public static AspectKey createAspectKey( + Label label, BuildConfiguration baseConfiguration, + ImmutableList<AspectKey> baseKeys, AspectDescriptor aspectDescriptor, BuildConfiguration aspectConfiguration) { return new AspectKey( - baseKey, aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters(), + label, baseConfiguration, + baseKeys, + aspectDescriptor, aspectConfiguration ); } @@ -368,8 +371,8 @@ public final class AspectValue extends ActionLookupValue { BuildConfiguration baseConfiguration, AspectDescriptor aspectDescriptor, BuildConfiguration aspectConfiguration) { return new AspectKey( - label, aspectConfiguration, baseConfiguration, - aspectDescriptor.getAspectClass(), aspectDescriptor.getParameters()); + label, baseConfiguration, ImmutableList.<AspectKey>of(), + aspectDescriptor, aspectConfiguration); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index e481dbd1d9..673a18fc40 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -31,6 +31,8 @@ import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.Actions; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException; +import com.google.devtools.build.lib.analysis.AspectCollection; +import com.google.devtools.build.lib.analysis.AspectCollection.AspectDeps; import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment; import com.google.devtools.build.lib.analysis.ConfiguredAspect; import com.google.devtools.build.lib.analysis.ConfiguredTarget; @@ -82,6 +84,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashMap; @@ -849,18 +852,14 @@ final class ConfiguredTargetFunction implements SkyFunction { NestedSetBuilder<Package> transitivePackages) throws AspectCreationException, InterruptedException { OrderedSetMultimap<SkyKey, ConfiguredAspect> result = OrderedSetMultimap.create(); - Set<SkyKey> aspectKeys = new HashSet<>(); + Set<SkyKey> allAspectKeys = new HashSet<>(); for (Dependency dep : deps) { - AspectKey key = null; - for (Entry<AspectDescriptor, BuildConfiguration> depAspect - : dep.getAspectConfigurations().entrySet()) { - key = getNextAspectKey(key, dep, depAspect); - aspectKeys.add(key.getSkyKey()); - } + allAspectKeys.addAll(getAspectKeys(dep).values()); } Map<SkyKey, ValueOrException2<AspectCreationException, NoSuchThingException>> depAspects = - env.getValuesOrThrow(aspectKeys, AspectCreationException.class, NoSuchThingException.class); + env.getValuesOrThrow(allAspectKeys, + AspectCreationException.class, NoSuchThingException.class); for (Dependency dep : deps) { SkyKey depKey = TO_KEYS.apply(dep); @@ -869,12 +868,12 @@ final class ConfiguredTargetFunction implements SkyFunction { if (result.containsKey(depKey)) { continue; } - AspectKey key = null; + Map<AspectDescriptor, SkyKey> aspectToKeys = getAspectKeys(dep); + ConfiguredTarget depConfiguredTarget = configuredTargetMap.get(depKey); - for (Entry<AspectDescriptor, BuildConfiguration> depAspect - : dep.getAspectConfigurations().entrySet()) { - key = getNextAspectKey(key, dep, depAspect); - SkyKey aspectKey = key.getSkyKey(); + for (AspectDeps depAspect : dep.getAspects().getVisibleAspects()) { + SkyKey aspectKey = aspectToKeys.get(depAspect.getAspect()); + AspectValue aspectValue; try { // TODO(ulfjack): Catch all thrown AspectCreationException and NoSuchThingException @@ -884,7 +883,7 @@ final class ConfiguredTargetFunction implements SkyFunction { throw new AspectCreationException( String.format( "Evaluation of aspect %s on %s failed: %s", - depAspect.getKey().getAspectClass().getName(), + depAspect.getAspect().getAspectClass().getName(), dep.getLabel(), e.toString())); } @@ -904,16 +903,32 @@ final class ConfiguredTargetFunction implements SkyFunction { return result; } - private static AspectKey getNextAspectKey(AspectKey key, Dependency dep, - Entry<AspectDescriptor, BuildConfiguration> depAspect) { - if (key == null) { - key = AspectValue.createAspectKey(dep.getLabel(), - dep.getConfiguration(), depAspect.getKey(), depAspect.getValue() - ); - } else { - key = AspectValue.createAspectKey(key, depAspect.getKey(), depAspect.getValue()); + private static Map<AspectDescriptor, SkyKey> getAspectKeys(Dependency dep) { + HashMap<AspectDescriptor, SkyKey> result = new HashMap<>(); + AspectCollection aspects = dep.getAspects(); + for (AspectDeps aspectDeps : aspects.getVisibleAspects()) { + buildAspectKey(aspectDeps, result, dep); + } + return result; + } + + private static AspectKey buildAspectKey(AspectDeps aspectDeps, + HashMap<AspectDescriptor, SkyKey> result, Dependency dep) { + if (result.containsKey(aspectDeps.getAspect())) { + return (AspectKey) result.get(aspectDeps.getAspect()).argument(); + } + + ImmutableList.Builder<AspectKey> dependentAspects = ImmutableList.builder(); + for (AspectDeps path : aspectDeps.getDependentAspects()) { + dependentAspects.add(buildAspectKey(path, result, dep)); } - return key; + AspectKey aspectKey = AspectValue.createAspectKey( + dep.getLabel(), dep.getConfiguration(), + dependentAspects.build(), + aspectDeps.getAspect(), + dep.getAspectConfiguration(aspectDeps.getAspect())); + result.put(aspectKey.getAspectDescriptor(), aspectKey.getSkyKey()); + return aspectKey; } private static boolean aspectMatchesConfiguredTarget(final ConfiguredTarget dep, Aspect aspect) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index 10e565714b..46b2b9df47 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -50,6 +50,7 @@ import com.google.devtools.build.lib.actions.Executor; import com.google.devtools.build.lib.actions.PackageRootResolutionException; import com.google.devtools.build.lib.actions.ResourceManager; import com.google.devtools.build.lib.actions.Root; +import com.google.devtools.build.lib.analysis.AspectCollection; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BuildView.Options; import com.google.devtools.build.lib.analysis.ConfiguredAspect; @@ -1227,7 +1228,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { } for (BuildConfiguration depConfig : configs.get(key)) { skyKeys.add(ConfiguredTargetValue.key(key.getLabel(), depConfig)); - for (AspectDescriptor aspectDescriptor : key.getAspects()) { + for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) { skyKeys.add(ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), depConfig, aspectDescriptor, depConfig))); } @@ -1260,7 +1261,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { ((ConfiguredTargetValue) result.get(configuredTargetKey)).getConfiguredTarget(); List<ConfiguredAspect> configuredAspects = new ArrayList<>(); - for (AspectDescriptor aspectDescriptor : key.getAspects()) { + for (AspectDescriptor aspectDescriptor : key.getAspects().getAllAspects()) { SkyKey aspectKey = ActionLookupValue.key(AspectValue.createAspectKey(key.getLabel(), depConfig, aspectDescriptor, depConfig)); if (result.get(aspectKey) == null) { @@ -1450,7 +1451,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { dep = Dependency.withNullConfiguration(label); } else if (configuration.useDynamicConfigurations()) { dep = Dependency.withTransitionAndAspects(label, Attribute.ConfigurationTransition.NONE, - ImmutableSet.<AspectDescriptor>of()); + AspectCollection.EMPTY); } else { dep = Dependency.withConfiguration(label, configuration); } |