diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java index 44b61c7506e229..f6337219a9a732 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java @@ -159,11 +159,7 @@ private static AspectKey buildAspectKey( } AspectKey aspectKey = AspectKeyCreator.createAspectKey( - dep.getLabel(), - dep.getConfiguration(), - dependentAspects.build(), - aspectDeps.getAspect(), - dep.getAspectConfiguration(aspectDeps.getAspect())); + aspectDeps.getAspect(), dependentAspects.build(), dep.getConfiguredTargetKey()); result.put(aspectKey.getAspectDescriptor(), aspectKey); return aspectKey; } 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 25f62574562f12..f1bbbc0ce29c9e 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 @@ -130,12 +130,6 @@ public static Builder builder() { */ public abstract AspectCollection getAspects(); - /** Returns the configuration an aspect should be evaluated with. */ - @Nullable - public BuildConfigurationValue getAspectConfiguration(AspectDescriptor aspect) { - return getConfiguration(); - } - /** * Returns the keys of a configuration transition, if any exist, associated with this dependency. * See {@link ConfigurationTransition#apply} for more details. Normally, this returns an empty 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 eaf519d1d234da..4cf3ceae0f457e 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 @@ -56,6 +56,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.events.Event; import com.google.devtools.build.lib.events.StoredEventHandler; import com.google.devtools.build.lib.packages.Aspect; @@ -88,7 +89,9 @@ import com.google.devtools.build.skyframe.ValueOrException; import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Objects; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -187,7 +190,9 @@ public SkyValue compute(SkyKey skyKey, Environment env) NativeAspectClass nativeAspectClass = (NativeAspectClass) key.getAspectClass(); aspectFactory = (ConfiguredAspectFactory) nativeAspectClass; aspect = Aspect.forNative(nativeAspectClass, key.getParameters()); - } else if (key.getAspectClass() instanceof StarlarkAspectClass) { + } else { + Preconditions.checkState( + key.getAspectClass() instanceof StarlarkAspectClass, "Unknown aspect class: %s", key); StarlarkAspectClass starlarkAspectClass = (StarlarkAspectClass) key.getAspectClass(); StarlarkDefinedAspect starlarkAspect; try { @@ -205,83 +210,70 @@ public SkyValue compute(SkyKey skyKey, Environment env) starlarkAspect.getAspectClass(), starlarkAspect.getDefinition(key.getParameters()), key.getParameters()); - } else { - throw new IllegalStateException(); } + SkyKey packageKey = PackageValue.key(key.getLabel().getPackageIdentifier()); + BuildConfigurationKey configurationKey = key.getConfigurationKey(); + ImmutableSet keys = + configurationKey == null + ? ImmutableSet.of(packageKey, key.getBaseConfiguredTargetKey()) + : ImmutableSet.of(packageKey, key.getBaseConfiguredTargetKey(), configurationKey); + + List> baseValues = + env.getOrderedValuesOrThrow(keys, ConfiguredValueCreationException.class); + // Keep this in sync with the same code in ConfiguredTargetFunction. - PackageValue packageValue = - (PackageValue) env.getValue(PackageValue.key(key.getLabel().getPackageIdentifier())); + PackageValue packageValue; + try { + packageValue = (PackageValue) baseValues.get(0).get(); + } catch (ConfiguredValueCreationException e) { + throw new IllegalStateException(e); + } if (packageValue == null) { return null; } - Package pkg = packageValue.getPackage(); - if (pkg.containsErrors()) { + if (packageValue.getPackage().containsErrors()) { throw new AspectFunctionException( new BuildFileContainsErrorsException(key.getLabel().getPackageIdentifier())); } - boolean aspectHasConfiguration = key.getConfigurationKey() != null; - - ImmutableSet keys = - aspectHasConfiguration - ? ImmutableSet.of(key.getBaseConfiguredTargetKey(), key.getConfigurationKey()) - : ImmutableSet.of(key.getBaseConfiguredTargetKey()); - - Map> baseAndAspectValues = - env.getValuesOrThrow(keys, ConfiguredValueCreationException.class); if (env.valuesMissing()) { return null; } ConfiguredTargetValue baseConfiguredTargetValue; - BuildConfigurationValue aspectConfiguration = null; - try { - baseConfiguredTargetValue = - (ConfiguredTargetValue) baseAndAspectValues.get(key.getBaseConfiguredTargetKey()).get(); + baseConfiguredTargetValue = (ConfiguredTargetValue) baseValues.get(1).get(); } catch (ConfiguredValueCreationException e) { throw new AspectFunctionException( new AspectCreationException(e.getMessage(), e.getRootCauses(), e.getDetailedExitCode())); } + ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget(); + Preconditions.checkState( + Objects.equals(key.getConfigurationKey(), associatedTarget.getConfigurationKey()), + "Aspect not in same configuration as associated target: %s, %s", + key, + associatedTarget); - if (aspectHasConfiguration) { + BuildConfigurationValue configuration; + if (configurationKey == null) { + configuration = null; + } else { try { - aspectConfiguration = - (BuildConfigurationValue) baseAndAspectValues.get(key.getConfigurationKey()).get(); + configuration = (BuildConfigurationValue) baseValues.get(2).get(); } catch (ConfiguredValueCreationException e) { - throw new IllegalStateException( - "Unexpected exception from BuildConfigurationFunction when computing " - + key.getConfigurationKey(), - e); + throw new IllegalStateException(e); } } - ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget(); - - Package targetPkg; - BuildConfigurationValue configuration = null; - PackageValue.Key packageKey = - PackageValue.key(associatedTarget.getOriginalLabel().getPackageIdentifier()); - if (associatedTarget.getConfigurationKey() == null) { - PackageValue val = ((PackageValue) env.getValue(packageKey)); - if (val == null) { - // Unexpected in Bazel logic, but Skyframe makes no guarantees that this package is - // actually present. - return null; - } - targetPkg = val.getPackage(); - } else { - Map result = - env.getValues(ImmutableSet.of(packageKey, associatedTarget.getConfigurationKey())); - if (env.valuesMissing()) { - // Unexpected in Bazel logic, but Skyframe makes no guarantees that this package and - // configuration are actually present. - return null; - } - targetPkg = ((PackageValue) result.get(packageKey)).getPackage(); - configuration = (BuildConfigurationValue) result.get(associatedTarget.getConfigurationKey()); + PackageValue val = + (PackageValue) + env.getValue( + PackageValue.key(associatedTarget.getOriginalLabel().getPackageIdentifier())); + if (val == null) { + return null; } + Package targetPkg = val.getPackage(); Target target; try { @@ -297,7 +289,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) new TargetAndConfiguration(target, configuration), aspect, key, - aspectConfiguration, + configuration, associatedTarget); } // If we get here, label should match original label, and therefore the target we looked up @@ -332,9 +324,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) aspect, target.getLocation(), ConfiguredAspect.forNonapplicableTarget(), - /*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder - .stableOrder() - .build()); + /*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder.emptySet( + Order.STABLE_ORDER)); } } } @@ -357,8 +348,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) aspectPathBuilder.add(((AspectValue) values.get(aspectPathSkyKey)).getAspect()); } try { - associatedTarget = getBaseTarget( - associatedTarget, key.getBaseKeys(), values); + associatedTarget = getBaseTarget(associatedTarget, key.getBaseKeys(), values); } catch (DuplicateException e) { env.getListener() .handle( @@ -367,7 +357,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throw new AspectFunctionException( new AspectCreationException( - e.getMessage(), associatedTarget.getLabel(), aspectConfiguration)); + e.getMessage(), associatedTarget.getLabel(), configuration)); } } associatedConfiguredTargetAndData = @@ -378,15 +368,8 @@ public SkyValue compute(SkyKey skyKey, Environment env) NestedSetBuilder transitivePackagesForPackageRootResolution = storeTransitivePackagesForPackageRootResolution ? NestedSetBuilder.stableOrder() : null; - // When getting the dependencies of this hybrid aspect+base target, use the aspect's - // configuration. The configuration of the aspect will always be a superset of the target's - // (trimmed configuration mode: target is part of the aspect's config fragment requirements; - // untrimmed mode: target is the same configuration as the aspect), so the fragments - // required by all dependencies (both those of the aspect and those of the base target) - // will be present this way. - TargetAndConfiguration originalTargetAndAspectConfiguration = - new TargetAndConfiguration( - associatedConfiguredTargetAndData.getTarget(), aspectConfiguration); + TargetAndConfiguration originalTargetAndConfiguration = + new TargetAndConfiguration(associatedConfiguredTargetAndData.getTarget(), configuration); ImmutableList aspectPath = aspectPathBuilder.build(); try { UnloadedToolchainContext unloadedToolchainContext = @@ -399,7 +382,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ConfigConditions configConditions = ConfiguredTargetFunction.getConfigConditions( env, - originalTargetAndAspectConfiguration, + originalTargetAndConfiguration, transitivePackagesForPackageRootResolution, unloadedToolchainContext == null ? null : unloadedToolchainContext.targetPlatform(), transitiveRootCauses); @@ -414,7 +397,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) ConfiguredTargetFunction.computeDependencies( env, resolver, - originalTargetAndAspectConfiguration, + originalTargetAndConfiguration, aspectPath, configConditions.asProviders(), unloadedToolchainContext == null @@ -429,7 +412,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) transitiveRootCauses); } catch (ConfiguredValueCreationException e) { throw new AspectCreationException( - e.getMessage(), key.getLabel(), aspectConfiguration, e.getDetailedExitCode()); + e.getMessage(), key.getLabel(), configuration, e.getDetailedExitCode()); } if (depValueMap == null) { return null; @@ -466,7 +449,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) aspect, aspectFactory, associatedConfiguredTargetAndData, - aspectConfiguration, + configuration, configConditions, toolchainContext, depValueMap, @@ -487,21 +470,18 @@ public SkyValue compute(SkyKey skyKey, Environment env) InconsistentAspectOrderException cause = (InconsistentAspectOrderException) e.getCause(); env.getListener().handle(Event.error(cause.getLocation(), cause.getMessage())); throw new AspectFunctionException( - new AspectCreationException(cause.getMessage(), key.getLabel(), aspectConfiguration)); + new AspectCreationException(cause.getMessage(), key.getLabel(), configuration)); } else if (e.getCause() instanceof TransitionException) { TransitionException cause = (TransitionException) e.getCause(); throw new AspectFunctionException( - new AspectCreationException(cause.getMessage(), key.getLabel(), aspectConfiguration)); + new AspectCreationException(cause.getMessage(), key.getLabel(), configuration)); } else { // Cast to InvalidConfigurationException as a consistency check. If you add any // DependencyEvaluationException constructors, you may need to change this code, too. InvalidConfigurationException cause = (InvalidConfigurationException) e.getCause(); throw new AspectFunctionException( new AspectCreationException( - cause.getMessage(), - key.getLabel(), - aspectConfiguration, - cause.getDetailedExitCode())); + cause.getMessage(), key.getLabel(), configuration, cause.getDetailedExitCode())); } } catch (AspectCreationException e) { throw new AspectFunctionException(e); @@ -781,11 +761,12 @@ private static AspectKey buildAliasAspectKey( .map(baseKey -> buildAliasAspectKey(baseKey, aliasLabel, dep)) .collect(toImmutableList()); return AspectKeyCreator.createAspectKey( - aliasLabel, - dep.getConfiguration(), - aliasedBaseKeys, originalKey.getAspectDescriptor(), - dep.getAspectConfiguration(originalKey.getAspectDescriptor())); + aliasedBaseKeys, + ConfiguredTargetKey.builder() + .setLabel(aliasLabel) + .setConfiguration(dep.getConfiguration()) + .build()); } /** @@ -828,7 +809,7 @@ private AspectValue createAspect( Aspect aspect, ConfiguredAspectFactory aspectFactory, ConfiguredTargetAndData associatedTarget, - BuildConfigurationValue aspectConfiguration, + BuildConfigurationValue configuration, ConfigConditions configConditions, ResolvedToolchainContext toolchainContext, OrderedSetMultimap directDeps, @@ -845,8 +826,7 @@ private AspectValue createAspect( StoredEventHandler events = new StoredEventHandler(); CachingAnalysisEnvironment analysisEnvironment = - view.createAnalysisEnvironment( - key, events, env, aspectConfiguration, starlarkBuiltinsValue); + view.createAnalysisEnvironment(key, events, env, configuration, starlarkBuiltinsValue); ConfiguredAspect configuredAspect; if (aspect.getDefinition().applyToGeneratingRules() @@ -874,7 +854,7 @@ private AspectValue createAspect( directDeps, configConditions, toolchainContext, - aspectConfiguration, + configuration, view.getHostConfiguration(), key); } catch (MissingDepException e) { @@ -896,7 +876,7 @@ private AspectValue createAspect( analysisEnvironment.disable(associatedTarget.getTarget()); String msg = "Analysis of target '" + associatedTarget.getTarget().getLabel() + "' failed"; throw new AspectFunctionException( - new AspectCreationException(msg, key.getLabel(), aspectConfiguration)); + new AspectCreationException(msg, key.getLabel(), configuration)); } Preconditions.checkState(!analysisEnvironment.hasErrors(), "Analysis environment hasError() but no errors reported"); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java b/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java index de7fdbc2441a38..3b140dd3395800 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java @@ -32,42 +32,17 @@ public final class AspectKeyCreator { private AspectKeyCreator() {} - private static final Interner aspectKeyInterner = BlazeInterners.newWeakInterner(); - private static final Interner topLevelAspectsKeyInterner = - BlazeInterners.newWeakInterner(); - public static AspectKey createAspectKey( - Label label, - @Nullable BuildConfigurationValue baseConfiguration, - ImmutableList baseKeys, - AspectDescriptor aspectDescriptor, - @Nullable BuildConfigurationValue aspectConfiguration) { - return AspectKey.createAspectKey( - ConfiguredTargetKey.builder().setLabel(label).setConfiguration(baseConfiguration).build(), - baseKeys, - aspectDescriptor, - aspectConfiguration == null ? null : aspectConfiguration.getKey()); + AspectDescriptor aspectDescriptor, ConfiguredTargetKey baseConfiguredTargetKey) { + return createAspectKey( + aspectDescriptor, /*baseKeys=*/ ImmutableList.of(), baseConfiguredTargetKey); } public static AspectKey createAspectKey( AspectDescriptor aspectDescriptor, ImmutableList baseKeys, - BuildConfigurationKey aspectConfigurationKey, ConfiguredTargetKey baseConfiguredTargetKey) { - return AspectKey.createAspectKey( - baseConfiguredTargetKey, baseKeys, aspectDescriptor, aspectConfigurationKey); - } - - public static AspectKey createAspectKey( - Label label, - @Nullable BuildConfigurationValue baseConfiguration, - AspectDescriptor aspectDescriptor, - @Nullable BuildConfigurationValue aspectConfiguration) { - return AspectKey.createAspectKey( - ConfiguredTargetKey.builder().setLabel(label).setConfiguration(baseConfiguration).build(), - ImmutableList.of(), - aspectDescriptor, - aspectConfiguration == null ? null : aspectConfiguration.getKey()); + return AspectKey.createAspectKey(baseConfiguredTargetKey, baseKeys, aspectDescriptor); } public static TopLevelAspectsKey createTopLevelAspectsKey( @@ -109,19 +84,18 @@ public final int hashCode() { /** Represents an aspect applied to a particular target. */ @AutoCodec public static final class AspectKey extends AspectBaseKey { + private static final Interner interner = BlazeInterners.newWeakInterner(); + private final ImmutableList baseKeys; - @Nullable private final BuildConfigurationKey aspectConfigurationKey; private final AspectDescriptor aspectDescriptor; private AspectKey( ConfiguredTargetKey baseConfiguredTargetKey, ImmutableList baseKeys, AspectDescriptor aspectDescriptor, - @Nullable BuildConfigurationKey aspectConfigurationKey, int hashCode) { super(baseConfiguredTargetKey, hashCode); this.baseKeys = baseKeys; - this.aspectConfigurationKey = aspectConfigurationKey; this.aspectDescriptor = aspectDescriptor; } @@ -130,16 +104,13 @@ private AspectKey( static AspectKey createAspectKey( ConfiguredTargetKey baseConfiguredTargetKey, ImmutableList baseKeys, - AspectDescriptor aspectDescriptor, - @Nullable BuildConfigurationKey aspectConfigurationKey) { - return aspectKeyInterner.intern( + AspectDescriptor aspectDescriptor) { + return interner.intern( new AspectKey( baseConfiguredTargetKey, baseKeys, aspectDescriptor, - aspectConfigurationKey, - Objects.hashCode( - baseConfiguredTargetKey, baseKeys, aspectDescriptor, aspectConfigurationKey))); + Objects.hashCode(baseConfiguredTargetKey, baseKeys, aspectDescriptor))); } @Override @@ -196,7 +167,7 @@ public String getDescription() { @Override @Nullable public BuildConfigurationKey getConfigurationKey() { - return aspectConfigurationKey; + return getBaseConfiguredTargetKey().getConfigurationKey(); } @Override @@ -210,7 +181,6 @@ public boolean equals(Object other) { AspectKey that = (AspectKey) other; return hashCode() == that.hashCode() && Objects.equal(baseKeys, that.baseKeys) - && Objects.equal(aspectConfigurationKey, that.aspectConfigurationKey) && Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey()) && Objects.equal(aspectDescriptor, that.aspectDescriptor); } @@ -232,8 +202,6 @@ public String toString() { + "#" + aspectDescriptor + " " - + aspectConfigurationKey - + " " + getBaseConfiguredTargetKey() + " " + aspectDescriptor.getParameters(); @@ -251,14 +219,15 @@ AspectKey withLabel(Label label) { .setConfigurationKey(getBaseConfiguredTargetKey().getConfigurationKey()) .build(), newBaseKeys.build(), - aspectDescriptor, - aspectConfigurationKey); + aspectDescriptor); } } /** The key for top level aspects specified by --aspects option on a top level target. */ @AutoCodec public static final class TopLevelAspectsKey extends AspectBaseKey { + private static final Interner interner = BlazeInterners.newWeakInterner(); + private final ImmutableList topLevelAspectsClasses; private final Label targetLabel; @@ -268,7 +237,7 @@ static TopLevelAspectsKey createInternal( ImmutableList topLevelAspectsClasses, Label targetLabel, ConfiguredTargetKey baseConfiguredTargetKey) { - return topLevelAspectsKeyInterner.intern( + return interner.intern( new TopLevelAspectsKey( topLevelAspectsClasses, targetLabel, 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 11ed606f4ad47b..a453c248ff234c 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 @@ -1841,15 +1841,15 @@ public ImmutableMultimap getConfiguredTa continue; } for (BuildConfigurationValue depConfig : configs.get(key)) { - skyKeys.add( + ConfiguredTargetKey configuredTargetKey = ConfiguredTargetKey.builder() .setLabel(key.getLabel()) .setConfiguration(depConfig) - .build()); + .build(); + skyKeys.add(configuredTargetKey); for (AspectDeps aspectDeps : key.getAspects().getUsedAspects()) { skyKeys.add( - AspectKeyCreator.createAspectKey( - key.getLabel(), depConfig, aspectDeps.getAspect(), depConfig)); + AspectKeyCreator.createAspectKey(aspectDeps.getAspect(), configuredTargetKey)); } } skyKeys.add(PackageValue.key(key.getLabel().getPackageIdentifier())); @@ -1874,7 +1874,7 @@ public ImmutableMultimap getConfiguredTa continue; } for (BuildConfigurationValue depConfig : configs.get(key)) { - SkyKey configuredTargetKey = + ConfiguredTargetKey configuredTargetKey = ConfiguredTargetKey.builder() .setLabel(key.getLabel()) .setConfiguration(depConfig) @@ -1904,8 +1904,7 @@ public ImmutableMultimap getConfiguredTa for (AspectDeps aspectDeps : key.getAspects().getUsedAspects()) { SkyKey aspectKey = - AspectKeyCreator.createAspectKey( - key.getLabel(), depConfig, aspectDeps.getAspect(), depConfig); + AspectKeyCreator.createAspectKey(aspectDeps.getAspect(), configuredTargetKey); if (result.get(aspectKey) == null) { continue DependentNodeLoop; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelStarlarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelStarlarkAspectFunction.java index 488f48e3f2443c..f5a61ab749c705 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelStarlarkAspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelStarlarkAspectFunction.java @@ -39,7 +39,8 @@ * com.google.devtools.build.lib.analysis.BuildView}, we cannot invoke two SkyFunctions one after * another, so BuildView calls this function to do the work. */ -public class ToplevelStarlarkAspectFunction implements SkyFunction { +public final class ToplevelStarlarkAspectFunction implements SkyFunction { + ToplevelStarlarkAspectFunction() {} @Nullable @@ -102,14 +103,13 @@ private static AspectKey buildAspectKey( AspectKeyCreator.createAspectKey( aspect.getAspectDescriptor(), dependentAspects.build(), - topLevelTargetKey.getConfigurationKey(), topLevelTargetKey); result.put(aspectKey.getAspectDescriptor(), aspectKey); return aspectKey; } /** Exceptions thrown from ToplevelStarlarkAspectFunction. */ - public static class TopLevelStarlarkAspectFunctionException extends SkyFunctionException { + public static final class TopLevelStarlarkAspectFunctionException extends SkyFunctionException { public TopLevelStarlarkAspectFunctionException(AspectCreationException cause) { super(cause, Transience.PERSISTENT); } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectValueTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectValueTest.java index 04455de42d4249..51c4c04f9cb327 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectValueTest.java @@ -27,6 +27,7 @@ import com.google.devtools.build.lib.packages.NativeAspectClass; import com.google.devtools.build.lib.skyframe.AspectKeyCreator; import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey; +import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey; import com.google.devtools.build.skyframe.SkyKey; import org.junit.Test; import org.junit.runner.RunWith; @@ -34,7 +35,7 @@ /** Tests for {@link AspectValue}. */ @RunWith(JUnit4.class) -public class AspectValueTest extends AnalysisTestCase { +public final class AspectValueTest extends AnalysisTestCase { @Test public void keyEquality() throws Exception { @@ -58,174 +59,82 @@ public void keyEquality() throws Exception { ExtraAttributeAspect a2 = TestAspects.EXTRA_ATTRIBUTE_ASPECT; // label: //a:l1 or //a:l2 - // aspectConfiguration: target or host // baseConfiguration: target or host // aspect: Attribute or ExtraAttribute // parameters: bar or baz new EqualsTester() .addEqualityGroup( - createKey(l1, c1, a1, i1, c1), - createKey(l1, c1, a1, i1b, c1), - createKey(l1, c1, a1b, i1, c1), - createKey(l1, c1, a1b, i1b, c1), - createKey(l1b, c1, a1, i1, c1), - createKey(l1b, c1, a1, i1b, c1), - createKey(l1b, c1, a1b, i1, c1), - createKey(l1b, c1, a1b, i1b, c1)) - .addEqualityGroup( - createKey(l1, c1, a1, i2, c1), - createKey(l1, c1, a1b, i2, c1), - createKey(l1b, c1, a1, i2, c1), - createKey(l1b, c1, a1b, i2, c1)) - .addEqualityGroup( - createKey(l1, c1, a2, i1, c1), - createKey(l1, c1, a2, i1b, c1), - createKey(l1b, c1, a2, i1, c1), - createKey(l1b, c1, a2, i1b, c1)) - .addEqualityGroup( - createKey(l1, c1, a2, i2, c1), - createKey(l1b, c1, a2, i2, c1)) - .addEqualityGroup( - createKey(l1, c2, a1, i1, c1), - createKey(l1, c2, a1, i1b, c1), - createKey(l1, c2, a1b, i1, c1), - createKey(l1, c2, a1b, i1b, c1), - createKey(l1b, c2, a1, i1, c1), - createKey(l1b, c2, a1, i1b, c1), - createKey(l1b, c2, a1b, i1, c1), - createKey(l1b, c2, a1b, i1b, c1)) - .addEqualityGroup( - createKey(l1, c2, a1, i2, c1), - createKey(l1, c2, a1b, i2, c1), - createKey(l1b, c2, a1, i2, c1), - createKey(l1b, c2, a1b, i2, c1)) - .addEqualityGroup( - createKey(l1, c2, a2, i1, c1), - createKey(l1, c2, a2, i1b, c1), - createKey(l1b, c2, a2, i1, c1), - createKey(l1b, c2, a2, i1b, c1)) - .addEqualityGroup( - createKey(l1, c2, a2, i2, c1), - createKey(l1b, c2, a2, i2, c1)) - .addEqualityGroup( - createKey(l1, c1, a1, i1, c2), - createKey(l1, c1, a1, i1b, c2), - createKey(l1, c1, a1b, i1, c2), - createKey(l1, c1, a1b, i1b, c2), - createKey(l1b, c1, a1, i1, c2), - createKey(l1b, c1, a1, i1b, c2), - createKey(l1b, c1, a1b, i1, c2), - createKey(l1b, c1, a1b, i1b, c2)) - .addEqualityGroup( - createKey(l1, c1, a1, i2, c2), - createKey(l1, c1, a1b, i2, c2), - createKey(l1b, c1, a1, i2, c2), - createKey(l1b, c1, a1b, i2, c2)) - .addEqualityGroup( - createKey(l1, c1, a2, i1, c2), - createKey(l1, c1, a2, i1b, c2), - createKey(l1b, c1, a2, i1, c2), - createKey(l1b, c1, a2, i1b, c2)) - .addEqualityGroup( - createKey(l1, c1, a2, i2, c2), - createKey(l1b, c1, a2, i2, c2)) - .addEqualityGroup( - createKey(l1, c2, a1, i1, c2), - createKey(l1, c2, a1, i1b, c2), - createKey(l1, c2, a1b, i1, c2), - createKey(l1, c2, a1b, i1b, c2), - createKey(l1b, c2, a1, i1, c2), - createKey(l1b, c2, a1, i1b, c2), - createKey(l1b, c2, a1b, i1, c2), - createKey(l1b, c2, a1b, i1b, c2)) - .addEqualityGroup( - createKey(l1, c2, a1, i2, c2), - createKey(l1, c2, a1b, i2, c2), - createKey(l1b, c2, a1, i2, c2), - createKey(l1b, c2, a1b, i2, c2)) - .addEqualityGroup( - createKey(l1, c2, a2, i1, c2), - createKey(l1, c2, a2, i1b, c2), - createKey(l1b, c2, a2, i1, c2), - createKey(l1b, c2, a2, i1b, c2)) - .addEqualityGroup( - createKey(l1, c2, a2, i2, c2), - createKey(l1b, c2, a2, i2, c2)) - .addEqualityGroup( - createKey(l2, c1, a1, i1, c1), - createKey(l2, c1, a1, i1b, c1), - createKey(l2, c1, a1b, i1, c1), - createKey(l2, c1, a1b, i1b, c1)) - .addEqualityGroup( - createKey(l2, c1, a1, i2, c1), - createKey(l2, c1, a1b, i2, c1)) - .addEqualityGroup( - createKey(l2, c1, a2, i1, c1), - createKey(l2, c1, a2, i1b, c1)) - .addEqualityGroup( - createKey(l2, c1, a2, i2, c1)) - .addEqualityGroup( - createKey(l2, c2, a1, i1, c1), - createKey(l2, c2, a1, i1b, c1), - createKey(l2, c2, a1b, i1, c1), - createKey(l2, c2, a1b, i1b, c1)) - .addEqualityGroup( - createKey(l2, c2, a1, i2, c1), - createKey(l2, c2, a1b, i2, c1)) - .addEqualityGroup( - createKey(l2, c2, a2, i1, c1), - createKey(l2, c2, a2, i1b, c1)) - .addEqualityGroup( - createKey(l2, c2, a2, i2, c1)) - .addEqualityGroup( - createKey(l2, c1, a1, i1, c2), - createKey(l2, c1, a1, i1b, c2), - createKey(l2, c1, a1b, i1, c2), - createKey(l2, c1, a1b, i1b, c2)) - .addEqualityGroup( - createKey(l2, c1, a1, i2, c2), - createKey(l2, c1, a1b, i2, c2)) - .addEqualityGroup( - createKey(l2, c1, a2, i1, c2), - createKey(l2, c1, a2, i1b, c2)) - .addEqualityGroup( - createKey(l2, c1, a2, i2, c2)) - .addEqualityGroup( - createKey(l2, c2, a1, i1, c2), - createKey(l2, c2, a1, i1b, c2), - createKey(l2, c2, a1b, i1, c2), - createKey(l2, c2, a1b, i1b, c2)) - .addEqualityGroup( - createKey(l2, c2, a1, i2, c2), - createKey(l2, c2, a1b, i2, c2)) - .addEqualityGroup( - createKey(l2, c2, a2, i1, c2), - createKey(l2, c2, a2, i1b, c2)) - .addEqualityGroup( - createKey(l2, c2, a2, i2, c2)) - .addEqualityGroup( - createDerivedKey(l1, c1, a1, i1, c1, a2, i2, c2), - createDerivedKey(l1, c1, a1, i1b, c1, a2, i2, c2) - ) - .addEqualityGroup( - createDerivedKey(l1, c1, a2, i1, c1, a1, i2, c2), - createDerivedKey(l1, c1, a2, i1b, c1, a1, i2, c2) - ) + createKey(l1, c1, a1, i1), + createKey(l1, c1, a1, i1b), + createKey(l1, c1, a1b, i1), + createKey(l1, c1, a1b, i1b), + createKey(l1b, c1, a1, i1), + createKey(l1b, c1, a1, i1b), + createKey(l1b, c1, a1b, i1), + createKey(l1b, c1, a1b, i1b)) + .addEqualityGroup( + createKey(l1, c1, a1, i2), + createKey(l1, c1, a1b, i2), + createKey(l1b, c1, a1, i2), + createKey(l1b, c1, a1b, i2)) + .addEqualityGroup( + createKey(l1, c1, a2, i1), + createKey(l1, c1, a2, i1b), + createKey(l1b, c1, a2, i1), + createKey(l1b, c1, a2, i1b)) + .addEqualityGroup(createKey(l1, c1, a2, i2), createKey(l1b, c1, a2, i2)) + .addEqualityGroup( + createKey(l1, c2, a1, i1), + createKey(l1, c2, a1, i1b), + createKey(l1, c2, a1b, i1), + createKey(l1, c2, a1b, i1b), + createKey(l1b, c2, a1, i1), + createKey(l1b, c2, a1, i1b), + createKey(l1b, c2, a1b, i1), + createKey(l1b, c2, a1b, i1b)) + .addEqualityGroup( + createKey(l1, c2, a1, i2), + createKey(l1, c2, a1b, i2), + createKey(l1b, c2, a1, i2), + createKey(l1b, c2, a1b, i2)) + .addEqualityGroup( + createKey(l1, c2, a2, i1), + createKey(l1, c2, a2, i1b), + createKey(l1b, c2, a2, i1), + createKey(l1b, c2, a2, i1b)) + .addEqualityGroup(createKey(l1, c2, a2, i2), createKey(l1b, c2, a2, i2)) + .addEqualityGroup( + createKey(l2, c1, a1, i1), + createKey(l2, c1, a1, i1b), + createKey(l2, c1, a1b, i1), + createKey(l2, c1, a1b, i1b)) + .addEqualityGroup(createKey(l2, c1, a1, i2), createKey(l2, c1, a1b, i2)) + .addEqualityGroup(createKey(l2, c1, a2, i1), createKey(l2, c1, a2, i1b)) + .addEqualityGroup(createKey(l2, c1, a2, i2)) + .addEqualityGroup( + createKey(l2, c2, a1, i1), + createKey(l2, c2, a1, i1b), + createKey(l2, c2, a1b, i1), + createKey(l2, c2, a1b, i1b)) + .addEqualityGroup(createKey(l2, c2, a1, i2), createKey(l2, c2, a1b, i2)) + .addEqualityGroup(createKey(l2, c2, a2, i1), createKey(l2, c2, a2, i1b)) + .addEqualityGroup(createKey(l2, c2, a2, i2)) + .addEqualityGroup( + createDerivedKey(l1, c1, a1, i1, a2, i2), createDerivedKey(l1, c1, a1, i1b, a2, i2)) + .addEqualityGroup( + createDerivedKey(l1, c1, a2, i1, a1, i2), createDerivedKey(l1, c1, a2, i1b, a1, i2)) .testEquals(); } - private static SkyKey createKey( + private static AspectKey createKey( Label label, BuildConfigurationValue baseConfiguration, NativeAspectClass aspectClass, - AspectParameters parameters, - BuildConfigurationValue aspectConfiguration) { + AspectParameters parameters) { return AspectKeyCreator.createAspectKey( - label, - baseConfiguration, new AspectDescriptor(aspectClass, parameters), - aspectConfiguration); + ConfiguredTargetKey.builder().setLabel(label).setConfiguration(baseConfiguration).build()); } private static SkyKey createDerivedKey( @@ -233,22 +142,12 @@ private static SkyKey createDerivedKey( BuildConfigurationValue baseConfiguration, NativeAspectClass aspectClass1, AspectParameters parameters1, - BuildConfigurationValue aspectConfiguration1, NativeAspectClass aspectClass2, - AspectParameters parameters2, - BuildConfigurationValue aspectConfiguration2) { - AspectKey baseKey = - AspectKeyCreator.createAspectKey( - label, - baseConfiguration, - new AspectDescriptor(aspectClass1, parameters1), - aspectConfiguration1); + AspectParameters parameters2) { + AspectKey baseKey = createKey(label, baseConfiguration, aspectClass1, parameters1); return AspectKeyCreator.createAspectKey( - label, - baseConfiguration, - ImmutableList.of(baseKey), new AspectDescriptor(aspectClass2, parameters2), - aspectConfiguration2); + ImmutableList.of(baseKey), + ConfiguredTargetKey.builder().setLabel(label).setConfiguration(baseConfiguration).build()); } - } diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DependencyTest.java index bfc2a0b0ef45e2..40c9007d22d9e5 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/DependencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/DependencyTest.java @@ -36,7 +36,8 @@ *

Although this is just a data class, we need a way to create a configuration. */ @RunWith(JUnit4.class) -public class DependencyTest extends AnalysisTestCase { +public final class DependencyTest extends AnalysisTestCase { + @Test public void withNullConfiguration_BasicAccessors() throws Exception { Dependency nullDep = @@ -81,15 +82,10 @@ public void withConfigurationAndAspects_BasicAccessors() throws Exception { assertThat(targetDep.getLabel()).isEqualTo(Label.parseAbsolute("//a", ImmutableMap.of())); assertThat(targetDep.getConfiguration()).isEqualTo(getTargetConfiguration()); assertThat(targetDep.getAspects()).isEqualTo(twoAspects); - assertThat(targetDep.getAspectConfiguration(simpleAspect)).isEqualTo(getTargetConfiguration()); - assertThat(targetDep.getAspectConfiguration(attributeAspect)) - .isEqualTo(getTargetConfiguration()); } @Test - public void withConfigurationAndAspects_RejectsNullConfig() throws Exception { - // Although the NullPointerTester should check this, this test invokes a different code path, - // because it includes aspects (which the NPT test will not). + public void withConfigurationAndAspects_rejectsNullConfig() { AspectDescriptor simpleAspect = new AspectDescriptor(TestAspects.SIMPLE_ASPECT); AspectDescriptor attributeAspect = new AspectDescriptor(TestAspects.ATTRIBUTE_ASPECT); AspectCollection twoAspects = AspectCollection.createForTests(simpleAspect, attributeAspect); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java index a691c2b18d81d7..21196b0be467f8 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java @@ -156,7 +156,7 @@ public BuildViewForTesting( this.skyframeBuildView = skyframeExecutor.getSkyframeBuildView(); } - public Set getSkyframeEvaluatedActionLookupKeyCountForTesting() { + Set getSkyframeEvaluatedActionLookupKeyCountForTesting() { Set actionLookupKeys = populateActionLookupKeyMapAndGetDiff(); Preconditions.checkState( actionLookupKeys.size() == skyframeBuildView.getEvaluatedCounts().total(), @@ -360,16 +360,17 @@ protected static ConfiguredTargetAndData mergeAspects( return ctd; } - // Collect the aspects. + ConfiguredTargetKey ctKey = + ConfiguredTargetKey.builder() + .setLabel(dependencyKey.getLabel()) + .setConfiguration(ctd.getConfiguration()) + .build(); + List aspectKeys = + dependencyKey.getAspects().getUsedAspects().stream() + .map(aspect -> AspectKeyCreator.createAspectKey(aspect.getAspect(), ctKey)) + .collect(toImmutableList()); + try { - BuildConfigurationValue config = ctd.getConfiguration(); - List aspectKeys = - dependencyKey.getAspects().getUsedAspects().stream() - .map( - aspect -> - AspectKeyCreator.createAspectKey( - dependencyKey.getLabel(), config, aspect.getAspect(), config)) - .collect(toImmutableList()); ImmutableList configuredAspects = graph.getSuccessfulValues(aspectKeys).values().stream() .map(value -> (AspectValue) value) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java index 4f9fe4058b0205..1248a2912070a3 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java @@ -874,7 +874,7 @@ protected final String paramFileStringContentsForAction(Action action) pair.commandLine.arguments(), pair.paramFileInfo.getFileType(), pair.paramFileInfo.getCharset()); - return new String(out.toByteArray(), pair.paramFileInfo.getCharset()); + return out.toString(pair.paramFileInfo.getCharset()); } } } @@ -1521,13 +1521,12 @@ protected Artifact getBinArtifact( return getPackageRelativeDerivedArtifact( packageRelativePath, getConfiguration(owner).getBinDirectory(RepositoryName.MAIN), - (AspectKey) - AspectKeyCreator.createAspectKey( - owner.getLabel(), - getConfiguration(owner), - new AspectDescriptor(creatingAspectFactory, parameters), - getConfiguration(owner)) - .argument()); + AspectKeyCreator.createAspectKey( + new AspectDescriptor(creatingAspectFactory, parameters), + ConfiguredTargetKey.builder() + .setLabel(owner.getLabel()) + .setConfiguration(getConfiguration(owner)) + .build())); } /** @@ -1618,13 +1617,12 @@ private Artifact getGenfilesArtifact( protected AspectKey getOwnerForAspect( ConfiguredTarget owner, NativeAspectClass creatingAspectFactory, AspectParameters params) { - return (AspectKey) - AspectKeyCreator.createAspectKey( - owner.getLabel(), - getConfiguration(owner), - new AspectDescriptor(creatingAspectFactory, params), - getConfiguration(owner)) - .argument(); + return AspectKeyCreator.createAspectKey( + new AspectDescriptor(creatingAspectFactory, params), + ConfiguredTargetKey.builder() + .setLabel(owner.getLabel()) + .setConfiguration(getConfiguration(owner)) + .build()); } /** @@ -2293,7 +2291,7 @@ protected Iterable baselineCoverageArtifactBasenames(ConfiguredTarget ta .newDeterministicWriter(ActionsTestUtil.createContext(reporter)) .writeOutputFile(bytes); - for (String line : Splitter.on('\n').split(new String(bytes.toByteArray(), UTF_8))) { + for (String line : Splitter.on('\n').split(bytes.toString(UTF_8))) { if (line.startsWith("SF:")) { String basename = line.substring(line.lastIndexOf('/') + 1); basenames.add(basename); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java index 53585e072c8f28..535edc397df71a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TargetCycleReporterTest.java @@ -27,7 +27,7 @@ /** Tests for {@link TargetCycleReporter}. */ @RunWith(JUnit4.class) -public class TargetCycleReporterTest extends BuildViewTestCase { +public final class TargetCycleReporterTest extends BuildViewTestCase { /** * Regression test for b/142966884 : Blaze crashes when building with --aspects and --keep_going @@ -62,9 +62,7 @@ public void loadingPhaseCycleWithDifferentTopLevelKeyTypes() throws Exception { "The cycle is caused by a visibility edge from //foo:b to the non-package_group " + "target //foo:c"); - SkyKey aspectKey = - AspectKeyCreator.AspectKey.createAspectKey( - ctKey, ImmutableList.of(), null, targetConfig.getKey()); + SkyKey aspectKey = AspectKeyCreator.createAspectKey(null, ctKey); assertThat(cycleReporter.getAdditionalMessageAboutCycle(reporter, aspectKey, cycle)) .contains( "The cycle is caused by a visibility edge from //foo:b to the non-package_group "