diff options
author | 2015-09-25 21:35:26 +0000 | |
---|---|---|
committer | 2015-09-28 11:39:40 +0000 | |
commit | e27d06308902f44b53809cffd23fc8064e8a7297 (patch) | |
tree | 8b04c86cf20edc174bb230e33ab455b38fb9bd73 /src/main/java/com/google/devtools/build/lib | |
parent | 7bd3e023e4791468efe06a989645d56c8cf880a4 (diff) |
Clean up Aspect checks in query tests.
--
MOS_MIGRATED_REVID=103977080
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
5 files changed, 183 insertions, 103 deletions
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 41cdfb48dc..9bacb4ac87 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 @@ -53,6 +53,7 @@ public final class AspectDefinition { private final String name; private final ImmutableSet<Class<?>> requiredProviders; + private final ImmutableSet<String> requiredProviderNames; private final ImmutableMap<String, Attribute> attributes; private final ImmutableMultimap<String, Class<? extends AspectFactory<?, ?, ?>>> attributeAspects; @@ -63,6 +64,7 @@ public final class AspectDefinition { ImmutableMultimap<String, Class<? extends AspectFactory<?, ?, ?>>> attributeAspects) { this.name = name; this.requiredProviders = requiredProviders; + this.requiredProviderNames = toStringSet(requiredProviders); this.attributes = attributes; this.attributeAspects = attributeAspects; } @@ -95,6 +97,20 @@ public final class AspectDefinition { } /** + * Returns the set of {@link com.google.devtools.build.lib.analysis.TransitiveInfoProvider} + * instances that must be present on a configured target so that this aspect can be applied to it. + * + * <p>We cannot refer to that class here due to our dependency structure, so this returns a set + * of unconstrained class objects. + * + * <p>If a configured target does not have a required provider, the aspect is silently not created + * for it. + */ + public ImmutableSet<String> getRequiredProviderNames() { + return requiredProviderNames; + } + + /** * Returns the attribute -> set of required aspects map. * * <p>Note that the map actually contains {@link AspectFactory} @@ -113,14 +129,27 @@ public final class AspectDefinition { if (!(from instanceof Rule) || !(to instanceof Rule)) { return ImmutableMultimap.of(); } - LinkedHashMultimap<Attribute, Label> result = LinkedHashMultimap.create(); RuleClass ruleClass = ((Rule) to).getRuleClassObject(); + ImmutableSet<Class<?>> providers = ruleClass.getAdvertisedProviders(); + return visitAspectsIfRequired(from, attribute, toStringSet(providers)); + } + + /** + * Returns the attribute -> set of labels that are provided by aspects of attribute. + */ + public static ImmutableMultimap<Attribute, Label> visitAspectsIfRequired( + Target from, Attribute attribute, Set<String> advertisedProviders) { + if (advertisedProviders.isEmpty()) { + return ImmutableMultimap.of(); + } + + LinkedHashMultimap<Attribute, Label> result = LinkedHashMultimap.create(); for (Class<? extends AspectFactory<?, ?, ?>> candidateClass : attribute.getAspects()) { AspectFactory<?, ?, ?> candidate = AspectFactory.Util.create(candidateClass); // Check if target satisfies condition for this aspect (has to provide all required // TransitiveInfoProviders) - if (!ruleClass.getAdvertisedProviders().containsAll( - candidate.getDefinition().getRequiredProviders())) { + if (!advertisedProviders.containsAll( + candidate.getDefinition().getRequiredProviderNames())) { continue; } addAllAttributesOfAspect((Rule) from, result, candidate.getDefinition(), Rule.ALL_DEPS); @@ -128,6 +157,14 @@ public final class AspectDefinition { return ImmutableMultimap.copyOf(result); } + private static ImmutableSet<String> toStringSet(ImmutableSet<Class<?>> classes) { + ImmutableSet.Builder<String> classStrings = new ImmutableSet.Builder<>(); + for (Class<?> clazz : classes) { + classStrings.add(clazz.getName()); + } + return classStrings.build(); + } + /** * Collects all attribute labels from the specified aspectDefinition. */ diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java index e13f6594e5..5bdcd9c01d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveBaseTraversalFunction.java @@ -14,11 +14,12 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Preconditions; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Multimap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; +import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.InputFile; import com.google.devtools.build.lib.packages.NoSuchPackageException; @@ -36,8 +37,11 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException2; +import java.util.Collection; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Map.Entry; import java.util.Set; @@ -113,23 +117,20 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> TargetAndErrorIfAny targetAndErrorIfAny = (TargetAndErrorIfAny) loadTargetResults; TProcessedTargets processedTargets = processTarget(label, targetAndErrorIfAny); - // Process deps from attributes and conservative aspects of current target. + // Process deps from attributes. Iterable<SkyKey> labelDepKeys = getLabelDepKeys(targetAndErrorIfAny.getTarget()); - Iterable<SkyKey> labelAspectKeys = - getConservativeLabelAspectKeys(targetAndErrorIfAny.getTarget()); - Iterable<SkyKey> depAndAspectKeys = Iterables.concat(labelDepKeys, labelAspectKeys); - Set<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>> - depsAndAspectEntries = env.getValuesOrThrow(depAndAspectKeys, - NoSuchPackageException.class, NoSuchTargetException.class).entrySet(); - processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depsAndAspectEntries); + Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap = + env.getValuesOrThrow(labelDepKeys, NoSuchPackageException.class, + NoSuchTargetException.class); + processDeps(processedTargets, env.getListener(), targetAndErrorIfAny, depMap.entrySet()); if (env.valuesMissing()) { return null; } - - // Process deps from strict aspects. - labelAspectKeys = getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), env); + // Process deps from aspects. + Iterable<SkyKey> labelAspectKeys = + getStrictLabelAspectKeys(targetAndErrorIfAny.getTarget(), depMap, env); Set<Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>>> labelAspectEntries = env.getValuesOrThrow(labelAspectKeys, NoSuchPackageException.class, NoSuchTargetException.class).entrySet(); @@ -151,18 +152,41 @@ abstract class TransitiveBaseTraversalFunction<TProcessedTargets> * * <p>This method may return a precise set of aspect keys, but may need to request additional * dependencies from the env to do so. - * - * <p>Subclasses should implement only one of #getStrictLabelAspectKeys and - * @getConservativeLabelAspectKeys. */ - protected abstract Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env); + private Iterable<SkyKey> getStrictLabelAspectKeys(Target target, + Map<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> depMap, + Environment env) { + List<SkyKey> depKeys = Lists.newArrayList(); + if (target instanceof Rule) { + Map<Label, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> labelDepMap = + new HashMap<>(depMap.size()); + for (Entry<SkyKey, ValueOrException2<NoSuchPackageException, NoSuchTargetException>> entry : + depMap.entrySet()) { + labelDepMap.put((Label) entry.getKey().argument(), entry.getValue()); + } + + Multimap<Attribute, Label> transitions = + ((Rule) target).getTransitions(Rule.NO_NODEP_ATTRIBUTES); + for (Entry<Attribute, Label> entry : transitions.entries()) { + ValueOrException2<NoSuchPackageException, NoSuchTargetException> value = + labelDepMap.get(entry.getValue()); + for (Label label : + getAspectLabels(target, entry.getKey(), entry.getValue(), value, env)) { + depKeys.add(getKey(label)); + } + } + } + return depKeys; + } /** - * Return an Iterable of SkyKeys corresponding to the Aspect-related dependencies of target. - * - * <p>This method may return a conservative over-approximation of the exact set. + * Get the Aspect-related Label deps for the given edge. */ - protected abstract Iterable<SkyKey> getConservativeLabelAspectKeys(Target target); + protected abstract Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, + Label toLabel, + ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal, + Environment env); + private Iterable<SkyKey> getLabelDepKeys(Target target) { List<SkyKey> depKeys = Lists.newArrayList(); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java index a01bec072c..06e068dd2e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTargetFunction.java @@ -13,9 +13,8 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Lists; -import com.google.common.collect.Multimap; import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.BuildConfiguration.Fragment; @@ -41,7 +40,6 @@ import com.google.devtools.build.skyframe.ValueOrException2; import java.util.Collection; import java.util.LinkedHashSet; -import java.util.List; import java.util.Map.Entry; import java.util.Set; @@ -154,43 +152,29 @@ public class TransitiveTargetFunction return builder.build(errorLoadingTarget); } - @Override - protected Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env) { - List<SkyKey> depKeys = Lists.newArrayList(); - if (target instanceof Rule) { - Multimap<Attribute, Label> transitions = - ((Rule) target).getTransitions(Rule.NO_NODEP_ATTRIBUTES); - for (Entry<Attribute, Label> entry : transitions.entries()) { - SkyKey packageKey = PackageValue.key(entry.getValue().getPackageIdentifier()); - try { - PackageValue pkgValue = - (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class); - if (pkgValue == null) { - continue; - } - Package pkg = pkgValue.getPackage(); - if (pkg.containsErrors()) { - // Do nothing. This error was handled when we computed the corresponding - // TransitiveTargetValue. - continue; - } - Collection<Label> labels = AspectDefinition.visitAspectsIfRequired(target, entry.getKey(), - pkgValue.getPackage().getTarget(entry.getValue().getName())).values(); - for (Label label : labels) { - depKeys.add(getKey(label)); - } - } catch (NoSuchThingException e) { - // Do nothing. This error was handled when we computed the corresponding - // TransitiveTargetValue. - } + protected Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, Label toLabel, + ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal, + Environment env) { + SkyKey packageKey = PackageValue.key(toLabel.getPackageIdentifier()); + try { + PackageValue pkgValue = + (PackageValue) env.getValueOrThrow(packageKey, NoSuchPackageException.class); + if (pkgValue == null) { + return ImmutableList.of(); + } + Package pkg = pkgValue.getPackage(); + if (pkg.containsErrors()) { + // Do nothing. This error was handled when we computed the corresponding + // TransitiveTargetValue. + return ImmutableList.of(); } + Target dependedTarget = pkgValue.getPackage().getTarget(toLabel.getName()); + return AspectDefinition.visitAspectsIfRequired(fromTarget, attr, dependedTarget).values(); + } catch (NoSuchThingException e) { + // Do nothing. This error was handled when we computed the corresponding + // TransitiveTargetValue. + return ImmutableList.of(); } - return depKeys; - } - - @Override - protected Iterable<SkyKey> getConservativeLabelAspectKeys(Target target) { - return ImmutableSet.of(); } /** diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java index 955d8ee238..bd28a39b4c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunction.java @@ -13,25 +13,24 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.LinkedHashMultimap; -import com.google.common.collect.Multimap; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.AspectDefinition; -import com.google.devtools.build.lib.packages.AspectFactory; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.NoSuchPackageException; import com.google.devtools.build.lib.packages.NoSuchTargetException; -import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.skyframe.TransitiveTraversalFunction.DummyAccumulator; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.ValueOrException2; +import java.util.Collection; import java.util.Map.Entry; +import java.util.Set; /** * This class is like {@link TransitiveTargetFunction}, but the values it returns do not contain @@ -77,41 +76,36 @@ public class TransitiveTraversalFunction extends TransitiveBaseTraversalFunction } } + protected Collection<Label> getAspectLabels(Target fromTarget, Attribute attr, Label toLabel, + ValueOrException2<NoSuchPackageException, NoSuchTargetException> toVal, Environment env) { + try { + if (toVal == null) { + return ImmutableList.of(); + } + TransitiveTraversalValue traversalVal = (TransitiveTraversalValue) toVal.get(); + if (traversalVal == null || traversalVal.getProviders() == null) { + return ImmutableList.of(); + } + // Retrieve the providers of the dep from the TransitiveTraversalValue, so we can avoid + // issuing a dep on its defining Package. + Set<String> providers = traversalVal.getProviders(); + return AspectDefinition.visitAspectsIfRequired(fromTarget, attr, providers).values(); + } catch (NoSuchThingException e) { + // Do nothing. This error was handled when we computed the corresponding + // TransitiveTargetValue. + return ImmutableList.of(); + } + } + @Override SkyValue computeSkyValue(TargetAndErrorIfAny targetAndErrorIfAny, DummyAccumulator processedTargets) { NoSuchTargetException errorLoadingTarget = targetAndErrorIfAny.getErrorLoadingTarget(); return errorLoadingTarget == null - ? TransitiveTraversalValue.SUCCESSFUL_TRANSITIVE_TRAVERSAL_VALUE + ? TransitiveTraversalValue.forTarget(targetAndErrorIfAny.getTarget()) : TransitiveTraversalValue.unsuccessfulTransitiveTraversal(errorLoadingTarget); } - @Override - protected Iterable<SkyKey> getStrictLabelAspectKeys(Target target, Environment env) { - return ImmutableSet.of(); - } - - @Override - protected Iterable<SkyKey> getConservativeLabelAspectKeys(Target target) { - if (!(target instanceof Rule)) { - return ImmutableSet.of(); - } - Rule rule = (Rule) target; - Multimap<Attribute, Label> attibuteMap = LinkedHashMultimap.create(); - for (Attribute attribute : rule.getTransitions(Rule.NO_NODEP_ATTRIBUTES).keys()) { - for (Class<? extends AspectFactory<?, ?, ?>> aspectFactory : attribute.getAspects()) { - AspectDefinition.addAllAttributesOfAspect(rule, attibuteMap, - AspectFactory.Util.create(aspectFactory).getDefinition(), Rule.ALL_DEPS); - } - } - - ImmutableSet.Builder<SkyKey> depKeys = new ImmutableSet.Builder<>(); - for (Label label : attibuteMap.values()) { - depKeys.add(getKey(label)); - } - return depKeys.build(); - } - /** * Because {@link TransitiveTraversalFunction} is invoked only when its side-effects are desired, * this value accumulator has nothing to keep track of. diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java index e01588b7a2..483167811a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalValue.java @@ -14,14 +14,21 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable; import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe; import com.google.devtools.build.lib.packages.NoSuchTargetException; +import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.util.StringCanonicalizer; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.Collection; import java.util.Objects; +import java.util.Set; import javax.annotation.Nullable; @@ -35,20 +42,53 @@ public class TransitiveTraversalValue implements SkyValue { @Nullable private final NoSuchTargetException errorLoadingTarget; + @Nullable + private final ImmutableSet<String> providers; - // Note that this value does not guarantee singleton-like reference equality for successful - // {@link TransitiveTraversalValue}s because we use Java deserialization. Java deserialization can - // create other instances. - public static final TransitiveTraversalValue SUCCESSFUL_TRANSITIVE_TRAVERSAL_VALUE = - new TransitiveTraversalValue(null); + private TransitiveTraversalValue(@Nullable Iterable<String> providers, + @Nullable NoSuchTargetException errorLoadingTarget) { + this.errorLoadingTarget = errorLoadingTarget; + this.providers = (providers == null) ? null : canonicalSet(providers); + } public static TransitiveTraversalValue unsuccessfulTransitiveTraversal( NoSuchTargetException errorLoadingTarget) { - return new TransitiveTraversalValue(Preconditions.checkNotNull(errorLoadingTarget)); + return new TransitiveTraversalValue(null, Preconditions.checkNotNull(errorLoadingTarget)); } - private TransitiveTraversalValue(@Nullable NoSuchTargetException errorLoadingTarget) { - this.errorLoadingTarget = errorLoadingTarget; + public static TransitiveTraversalValue forTarget(Target target) { + if (target instanceof Rule) { + Rule rule = (Rule) target; + return new TransitiveTraversalValue( + toStringSet(rule.getRuleClassObject().getAdvertisedProviders()), null); + } + return new TransitiveTraversalValue(ImmutableList.<String>of(), null); + } + + public static TransitiveTraversalValue withProviders(Collection<String> vals) { + return new TransitiveTraversalValue(ImmutableSet.copyOf(vals), null); + } + + private static ImmutableSet<String> canonicalSet(Iterable<String> strIterable) { + ImmutableSet.Builder<String> builder = new ImmutableSet.Builder<>(); + for (String str : strIterable) { + builder.add(StringCanonicalizer.intern(str)); + } + return builder.build(); + } + + private static ImmutableSet<String> toStringSet(Iterable<Class<?>> providers) { + ImmutableSet.Builder<String> pBuilder = new ImmutableSet.Builder<>(); + if (providers != null) { + for (Class<?> clazz : providers) { + pBuilder.add(StringCanonicalizer.intern(clazz.getName())); + } + } + return pBuilder.build(); + } + + public Set<String> getProviders() { + return providers; } /** Returns the error, if any, from loading the target. */ @@ -66,12 +106,13 @@ public class TransitiveTraversalValue implements SkyValue { return false; } TransitiveTraversalValue that = (TransitiveTraversalValue) o; - return Objects.equals(this.errorLoadingTarget, that.errorLoadingTarget); + return Objects.equals(this.errorLoadingTarget, that.errorLoadingTarget) + && Objects.equals(this.providers, that.providers); } @Override public int hashCode() { - return Objects.hashCode(errorLoadingTarget); + return 31 * Objects.hashCode(errorLoadingTarget) + Objects.hashCode(providers); } @ThreadSafe |