Skip to content

Commit

Permalink
Use the same method as LoadedPackageProvider.Bridge to look up targets.
Browse files Browse the repository at this point in the history
This unfortunately requires injecting an EventHandler into the
getTargetForLabel call, which is not super nice. On the other hand, it's not
clear how this can be much better - looking up targets in the cycle reporter
doesn't lend itself to prettiness.

--
MOS_MIGRATED_REVID=103965373
  • Loading branch information
ulfjack authored and hanwen committed Sep 28, 2015
1 parent 869605e commit 9d43bd0
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,27 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.concurrent.Uninterruptibles;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.NoSuchPackageException;
import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.NoSuchThingException;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.CyclesReporter;
import com.google.devtools.build.skyframe.SkyKey;

import java.util.concurrent.Callable;

/** Reports cycles between skyframe values whose keys contains {@link Label}s. */
abstract class AbstractLabelCycleReporter implements CyclesReporter.SingleCycleReporter {

private final LoadedPackageProvider loadedPackageProvider;
private final PackageProvider packageProvider;

AbstractLabelCycleReporter(LoadedPackageProvider loadedPackageProvider) {
this.loadedPackageProvider = loadedPackageProvider;
AbstractLabelCycleReporter(PackageProvider packageProvider) {
this.packageProvider = packageProvider;
}

/** Returns the String representation of the {@code SkyKey}. */
Expand All @@ -44,7 +49,8 @@ abstract class AbstractLabelCycleReporter implements CyclesReporter.SingleCycleR

protected abstract boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo);

protected String getAdditionalMessageAboutCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
protected String getAdditionalMessageAboutCycle(
EventHandler eventHandler, SkyKey topLevelKey, CycleInfo cycleInfo) {
return "";
}

Expand All @@ -58,7 +64,7 @@ public boolean maybeReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo,

if (alreadyReported) {
Label label = getLabel(topLevelKey);
Target target = getTargetForLabel(label);
Target target = getTargetForLabel(eventHandler, label);
eventHandler.handle(Event.error(target.getLocation(),
"in " + target.getTargetKind() + " " + label +
": cycle in dependency graph: target depends on an already-reported cycle"));
Expand All @@ -78,10 +84,10 @@ public String apply(SkyKey input) {
}
});

cycleMessage.append(getAdditionalMessageAboutCycle(topLevelKey, cycleInfo));
cycleMessage.append(getAdditionalMessageAboutCycle(eventHandler, topLevelKey, cycleInfo));

Label label = getLabel(cycleValue);
Target target = getTargetForLabel(label);
Target target = getTargetForLabel(eventHandler, label);
eventHandler.handle(Event.error(
target.getLocation(),
"in " + target.getTargetKind() + " " + label + ": " + cycleMessage));
Expand Down Expand Up @@ -117,14 +123,22 @@ static SkyKey printCycle(ImmutableList<SkyKey> cycle, StringBuilder cycleMessage
return cycleValue;
}

protected final Target getTargetForLabel(Label label) {
protected final Target getTargetForLabel(final EventHandler eventHandler, final Label label) {
try {
return loadedPackageProvider.getLoadedTarget(label);
return Uninterruptibles.callUninterruptibly(new Callable<Target>() {
@Override
public Target call()
throws NoSuchPackageException, NoSuchTargetException, InterruptedException {
return packageProvider.getTarget(eventHandler, label);
}
});
} catch (NoSuchThingException e) {
// This method is used for getting the target from a label in a circular dependency.
// If we have a cycle that means that we need to have accessed the target (to get its
// dependencies). So all the labels in a dependency cycle need to exist.
throw new IllegalStateException(e);
} catch (Exception e) {
throw new IllegalStateException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.analysis.LabelAndConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.lib.skyframe.ArtifactValue.OwnedArtifact;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.SkyFunctionName;
Expand All @@ -36,8 +36,8 @@ public class ActionArtifactCycleReporter extends AbstractLabelCycleReporter {
SkyFunctions.isSkyFunction(SkyFunctions.ACTION_EXECUTION),
SkyFunctions.isSkyFunction(SkyFunctions.TARGET_COMPLETION));

ActionArtifactCycleReporter(LoadedPackageProvider loadedPackageProvider) {
super(loadedPackageProvider);
ActionArtifactCycleReporter(PackageProvider packageProvider) {
super(packageProvider);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.SkyKey;

Expand All @@ -34,8 +35,8 @@ class ConfiguredTargetCycleReporter extends AbstractLabelCycleReporter {
private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET);

ConfiguredTargetCycleReporter(LoadedPackageProvider loadedPackageProvider) {
super(loadedPackageProvider);
ConfiguredTargetCycleReporter(PackageProvider packageProvider) {
super(packageProvider);
}

@Override
Expand All @@ -45,7 +46,8 @@ protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
}

@Override
protected String getAdditionalMessageAboutCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
protected String getAdditionalMessageAboutCycle(
EventHandler eventHandler, SkyKey topLevelKey, CycleInfo cycleInfo) {
return "\nThis cycle occurred because of a configuration option";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1605,12 +1605,11 @@ protected PathPackageLocator createPackageLocator(EventHandler eventHandler,
}

private CyclesReporter createCyclesReporter() {
LoadedPackageProvider loadedPackageProvider = getLoadedPackageProvider();
return new CyclesReporter(
new TransitiveTargetCycleReporter(loadedPackageProvider),
new ActionArtifactCycleReporter(loadedPackageProvider),
new TransitiveTargetCycleReporter(packageManager),
new ActionArtifactCycleReporter(packageManager),
new SkylarkModuleCycleReporter(),
new ConfiguredTargetCycleReporter(loadedPackageProvider));
new ConfiguredTargetCycleReporter(packageManager));
}

CyclesReporter getCyclesReporter() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.packages.PackageGroup;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.pkgcache.LoadedPackageProvider;
import com.google.devtools.build.lib.pkgcache.PackageProvider;
import com.google.devtools.build.skyframe.CycleInfo;
import com.google.devtools.build.skyframe.SkyKey;

Expand All @@ -35,8 +36,8 @@ class TransitiveTargetCycleReporter extends AbstractLabelCycleReporter {
private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
SkyFunctions.isSkyFunction(SkyFunctions.TRANSITIVE_TARGET);

TransitiveTargetCycleReporter(LoadedPackageProvider loadedPackageProvider) {
super(loadedPackageProvider);
TransitiveTargetCycleReporter(PackageProvider packageProvider) {
super(packageProvider);
}

@Override
Expand All @@ -57,8 +58,9 @@ protected Label getLabel(SkyKey key) {
}

@Override
protected String getAdditionalMessageAboutCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
Target currentTarget = getTargetForLabel(getLabel(topLevelKey));
protected String getAdditionalMessageAboutCycle(
EventHandler eventHandler, SkyKey topLevelKey, CycleInfo cycleInfo) {
Target currentTarget = getTargetForLabel(eventHandler, getLabel(topLevelKey));
List<SkyKey> keys = Lists.newArrayList();
if (!cycleInfo.getPathToCycle().isEmpty()) {
keys.add(topLevelKey);
Expand All @@ -70,7 +72,7 @@ protected String getAdditionalMessageAboutCycle(SkyKey topLevelKey, CycleInfo cy
keys.add(cycleInfo.getCycle().get(0));
for (SkyKey nextKey : keys) {
Label nextLabel = getLabel(nextKey);
Target nextTarget = getTargetForLabel(nextLabel);
Target nextTarget = getTargetForLabel(eventHandler, nextLabel);
// This is inefficient but it's no big deal since we only do this when there's a cycle.
if (currentTarget.getVisibility().getDependencyLabels().contains(nextLabel)
&& !nextTarget.getTargetKind().equals(PackageGroup.targetKind())) {
Expand Down

0 comments on commit 9d43bd0

Please sign in to comment.