Restrict aspects visible to other aspects according to their advertised providers.
--
PiperOrigin-RevId: 147526961
MOS_MIGRATED_REVID=147526961
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 0018525..6427014 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 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 19e972c..10a9216 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 @@
targetAndConfig.getLabel(),
Attribute.ConfigurationTransition.NONE,
// TODO(bazel-team): support top-level aspects
- ImmutableSet.<AspectDescriptor>of()));
+ AspectCollection.EMPTY));
}
}
@@ -1003,7 +1003,7 @@
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 d72bced..9b67fe8 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 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 @@
* <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 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 @@
* 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 @@
* 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 @@
}
@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 @@
*/
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 @@
}
@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 @@
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 @@
*/
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 @@
}
@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 dc1f292..06d561b 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.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 @@
}
- 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();
- for (Aspect aspectCandidate : aspectCandidates) {
- if (aspectCandidate.getDefinition()
- .getRequiredProviders()
- .isSatisfiedBy(ruleClass.getAdvertisedProviders())) {
- result.add(
- new AspectDescriptor(
- aspectCandidate.getAspectClass(),
- aspectCandidate.getParameters()));
+ 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);
+ }
+ }
+
+ /**
+ * 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;
}
}
- return result.build();
+
+ 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())) {
+ filteredAspectPath.add(baseAspect);
+ visibleAspects.add(baseAspect.getDescriptor());
+ }
+ }
}
private static Iterable<Aspect> extractAspectCandidates(
@@ -686,7 +737,7 @@
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 5823a52..b16a4a6 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.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.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 @@
* 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 @@
@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 @@
@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 a305e51..8cc374f 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 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 5518692..ebd734b 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 @@
.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 591057e..5e89e5b 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.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 @@
/** 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 a151b3d..9eef47a 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 @@
.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 @@
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 a248bbf..e702f27 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 @@
.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 @@
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 4f8835b..3d5aa88 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.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.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 @@
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 @@
* @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 @@
}
/**
- * 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 7accbc1..df6048d 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 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;
- this.label = label;
- this.aspectConfiguration = aspectConfiguration;
- this.baseConfiguration = baseConfiguration;
- this.aspectClass = aspectClass;
- this.parameters = parameters;
- }
-
- private AspectKey(
- AspectKey baseKey,
- AspectClass aspectClass, AspectParameters aspectParameters,
+ ImmutableList<AspectKey> baseKeys,
+ AspectDescriptor aspectDescriptor,
BuildConfiguration aspectConfiguration) {
- this.baseKey = baseKey;
- this.label = baseKey.label;
- this.baseConfiguration = baseKey.getBaseConfiguration();
+ this.baseKeys = baseKeys;
+ this.label = label;
+ this.baseConfiguration = baseConfiguration;
this.aspectConfiguration = aspectConfiguration;
- this.aspectClass = aspectClass;
- this.parameters = aspectParameters;
+ this.aspectDescriptor = aspectDescriptor;
}
@Override
@@ -91,25 +78,31 @@
}
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 int hashCode() {
return Objects.hashCode(
label,
- baseKey,
+ baseKeys,
aspectConfiguration,
baseConfiguration,
- aspectClass,
- parameters);
+ aspectDescriptor);
}
@Override
@@ -172,45 +164,52 @@
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 @@
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 @@
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 e481dbd..673a18f 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.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.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 @@
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 @@
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 @@
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 @@
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 key;
+ 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));
+ }
+ 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 10e5657..46b2b9d 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.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 @@
}
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 @@
((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 @@
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);
}
