Skip to content

Commit

Permalink
Remove the originating rule and attribute from the "inconsistent aspe…
Browse files Browse the repository at this point in the history
…ct order" error message.

This is unfortunate, but necessary for separating the configuration and aspect computation of a dependency into a Skyframe function that's separate from the evaluation of its parent.

It's conceivable that this could be restored by emitting the message when the unfiltered aspect path is computed, but let's do that in a followup change if necessary. I'm not convinced that we need this at all...

RELNOTES: None.
PiperOrigin-RevId: 232456350
  • Loading branch information
lberki authored and Copybara-Service committed Feb 5, 2019
1 parent 0cd3594 commit 439665c
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -384,16 +384,15 @@ public final OrderedSetMultimap<Attribute, Dependency> dependentNodeMap(
continue;
}

Attribute attribute = entry.getKey().getAttribute();

ConfigurationTransition transition =
TransitionResolver.evaluateTransition(
node.getConfiguration(), dep.getTransition(), toTarget, trimmingTransitionFactory);

AspectCollection requiredAspects =
requiredAspects(fromRule, dep.getPropagatingAspects(), attribute.getName(), toTarget);
filterPropagatingAspects(dep.getPropagatingAspects(), toTarget);

outgoingEdges.put(
attribute,
entry.getKey().getAttribute(),
transition == NullTransition.INSTANCE
? Dependency.withNullConfiguration(dep.getLabel())
: Dependency.withTransitionAndAspects(dep.getLabel(), transition, requiredAspects));
Expand Down Expand Up @@ -630,8 +629,11 @@ private List<AttributeDependencyKind> getAttributes(Rule rule, Iterable<Aspect>
return result.build();
}

private AspectCollection requiredAspects(
Rule fromRule, Iterable<Aspect> aspects, String attributeName, Target toTarget)
/**
* Filter the set of aspects that are to be propagated according to the set of advertised
* providers of the dependency.
*/
private AspectCollection filterPropagatingAspects(Iterable<Aspect> aspects, Target toTarget)
throws InconsistentAspectOrderException {
if (!(toTarget instanceof Rule)) {
return AspectCollection.EMPTY;
Expand All @@ -653,7 +655,7 @@ private AspectCollection requiredAspects(
try {
return AspectCollection.create(filteredAspectPath.build(), visibleAspects.build());
} catch (AspectCycleOnPathException e) {
throw new InconsistentAspectOrderException(fromRule, attributeName, toTarget, e);
throw new InconsistentAspectOrderException(toTarget, e);
}
}

Expand Down Expand Up @@ -694,13 +696,9 @@ protected abstract Map<Label, Target> getTargets(
public class InconsistentAspectOrderException extends Exception {
private final Location location;

public InconsistentAspectOrderException(
Rule originalRule, String attributeName, Target target, AspectCycleOnPathException e) {
super(
String.format(
"%s (when propagating from %s to %s via attribute %s)",
e.getMessage(), originalRule.getLabel(), target.getLabel(), attributeName));
this.location = originalRule.getLocation();
public InconsistentAspectOrderException(Target target, AspectCycleOnPathException e) {
super(String.format("%s (when propagating to %s)", e.getMessage(), target.getLabel()));
this.location = target.getLocation();
}

public Location getLocation() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2139,9 +2139,9 @@ public void aspectOnAspectInconsistentVisibility() throws Exception {
// expected
}
assertContainsEvent(
"ERROR /workspace/test/BUILD:4:1: Aspect //test:aspect.bzl%a2 is"
"ERROR /workspace/test/BUILD:3:1: Aspect //test:aspect.bzl%a2 is"
+ " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+ "(when propagating from //test:r2 to //test:r1 via attribute dep)");
+ "(when propagating to //test:r1)");
}

@Test
Expand Down Expand Up @@ -2184,9 +2184,9 @@ public void aspectOnAspectInconsistentVisibilityIndirect() throws Exception {
// expected
}
assertContainsEvent(
"ERROR /workspace/test/BUILD:4:1: Aspect //test:aspect.bzl%a2 is"
"ERROR /workspace/test/BUILD:3:1: Aspect //test:aspect.bzl%a2 is"
+ " applied twice, both before and after aspect //test:aspect.bzl%a1 "
+ "(when propagating from //test:r2 to //test:r1 via attribute dep)");
+ "(when propagating to //test:r1)");
}

/**
Expand Down

0 comments on commit 439665c

Please sign in to comment.