Skip to content

Commit

Permalink
Remove aspect configuration keys and clean up AspectKey factory met…
Browse files Browse the repository at this point in the history 
…hods.

This appears to be left over from the dynamic configs prototype. Aspects now always have the same configuration as their associated target, so there is no need to store two configuration keys.

PiperOrigin-RevId: 410962427
  • Loading branch information
@justinhorvitz
justinhorvitz authored and Copybara-Service committed Nov 19, 2021
1 parent 9b0ba60 commit 564a689
Show file tree
Hide file tree
Showing 11 changed files with 188 additions and 358 deletions.
6 changes: 1 addition & 5 deletions src/main/java/com/google/devtools/build/lib/analysis/AspectResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 0 additions & 6 deletions src/main/java/com/google/devtools/build/lib/analysis/Dependency.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
152 changes: 66 additions & 86 deletions src/main/java/com/google/devtools/build/lib/skyframe/AspectFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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 {
Expand All @@ -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<SkyKey> keys =
configurationKey == null
? ImmutableSet.of(packageKey, key.getBaseConfiguredTargetKey())
: ImmutableSet.of(packageKey, key.getBaseConfiguredTargetKey(), configurationKey);

List<ValueOrException<ConfiguredValueCreationException>> 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<SkyKey> keys =
aspectHasConfiguration
? ImmutableSet.of(key.getBaseConfiguredTargetKey(), key.getConfigurationKey())
: ImmutableSet.of(key.getBaseConfiguredTargetKey());

Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> 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<SkyKey, SkyValue> 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 {
Expand All @@ -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
Expand Down Expand Up @@ -332,9 +324,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
aspect,
target.getLocation(),
ConfiguredAspect.forNonapplicableTarget(),
/*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder
.<Package>stableOrder()
.build());
/*transitivePackagesForPackageRootResolution=*/ NestedSetBuilder.emptySet(
Order.STABLE_ORDER));
}
}
}
Expand All @@ -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(
Expand All @@ -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 =
Expand All @@ -378,15 +368,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
NestedSetBuilder<Package> 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<Aspect> aspectPath = aspectPathBuilder.build();
try {
UnloadedToolchainContext unloadedToolchainContext =
Expand All @@ -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);
Expand All @@ -414,7 +397,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
ConfiguredTargetFunction.computeDependencies(
env,
resolver,
originalTargetAndAspectConfiguration,
originalTargetAndConfiguration,
aspectPath,
configConditions.asProviders(),
unloadedToolchainContext == null
Expand All @@ -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;
Expand Down Expand Up @@ -466,7 +449,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
aspect,
aspectFactory,
associatedConfiguredTargetAndData,
aspectConfiguration,
configuration,
configConditions,
toolchainContext,
depValueMap,
Expand All @@ -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);
Expand Down Expand Up @@ -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());
}

/**
Expand Down Expand Up @@ -828,7 +809,7 @@ private AspectValue createAspect(
Aspect aspect,
ConfiguredAspectFactory aspectFactory,
ConfiguredTargetAndData associatedTarget,
BuildConfigurationValue aspectConfiguration,
BuildConfigurationValue configuration,
ConfigConditions configConditions,
ResolvedToolchainContext toolchainContext,
OrderedSetMultimap<DependencyKind, ConfiguredTargetAndData> directDeps,
Expand All @@ -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()
Expand Down Expand Up @@ -874,7 +854,7 @@ private AspectValue createAspect(
directDeps,
configConditions,
toolchainContext,
aspectConfiguration,
configuration,
view.getHostConfiguration(),
key);
} catch (MissingDepException e) {
Expand All @@ -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");
Expand Down
Loading

0 comments on commit 564a689

Please sign in to comment.