Skip to content

Commit

Permalink
Add safety checks for aspects.
Browse files Browse the repository at this point in the history
As aspects are not currently supported in retroactive trimming mode, these
checks ensure that they are not silently executed incorrectly.

Progress on #6524.

PiperOrigin-RevId: 244946454
  • Loading branch information
mstaib authored and meisterT committed Apr 24, 2019
1 parent 8c094bf commit 6c224b0
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,11 @@ public AnalysisResult update(

String skylarkFunctionName = aspect.substring(delimiterPosition + 1);
for (TargetAndConfiguration targetSpec : topLevelTargetsWithConfigs) {
if (targetSpec.getConfiguration() != null
&& targetSpec.getConfiguration().trimConfigurationsRetroactively()) {
throw new ViewCreationFailedException(
"Aspects were requested, but are not supported in retroactive trimming mode.");
}
aspectConfigurations.put(
Pair.of(targetSpec.getLabel(), aspect), targetSpec.getConfiguration());
aspectKeys.add(
Expand All @@ -342,6 +347,11 @@ public AnalysisResult update(

if (aspectFactoryClass != null) {
for (TargetAndConfiguration targetSpec : topLevelTargetsWithConfigs) {
if (targetSpec.getConfiguration() != null
&& targetSpec.getConfiguration().trimConfigurationsRetroactively()) {
throw new ViewCreationFailedException(
"Aspects were requested, but are not supported in retroactive trimming mode.");
}
// For invoking top-level aspects, use the top-level configuration for both the
// aspect and the base target while the top-level configuration is untrimmed.
BuildConfiguration configuration = targetSpec.getConfiguration();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import com.google.devtools.build.lib.packages.Attribute;
import com.google.devtools.build.lib.packages.RuleClassProvider;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.lib.skyframe.BuildConfigurationValue;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetFunction;
import com.google.devtools.build.lib.skyframe.ConfiguredTargetValue;
Expand Down Expand Up @@ -197,6 +198,17 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio

if (sameFragments) {
if (transition == NoTransition.INSTANCE) {
if (ctgValue.getConfiguration().trimConfigurationsRetroactively()
&& !dep.getAspects().isEmpty()) {
String message =
ctgValue.getLabel()
+ " has aspects attached, but these are not supported in retroactive"
+ " trimming mode.";
env.getListener()
.handle(Event.error(TargetUtils.getLocationMaybe(ctgValue.getTarget()), message));
throw new ConfiguredTargetFunction.DependencyEvaluationException(
new InvalidConfigurationException(message));
}
// The dep uses the same exact configuration. Let's re-use the current configuration and
// skip adding a Skyframe dependency edge on it.
putOnlyEntry(
Expand All @@ -211,6 +223,16 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio
// uniquely frequent. It's possible, e.g., for every node in the configured target graph
// to incur multiple host transitions. So we aggressively optimize to avoid hurting
// analysis time.
if (hostConfiguration.trimConfigurationsRetroactively() && !dep.getAspects().isEmpty()) {
String message =
ctgValue.getLabel()
+ " has aspects attached, but these are not supported in retroactive"
+ " trimming mode.";
env.getListener()
.handle(Event.error(TargetUtils.getLocationMaybe(ctgValue.getTarget()), message));
throw new ConfiguredTargetFunction.DependencyEvaluationException(
new InvalidConfigurationException(message));
}
putOnlyEntry(
resolvedDeps,
dependencyEdge,
Expand Down Expand Up @@ -256,6 +278,17 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio
if (sameFragments
&& toOptions.size() == 1
&& Iterables.getOnlyElement(toOptions).equals(currentConfiguration.getOptions())) {
if (ctgValue.getConfiguration().trimConfigurationsRetroactively()
&& !dep.getAspects().isEmpty()) {
String message =
ctgValue.getLabel()
+ " has aspects attached, but these are not supported in retroactive"
+ " trimming mode.";
env.getListener()
.handle(Event.error(TargetUtils.getLocationMaybe(ctgValue.getTarget()), message));
throw new ConfiguredTargetFunction.DependencyEvaluationException(
new InvalidConfigurationException(message));
}
putOnlyEntry(
resolvedDeps,
dependencyEdge,
Expand Down Expand Up @@ -330,12 +363,25 @@ public static OrderedSetMultimap<DependencyKind, Dependency> resolveConfiguratio
// null out on missing values from *this specific Skyframe request*.
return null;
}
BuildConfigurationValue trimmedConfig = (BuildConfigurationValue) valueOrException.get();
BuildConfiguration trimmedConfig =
((BuildConfigurationValue) valueOrException.get()).getConfiguration();
for (Map.Entry<DependencyKind, Dependency> info : keysToEntries.get(key)) {
Dependency originalDep = info.getValue();
if (trimmedConfig.trimConfigurationsRetroactively()
&& !originalDep.getAspects().isEmpty()) {
String message =
ctgValue.getLabel()
+ " has aspects attached, but these are not supported in retroactive"
+ " trimming mode.";
env.getListener()
.handle(Event.error(TargetUtils.getLocationMaybe(ctgValue.getTarget()), message));
throw new ConfiguredTargetFunction.DependencyEvaluationException(
new InvalidConfigurationException(message));
}
DependencyEdge attr = new DependencyEdge(info.getKey(), originalDep.getLabel());
Dependency resolvedDep = Dependency.withConfigurationAndAspects(originalDep.getLabel(),
trimmedConfig.getConfiguration(), originalDep.getAspects());
Dependency resolvedDep =
Dependency.withConfigurationAndAspects(
originalDep.getLabel(), trimmedConfig, originalDep.getAspects());
Attribute attribute = attr.dependencyKind.getAttribute();
if (attribute != null && attribute.getTransitionFactory().isSplit()) {
resolvedDeps.put(attr, resolvedDep);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
throw new IllegalStateException("Unexpected exception from BuildConfigurationFunction when "
+ "computing " + key.getAspectConfigurationKey(), e);
}
if (aspectConfiguration.trimConfigurationsRetroactively()) {
throw new AssertionError("Aspects should NEVER be evaluated in retroactive trimming mode.");
}
}

ConfiguredTarget associatedTarget = baseConfiguredTargetValue.getConfiguredTarget();
Expand Down Expand Up @@ -320,6 +323,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
configuration =
((BuildConfigurationValue) result.get(associatedTarget.getConfigurationKey()))
.getConfiguration();
if (configuration.trimConfigurationsRetroactively()) {
throw new AssertionError("Aspects should NEVER be evaluated in retroactive trimming mode.");
}
}
try {
associatedConfiguredTargetAndData =
Expand Down

0 comments on commit 6c224b0

Please sign in to comment.