Report inconsistent aspect order error to the user.
--
PiperOrigin-RevId: 148342788
MOS_MIGRATED_REVID=148342788
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 015fea9..77cefb5 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
@@ -35,6 +35,7 @@
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactOwner;
import com.google.devtools.build.lib.actions.Root;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
@@ -962,7 +963,8 @@
@VisibleForTesting
public Iterable<ConfiguredTarget> getDirectPrerequisitesForTesting(
EventHandler eventHandler, ConfiguredTarget ct, BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException,
+ InterruptedException, InconsistentAspectOrderException {
return skyframeExecutor.getConfiguredTargets(
eventHandler, ct.getConfiguration(),
ImmutableSet.copyOf(
@@ -974,7 +976,8 @@
public OrderedSetMultimap<Attribute, Dependency> getDirectPrerequisiteDependenciesForTesting(
final EventHandler eventHandler, final ConfiguredTarget ct,
BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
if (!(ct.getTarget() instanceof Rule)) {
return OrderedSetMultimap.create();
}
@@ -1062,7 +1065,8 @@
private OrderedSetMultimap<Attribute, ConfiguredTarget> getPrerequisiteMapForTesting(
final EventHandler eventHandler, ConfiguredTarget target,
BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException,
+ InterruptedException, InconsistentAspectOrderException {
OrderedSetMultimap<Attribute, Dependency> depNodeNames =
getDirectPrerequisiteDependenciesForTesting(eventHandler, target, configurations);
@@ -1094,7 +1098,8 @@
public RuleContext getRuleContextForTesting(
ConfiguredTarget target, StoredEventHandler eventHandler,
BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
BuildConfiguration targetConfig = target.getConfiguration();
CachingAnalysisEnvironment env =
new CachingAnalysisEnvironment(getArtifactFactory(),
@@ -1111,7 +1116,8 @@
@VisibleForTesting
public RuleContext getRuleContextForTesting(EventHandler eventHandler, ConfiguredTarget target,
AnalysisEnvironment env, BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
BuildConfiguration targetConfig = target.getConfiguration();
return new RuleContext.Builder(
env,
@@ -1139,7 +1145,8 @@
public ConfiguredTarget getPrerequisiteConfiguredTargetForTesting(
EventHandler eventHandler, ConfiguredTarget dependentTarget, Label desiredTarget,
BuildConfigurationCollection configurations)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
Collection<ConfiguredTarget> configuredTargets =
getPrerequisiteMapForTesting(eventHandler, dependentTarget, configurations).values();
for (ConfiguredTarget ct : configuredTargets) {
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 06d561b..440e7b5 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
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.analysis.config.PatchTransition;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.AspectDescriptor;
@@ -91,7 +92,8 @@
BuildConfiguration hostConfig,
@Nullable Aspect aspect,
ImmutableMap<Label, ConfigMatchingProvider> configConditions)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
NestedSetBuilder<Label> rootCauses = NestedSetBuilder.<Label>stableOrder();
OrderedSetMultimap<Attribute, Dependency> outgoingEdges = dependentNodeMap(
node, hostConfig,
@@ -138,7 +140,8 @@
Iterable<Aspect> aspects,
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
NestedSetBuilder<Label> rootCauses)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InterruptedException,
+ InconsistentAspectOrderException {
Target target = node.getTarget();
BuildConfiguration config = node.getConfiguration();
OrderedSetMultimap<Attribute, Dependency> outgoingEdges = OrderedSetMultimap.create();
@@ -158,6 +161,7 @@
} else {
throw new IllegalStateException(target.getLabel().toString());
}
+
return outgoingEdges;
}
@@ -168,7 +172,8 @@
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
NestedSetBuilder<Label> rootCauses,
OrderedSetMultimap<Attribute, Dependency> outgoingEdges)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InconsistentAspectOrderException,
+ InterruptedException{
Preconditions.checkArgument(node.getTarget() instanceof Rule);
BuildConfiguration ruleConfig = Preconditions.checkNotNull(node.getConfiguration());
Rule rule = (Rule) node.getTarget();
@@ -188,7 +193,7 @@
* (which require special processing: see {@link #resolveLateBoundAttributes}).
*/
private void resolveEarlyBoundAttributes(RuleResolver depResolver)
- throws EvalException, InterruptedException {
+ throws EvalException, InterruptedException, InconsistentAspectOrderException {
Rule rule = depResolver.rule;
resolveExplicitAttributes(depResolver);
@@ -231,7 +236,11 @@
}
private void resolveExplicitAttributes(final RuleResolver depResolver)
- throws InterruptedException {
+ throws InterruptedException, InconsistentAspectOrderException {
+
+ // Record error that might happen during label visitation.
+ final InconsistentAspectOrderException[] exception = new InconsistentAspectOrderException[1];
+
depResolver.attributeMap.visitLabels(
new AttributeMap.AcceptsLabelAttribute() {
@Override
@@ -242,13 +251,24 @@
|| attribute.isLateBound()) {
return;
}
- depResolver.resolveDep(new AttributeAndOwner(attribute), label);
+ try {
+ depResolver.resolveDep(new AttributeAndOwner(attribute), label);
+ } catch (InconsistentAspectOrderException e) {
+ if (exception[0] == null) {
+ exception[0] = e;
+ }
+ }
}
});
+
+ if (exception[0] != null) {
+ throw exception[0];
+ }
}
/** Resolves the dependencies for all implicit attributes in this rule. */
- private void resolveImplicitAttributes(RuleResolver depResolver) throws InterruptedException {
+ private void resolveImplicitAttributes(RuleResolver depResolver)
+ throws InterruptedException, InconsistentAspectOrderException {
// Since the attributes that come from aspects do not appear in attributeMap, we have to get
// their values from somewhere else. This incidentally means that aspects attributes are not
// configurable. It would be nice if that wasn't the case, but we'd have to revamp how
@@ -318,7 +338,8 @@
RuleResolver depResolver,
BuildConfiguration ruleConfig,
BuildConfiguration hostConfig)
- throws EvalException, InvalidConfigurationException, InterruptedException {
+ throws EvalException, InvalidConfigurationException, InconsistentAspectOrderException,
+ InterruptedException{
ConfiguredAttributeMapper attributeMap = depResolver.attributeMap;
for (AttributeAndOwner attributeAndOwner : depResolver.attributes) {
Attribute attribute = attributeAndOwner.attribute;
@@ -459,7 +480,7 @@
* @param labels the dependencies to add
*/
private void addExplicitDeps(RuleResolver depResolver, String attrName, Iterable<Label> labels)
- throws InterruptedException {
+ throws InterruptedException, InconsistentAspectOrderException {
Rule rule = depResolver.rule;
if (!rule.isAttrDefined(attrName, BuildType.LABEL_LIST)
&& !rule.isAttrDefined(attrName, BuildType.NODEP_LABEL_LIST)) {
@@ -481,7 +502,7 @@
TargetAndConfiguration node,
OrderedSetMultimap<Attribute, Label> depLabels,
NestedSetBuilder<Label> rootCauses)
- throws InterruptedException {
+ throws InterruptedException, InconsistentAspectOrderException {
Preconditions.checkArgument(node.getTarget() instanceof Rule);
Rule rule = (Rule) node.getTarget();
OrderedSetMultimap<Attribute, Dependency> outgoingEdges = OrderedSetMultimap.create();
@@ -521,11 +542,11 @@
}
- private static AspectCollection requiredAspects(
+ private AspectCollection requiredAspects(
Iterable<Aspect> aspectPath,
AttributeAndOwner attributeAndOwner,
final Target target,
- Rule originalRule) {
+ Rule originalRule) throws InconsistentAspectOrderException {
if (!(target instanceof Rule)) {
return AspectCollection.EMPTY;
}
@@ -538,17 +559,17 @@
ImmutableList.Builder<Aspect> filteredAspectPath = ImmutableList.builder();
ImmutableSet.Builder<AspectDescriptor> visibleAspects = ImmutableSet.builder();
- collectOriginatingAspects(originalRule, attributeAndOwner.attribute, (Rule) target,
+ Attribute attribute = attributeAndOwner.attribute;
+ collectOriginatingAspects(originalRule, attribute, (Rule) target,
filteredAspectPath, visibleAspects);
collectPropagatingAspects(aspectPath,
- attributeAndOwner.attribute,
+ 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);
+ throw new InconsistentAspectOrderException(originalRule, attribute, target, e);
}
}
@@ -603,19 +624,6 @@
}
}
- private static Iterable<Aspect> extractAspectCandidates(
- Iterable<Aspect> aspects,
- Attribute attribute, Rule originalRule) {
- ImmutableList.Builder<Aspect> aspectCandidates = ImmutableList.builder();
- aspectCandidates.addAll(attribute.getAspects(originalRule));
- for (Aspect aspect : aspects) {
- if (aspect.getDefinition().propagateAlong(attribute)) {
- aspectCandidates.add(aspect);
- }
- }
- return aspectCandidates.build();
- }
-
/**
* Pair of (attribute, owner aspect if attribute is from an aspect).
*
@@ -700,7 +708,7 @@
* apply to it.
*/
void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel)
- throws InterruptedException {
+ throws InterruptedException, InconsistentAspectOrderException {
Target toTarget = getTarget(rule, depLabel, rootCauses);
if (toTarget == null) {
return; // Skip this round: we still need to Skyframe-evaluate the dep's target.
@@ -725,7 +733,7 @@
* #resolveDep(AttributeAndOwner, Label)}.
*/
void resolveDep(AttributeAndOwner attributeAndOwner, Label depLabel, BuildConfiguration config)
- throws InterruptedException {
+ throws InterruptedException, InconsistentAspectOrderException {
Target toTarget = getTarget(rule, depLabel, rootCauses);
if (toTarget == null) {
return; // Skip this round: this is either a loading error or unevaluated Skyframe dep.
@@ -852,4 +860,27 @@
Set<Class<? extends BuildConfiguration.Fragment>> fragments,
Iterable<BuildOptions> buildOptions)
throws InvalidConfigurationException, InterruptedException;
+
+ /**
+ * Signals an inconsistency on aspect path: an aspect occurs twice on the path and
+ * the second occurrence sees a different set of aspects.
+ *
+ * {@see AspectCycleOnPathException}
+ */
+ public class InconsistentAspectOrderException extends Exception {
+ private final Location location;
+ public InconsistentAspectOrderException(Rule originalRule, Attribute attribute, Target target,
+ AspectCycleOnPathException e) {
+ super(String.format("%s (when propagating from %s to %s via attribute %s)",
+ e.getMessage(),
+ originalRule.getLabel(),
+ target.getLabel(),
+ attribute.getName()));
+ this.location = originalRule.getLocation();
+ }
+
+ public Location getLocation() {
+ return location;
+ }
+ }
}
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 3d5aa88..71f6912 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
@@ -21,6 +21,7 @@
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
@@ -304,6 +305,10 @@
ConfiguredValueCreationException cause = (ConfiguredValueCreationException) e.getCause();
throw new AspectFunctionException(new AspectCreationException(
cause.getMessage(), cause.getAnalysisRootCause()));
+ } else if (e.getCause() instanceof InconsistentAspectOrderException) {
+ InconsistentAspectOrderException cause = (InconsistentAspectOrderException) e.getCause();
+ throw new AspectFunctionException(new AspectCreationException(
+ cause.getMessage()));
} else {
// Cast to InvalidConfigurationException as a consistency check. If you add any
// DependencyEvaluationException constructors, you may need to change this code, too.
@@ -519,5 +524,9 @@
public AspectFunctionException(AspectCreationException e) {
super(e, Transience.PERSISTENT);
}
+
+ public AspectFunctionException(InconsistentAspectOrderException cause) {
+ super(cause, Transience.PERSISTENT);
+ }
}
}
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 673a18f..c19e3c0 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
@@ -37,6 +37,7 @@
import com.google.devtools.build.lib.analysis.ConfiguredAspect;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.Dependency;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
import com.google.devtools.build.lib.analysis.MergedConfiguredTarget;
import com.google.devtools.build.lib.analysis.MergedConfiguredTarget.DuplicateException;
@@ -122,6 +123,10 @@
super(cause);
}
+ public DependencyEvaluationException(InconsistentAspectOrderException cause) {
+ super(cause);
+ }
+
@Override
public synchronized Exception getCause() {
return (Exception) super.getCause();
@@ -258,6 +263,10 @@
if (e.getCause() instanceof ConfiguredValueCreationException) {
throw new ConfiguredTargetFunctionException(
(ConfiguredValueCreationException) e.getCause());
+ } else if (e.getCause() instanceof InconsistentAspectOrderException) {
+ InconsistentAspectOrderException cause = (InconsistentAspectOrderException) e.getCause();
+ throw new ConfiguredTargetFunctionException(
+ new ConfiguredValueCreationException(cause.getMessage(), target.getLabel()));
} else {
// Cast to InvalidConfigurationException as a consistency check. If you add any
// DependencyEvaluationException constructors, you may need to change this code, too.
@@ -320,6 +329,9 @@
new ConfiguredValueCreationException(e.print(), ctgValue.getLabel()));
} catch (InvalidConfigurationException e) {
throw new DependencyEvaluationException(e);
+ } catch (InconsistentAspectOrderException e) {
+ env.getListener().handle(Event.error(e.getLocation(), e.getMessage()));
+ throw new DependencyEvaluationException(e);
}
// Trim each dep's configuration so it only includes the fragments needed by its transitive
@@ -986,8 +998,13 @@
// Collect the corresponding Skyframe configured target values. Abort early if they haven't
// been computed yet.
- Collection<Dependency> configValueNames = resolver.resolveRuleLabels(
- ctgValue, configLabelMap, transitiveLoadingRootCauses);
+ Collection<Dependency> configValueNames = null;
+ try {
+ configValueNames = resolver.resolveRuleLabels(
+ ctgValue, configLabelMap, transitiveLoadingRootCauses);
+ } catch (InconsistentAspectOrderException e) {
+ throw new DependencyEvaluationException(e);
+ }
if (env.valuesMissing()) {
return null;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
index bd5eef5..7789164 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PostConfiguredTargetFunction.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.Dependency;
+import com.google.devtools.build.lib.analysis.DependencyResolver.InconsistentAspectOrderException;
import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -111,11 +112,8 @@
deps = ConfiguredTargetFunction.getDynamicConfigurations(env, ctgValue, deps,
hostConfiguration, ruleClassProvider);
}
- } catch (EvalException e) {
- throw new PostConfiguredTargetFunctionException(e);
- } catch (ConfiguredTargetFunction.DependencyEvaluationException e) {
- throw new PostConfiguredTargetFunctionException(e);
- } catch (InvalidConfigurationException e) {
+ } catch (EvalException | ConfiguredTargetFunction.DependencyEvaluationException
+ | InvalidConfigurationException | InconsistentAspectOrderException e) {
throw new PostConfiguredTargetFunctionException(e);
}
