Refactor DependencyResolver to collect and return loading errors.
This should never be triggered in production, where we always run a loading
phase first and only analyze targets that load successfully. I.e., this is
just plumbing which will be hooked up in a subsequent change.
--
MOS_MIGRATED_REVID=113258593
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 d409560..403528e 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
@@ -804,6 +804,11 @@
}
@Override
+ protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+ // The error must have been reported already during analysis.
+ }
+
+ @Override
protected Target getTarget(Label label) {
if (targetCache == null) {
try {
@@ -858,12 +863,19 @@
class SilentDependencyResolver extends DependencyResolver {
@Override
protected void invalidVisibilityReferenceHook(TargetAndConfiguration node, Label label) {
- // The error must have been reported already during analysis.
+ throw new RuntimeException("bad visibility on " + label + " during testing unexpected");
}
@Override
protected void invalidPackageGroupReferenceHook(TargetAndConfiguration node, Label label) {
- // The error must have been reported already during analysis.
+ throw new RuntimeException("bad package group on " + label + " during testing unexpected");
+ }
+
+ @Override
+ protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+ throw new RuntimeException(
+ "missing dependency from " + from.getLabel() + " to " + to + ": " + e.getMessage(),
+ e);
}
@Override
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 17d9d23..8d30b12 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
@@ -22,6 +22,7 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.ImmutableSortedKeyListMultimap;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.packages.Aspect;
import com.google.devtools.build.lib.packages.AspectClass;
import com.google.devtools.build.lib.packages.AspectDefinition;
@@ -89,21 +90,56 @@
Aspect aspect,
Set<ConfigMatchingProvider> configConditions)
throws EvalException, InterruptedException {
+ NestedSetBuilder<Label> rootCauses = NestedSetBuilder.<Label>stableOrder();
+ ListMultimap<Attribute, Dependency> outgoingEdges =
+ dependentNodeMap(node, hostConfig, aspect, configConditions, rootCauses);
+ if (!rootCauses.isEmpty()) {
+ throw new IllegalStateException(rootCauses.build().iterator().next().toString());
+ }
+ return outgoingEdges;
+ }
+
+ /**
+ * Returns ids for dependent nodes of a given node, sorted by attribute. Note that some
+ * dependencies do not have a corresponding attribute here, and we use the null attribute to
+ * represent those edges. Visibility attributes are only visited if {@code visitVisibility} is
+ * {@code true}.
+ *
+ * <p>If {@code aspect} is null, returns the dependent nodes of the configured
+ * target node representing the given target and configuration, otherwise that of the aspect
+ * node accompanying the aforementioned configured target node for the specified aspect.
+ *
+ * <p>The values are not simply labels because this also implements the first step of applying
+ * configuration transitions, namely, split transitions. This needs to be done before the labels
+ * are resolved because late bound attributes depend on the configuration. A good example for this
+ * is @{code :cc_toolchain}.
+ *
+ * <p>The long-term goal is that most configuration transitions be applied here. However, in order
+ * to do that, we first have to eliminate transitions that depend on the rule class of the
+ * dependency.
+ */
+ public final ListMultimap<Attribute, Dependency> dependentNodeMap(
+ TargetAndConfiguration node,
+ BuildConfiguration hostConfig,
+ Aspect aspect,
+ Set<ConfigMatchingProvider> configConditions,
+ NestedSetBuilder<Label> rootCauses)
+ throws EvalException, InterruptedException {
Target target = node.getTarget();
BuildConfiguration config = node.getConfiguration();
ListMultimap<Attribute, Dependency> outgoingEdges = ArrayListMultimap.create();
if (target instanceof OutputFile) {
Preconditions.checkNotNull(config);
- visitTargetVisibility(node, outgoingEdges.get(null));
+ visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
Rule rule = ((OutputFile) target).getGeneratingRule();
outgoingEdges.put(null, Dependency.withConfiguration(rule.getLabel(), config));
} else if (target instanceof InputFile) {
- visitTargetVisibility(node, outgoingEdges.get(null));
+ visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
} else if (target instanceof EnvironmentGroup) {
- visitTargetVisibility(node, outgoingEdges.get(null));
+ visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
} else if (target instanceof Rule) {
Preconditions.checkNotNull(config);
- visitTargetVisibility(node, outgoingEdges.get(null));
+ visitTargetVisibility(node, rootCauses, outgoingEdges.get(null));
Rule rule = (Rule) target;
ListMultimap<Attribute, LabelAndConfiguration> labelMap =
resolveAttributes(
@@ -112,15 +148,26 @@
config,
hostConfig,
configConditions);
- visitRule(rule, aspect, labelMap, outgoingEdges);
+ visitRule(rule, aspect, labelMap, rootCauses, outgoingEdges);
} else if (target instanceof PackageGroup) {
- visitPackageGroup(node, (PackageGroup) target, outgoingEdges.get(null));
+ visitPackageGroup(node, (PackageGroup) target, rootCauses, outgoingEdges.get(null));
} else {
throw new IllegalStateException(target.getLabel().toString());
}
return outgoingEdges;
}
+ @Nullable
+ private Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) {
+ try {
+ return getTarget(label);
+ } catch (NoSuchThingException e) {
+ rootCauses.add(label);
+ missingEdgeHook(from, label, e);
+ }
+ return null;
+ }
+
private ListMultimap<Attribute, LabelAndConfiguration> resolveAttributes(
Rule rule, AspectDefinition aspect, BuildConfiguration configuration,
BuildConfiguration hostConfiguration, Set<ConfigMatchingProvider> configConditions)
@@ -398,34 +445,30 @@
*/
public final Collection<Dependency> resolveRuleLabels(
TargetAndConfiguration node, ListMultimap<Attribute,
- LabelAndConfiguration> labelMap) {
+ LabelAndConfiguration> labelMap, NestedSetBuilder<Label> rootCauses) {
Preconditions.checkArgument(node.getTarget() instanceof Rule);
Rule rule = (Rule) node.getTarget();
ListMultimap<Attribute, Dependency> outgoingEdges = ArrayListMultimap.create();
- visitRule(rule, labelMap, outgoingEdges);
+ visitRule(rule, labelMap, rootCauses, outgoingEdges);
return outgoingEdges.values();
}
private void visitPackageGroup(TargetAndConfiguration node, PackageGroup packageGroup,
- Collection<Dependency> outgoingEdges) {
+ NestedSetBuilder<Label> rootCauses, Collection<Dependency> outgoingEdges) {
for (Label label : packageGroup.getIncludes()) {
- try {
- Target target = getTarget(label);
- if (target == null) {
- return;
- }
- if (!(target instanceof PackageGroup)) {
- // Note that this error could also be caught in PackageGroupConfiguredTarget, but since
- // these have the null configuration, visiting the corresponding target would trigger an
- // analysis of a rule with a null configuration, which doesn't work.
- invalidPackageGroupReferenceHook(node, label);
- continue;
- }
-
- outgoingEdges.add(Dependency.withNullConfiguration(label));
- } catch (NoSuchThingException e) {
- // Don't visit targets that don't exist (--keep_going)
+ Target target = getTarget(packageGroup, label, rootCauses);
+ if (target == null) {
+ continue;
}
+ if (!(target instanceof PackageGroup)) {
+ // Note that this error could also be caught in PackageGroupConfiguredTarget, but since
+ // these have the null configuration, visiting the corresponding target would trigger an
+ // analysis of a rule with a null configuration, which doesn't work.
+ invalidPackageGroupReferenceHook(node, label);
+ continue;
+ }
+
+ outgoingEdges.add(Dependency.withNullConfiguration(label));
}
}
@@ -467,14 +510,15 @@
}
private void visitRule(Rule rule, ListMultimap<Attribute, LabelAndConfiguration> labelMap,
- ListMultimap<Attribute, Dependency> outgoingEdges) {
- visitRule(rule, /*aspect=*/ null, labelMap, outgoingEdges);
+ NestedSetBuilder<Label> rootCauses, ListMultimap<Attribute, Dependency> outgoingEdges) {
+ visitRule(rule, /*aspect=*/ null, labelMap, rootCauses, outgoingEdges);
}
private void visitRule(
Rule rule,
Aspect aspect,
ListMultimap<Attribute, LabelAndConfiguration> labelMap,
+ NestedSetBuilder<Label> rootCauses,
ListMultimap<Attribute, Dependency> outgoingEdges) {
Preconditions.checkNotNull(labelMap);
for (Map.Entry<Attribute, Collection<LabelAndConfiguration>> entry :
@@ -484,13 +528,7 @@
Label label = dep.getLabel();
BuildConfiguration config = dep.getConfiguration();
- Target toTarget;
- try {
- toTarget = getTarget(label);
- } catch (NoSuchThingException e) {
- throw new IllegalStateException("not found: " + label + " from " + rule + " in "
- + attribute.getName());
- }
+ Target toTarget = getTarget(rule, label, rootCauses);
if (toTarget == null) {
continue;
}
@@ -526,28 +564,25 @@
}
private void visitTargetVisibility(TargetAndConfiguration node,
- Collection<Dependency> outgoingEdges) {
- for (Label label : node.getTarget().getVisibility().getDependencyLabels()) {
- try {
- Target visibilityTarget = getTarget(label);
- if (visibilityTarget == null) {
- return;
- }
- if (!(visibilityTarget instanceof PackageGroup)) {
- // Note that this error could also be caught in
- // AbstractConfiguredTarget.convertVisibility(), but we have an
- // opportunity here to avoid dependency cycles that result from
- // the visibility attribute of a rule referring to a rule that
- // depends on it (instead of its package)
- invalidVisibilityReferenceHook(node, label);
- continue;
- }
-
- // Visibility always has null configuration
- outgoingEdges.add(Dependency.withNullConfiguration(label));
- } catch (NoSuchThingException e) {
- // Don't visit targets that don't exist (--keep_going)
+ NestedSetBuilder<Label> rootCauses, Collection<Dependency> outgoingEdges) {
+ Target target = node.getTarget();
+ for (Label label : target.getVisibility().getDependencyLabels()) {
+ Target visibilityTarget = getTarget(target, label, rootCauses);
+ if (visibilityTarget == null) {
+ continue;
}
+ if (!(visibilityTarget instanceof PackageGroup)) {
+ // Note that this error could also be caught in
+ // AbstractConfiguredTarget.convertVisibility(), but we have an
+ // opportunity here to avoid dependency cycles that result from
+ // the visibility attribute of a rule referring to a rule that
+ // depends on it (instead of its package)
+ invalidVisibilityReferenceHook(node, label);
+ continue;
+ }
+
+ // Visibility always has null configuration
+ outgoingEdges.add(Dependency.withNullConfiguration(label));
}
}
@@ -569,6 +604,15 @@
Label label);
/**
+ * Hook for the error case where a dependency is missing.
+ *
+ * @param from the target referencing the missing target
+ * @param to the missing target
+ * @param e the exception that was thrown, e.g., by {@link #getTarget}
+ */
+ protected abstract void missingEdgeHook(Target from, Label to, NoSuchThingException e);
+
+ /**
* Returns the target by the given label.
*
* <p>Throws {@link NoSuchThingException} if the target is known not to exist.
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 2d0549b..4917d28 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
@@ -26,7 +26,9 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
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.Attribute;
@@ -105,6 +107,7 @@
throws AspectFunctionException, InterruptedException {
SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();
NestedSetBuilder<Package> transitivePackages = NestedSetBuilder.stableOrder();
+ NestedSetBuilder<Label> transitiveRootCauses = NestedSetBuilder.stableOrder();
AspectKey key = (AspectKey) skyKey.argument();
ConfiguredAspectFactory aspectFactory;
if (key.getAspectClass() instanceof NativeAspectClass<?>) {
@@ -152,9 +155,15 @@
"aspects must be attached to rules"));
}
- final ConfiguredTargetValue configuredTargetValue =
- (ConfiguredTargetValue)
- env.getValue(ConfiguredTargetValue.key(key.getLabel(), key.getConfiguration()));
+ final ConfiguredTargetValue configuredTargetValue;
+ try {
+ configuredTargetValue =
+ (ConfiguredTargetValue) env.getValueOrThrow(
+ ConfiguredTargetValue.key(key.getLabel(), key.getConfiguration()),
+ ConfiguredValueCreationException.class);
+ } catch (ConfiguredValueCreationException e) {
+ throw new AspectFunctionException(new AspectCreationException(e.getRootCauses()));
+ }
if (configuredTargetValue == null) {
// TODO(bazel-team): remove this check when top-level targets also use dynamic configurations.
// Right now the key configuration may be dynamic while the original target's configuration
@@ -175,7 +184,7 @@
try {
// Get the configuration targets that trigger this rule's configurable attributes.
Set<ConfigMatchingProvider> configConditions = ConfiguredTargetFunction.getConfigConditions(
- target, env, resolver, ctgValue, transitivePackages);
+ target, env, resolver, ctgValue, transitivePackages, transitiveRootCauses);
if (configConditions == null) {
// Those targets haven't yet been resolved.
return null;
@@ -190,7 +199,15 @@
configConditions,
ruleClassProvider,
view.getHostConfiguration(ctgValue.getConfiguration()),
- transitivePackages);
+ transitivePackages,
+ transitiveRootCauses);
+ if (depValueMap == null) {
+ return null;
+ }
+ if (!transitiveRootCauses.isEmpty()) {
+ throw new AspectFunctionException(
+ new AspectCreationException("Loading failed", transitiveRootCauses.build()));
+ }
return createAspect(
env,
@@ -282,6 +299,9 @@
* An exception indicating that there was a problem creating an aspect.
*/
public static final class AspectCreationException extends Exception {
+ /** Targets in the transitive closure that failed to load. May be empty. */
+ private final NestedSet<Label> loadingRootCauses;
+
/**
* The target for which analysis failed, if any. We can't represent aspects with labels, so if
* the aspect analysis fails, this will be {@code null}.
@@ -290,11 +310,26 @@
public AspectCreationException(String message, Label analysisRootCause) {
super(message);
+ this.loadingRootCauses = NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER);
this.analysisRootCause = analysisRootCause;
}
+ public AspectCreationException(String message, NestedSet<Label> loadingRootCauses) {
+ super(message);
+ this.loadingRootCauses = loadingRootCauses;
+ this.analysisRootCause = null;
+ }
+
+ public AspectCreationException(NestedSet<Label> loadingRootCauses) {
+ this("Loading failed", loadingRootCauses);
+ }
+
public AspectCreationException(String message) {
- this(message, null);
+ this(message, (Label) null);
+ }
+
+ public NestedSet<Label> getRootCauses() {
+ return loadingRootCauses;
}
@Nullable public Label getAnalysisRootCause() {
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 f4a7446..dba5155 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
@@ -42,7 +42,9 @@
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
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.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.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.StoredEventHandler;
@@ -68,7 +70,7 @@
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.build.skyframe.ValueOrException;
-import com.google.devtools.build.skyframe.ValueOrException3;
+import com.google.devtools.build.skyframe.ValueOrException2;
import java.util.Collection;
import java.util.HashMap;
@@ -124,6 +126,7 @@
InterruptedException {
SkyframeBuildView view = buildViewProvider.getSkyframeBuildView();
NestedSetBuilder<Package> transitivePackages = NestedSetBuilder.stableOrder();
+ NestedSetBuilder<Label> transitiveLoadingRootCauses = NestedSetBuilder.stableOrder();
ConfiguredTargetKey configuredTargetKey = (ConfiguredTargetKey) key.argument();
LabelAndConfiguration lc = LabelAndConfiguration.of(
configuredTargetKey.getLabel(), configuredTargetKey.getConfiguration());
@@ -162,8 +165,9 @@
new TargetAndConfiguration(target, configuration);
try {
// Get the configuration targets that trigger this rule's configurable attributes.
- Set<ConfigMatchingProvider> configConditions =
- getConfigConditions(ctgValue.getTarget(), env, resolver, ctgValue, transitivePackages);
+ Set<ConfigMatchingProvider> configConditions = getConfigConditions(
+ ctgValue.getTarget(), env, resolver, ctgValue, transitivePackages,
+ transitiveLoadingRootCauses);
if (env.valuesMissing()) {
return null;
}
@@ -177,7 +181,16 @@
configConditions,
ruleClassProvider,
view.getHostConfiguration(configuration),
- transitivePackages);
+ transitivePackages,
+ transitiveLoadingRootCauses);
+ if (env.valuesMissing()) {
+ return null;
+ }
+ if (!transitiveLoadingRootCauses.isEmpty()) {
+ throw new ConfiguredTargetFunctionException(
+ new ConfiguredValueCreationException(transitiveLoadingRootCauses.build()));
+ }
+ Preconditions.checkNotNull(depValueMap);
ConfiguredTargetValue ans = createConfiguredTarget(
view, env, target, configuration, depValueMap, configConditions, transitivePackages);
return ans;
@@ -194,12 +207,12 @@
}
} catch (AspectCreationException e) {
// getAnalysisRootCause may be null if the analysis of the aspect itself failed.
- Label rootCause = target.getLabel();
+ Label analysisRootCause = target.getLabel();
if (e.getAnalysisRootCause() != null) {
- rootCause = e.getAnalysisRootCause();
+ analysisRootCause = e.getAnalysisRootCause();
}
throw new ConfiguredTargetFunctionException(
- new ConfiguredValueCreationException(e.getMessage(), rootCause));
+ new ConfiguredValueCreationException(e.getMessage(), analysisRootCause));
}
}
@@ -229,13 +242,14 @@
Set<ConfigMatchingProvider> configConditions,
RuleClassProvider ruleClassProvider,
BuildConfiguration hostConfiguration,
- NestedSetBuilder<Package> transitivePackages)
+ NestedSetBuilder<Package> transitivePackages,
+ NestedSetBuilder<Label> transitiveLoadingRootCauses)
throws DependencyEvaluationException, AspectCreationException, InterruptedException {
// Create the map from attributes to list of (target, configuration) pairs.
ListMultimap<Attribute, Dependency> depValueNames;
try {
- depValueNames =
- resolver.dependentNodeMap(ctgValue, hostConfiguration, aspect, configConditions);
+ depValueNames = resolver.dependentNodeMap(
+ ctgValue, hostConfiguration, aspect, configConditions, transitiveLoadingRootCauses);
} catch (EvalException e) {
// EvalException can only be thrown by computed Skylark attributes in the current rule.
env.getListener().handle(Event.error(e.getLocation(), e.getMessage()));
@@ -256,14 +270,14 @@
// Resolve configured target dependencies and handle errors.
Map<SkyKey, ConfiguredTarget> depValues = resolveConfiguredTargetDependencies(env,
- depValueNames.values(), transitivePackages);
+ depValueNames.values(), transitivePackages, transitiveLoadingRootCauses);
if (depValues == null) {
return null;
}
// Resolve required aspects.
- ListMultimap<SkyKey, ConfiguredAspect> depAspects =
- resolveAspectDependencies(env, depValues, depValueNames.values(), transitivePackages);
+ ListMultimap<SkyKey, ConfiguredAspect> depAspects = resolveAspectDependencies(
+ env, depValues, depValueNames.values(), transitivePackages);
if (depAspects == null) {
return null;
}
@@ -566,10 +580,8 @@
}
}
- Map<SkyKey, ValueOrException3<
- AspectCreationException, NoSuchThingException, ConfiguredValueCreationException>>
- depAspects = env.getValuesOrThrow(aspectKeys, AspectCreationException.class,
- NoSuchThingException.class, ConfiguredValueCreationException.class);
+ Map<SkyKey, ValueOrException2<AspectCreationException, NoSuchThingException>> depAspects =
+ env.getValuesOrThrow(aspectKeys, AspectCreationException.class, NoSuchThingException.class);
for (Dependency dep : deps) {
SkyKey depKey = TO_KEYS.apply(dep);
@@ -587,10 +599,9 @@
SkyKey aspectKey = createAspectKey(dep.getLabel(), dep.getConfiguration(), depAspect);
AspectValue aspectValue = null;
try {
+ // TODO(ulfjack): Catch all thrown AspectCreationException and NoSuchThingException
+ // instances and merge them into a single Exception to get full root cause data.
aspectValue = (AspectValue) depAspects.get(aspectKey).get();
- } catch (ConfiguredValueCreationException e) {
- // The configured target should have been created in resolveConfiguredTargetDependencies()
- throw new IllegalStateException(e);
} catch (NoSuchThingException e) {
throw new AspectCreationException(
String.format(
@@ -639,7 +650,8 @@
@Nullable
static Set<ConfigMatchingProvider> getConfigConditions(Target target, Environment env,
SkyframeDependencyResolver resolver, TargetAndConfiguration ctgValue,
- NestedSetBuilder<Package> transitivePackages)
+ NestedSetBuilder<Package> transitivePackages,
+ NestedSetBuilder<Label> transitiveLoadingRootCauses)
throws DependencyEvaluationException {
if (!(target instanceof Rule)) {
return ImmutableSet.of();
@@ -665,7 +677,8 @@
// Collect the corresponding Skyframe configured target values. Abort early if they haven't
// been computed yet.
- Collection<Dependency> configValueNames = resolver.resolveRuleLabels(ctgValue, configLabelMap);
+ Collection<Dependency> configValueNames = resolver.resolveRuleLabels(
+ ctgValue, configLabelMap, transitiveLoadingRootCauses);
// No need to get new configs from Skyframe - config_setting rules always use the current
// target's config.
@@ -682,7 +695,7 @@
}
Map<SkyKey, ConfiguredTarget> configValues = resolveConfiguredTargetDependencies(
- env, configValueNames, transitivePackages);
+ env, configValueNames, transitivePackages, transitiveLoadingRootCauses);
if (configValues == null) {
return null;
}
@@ -714,30 +727,39 @@
*/
@Nullable
private static Map<SkyKey, ConfiguredTarget> resolveConfiguredTargetDependencies(
- Environment env, Collection<Dependency> deps, NestedSetBuilder<Package> transitivePackages)
- throws DependencyEvaluationException {
- boolean ok = !env.valuesMissing();
+ Environment env, Collection<Dependency> deps, NestedSetBuilder<Package> transitivePackages,
+ NestedSetBuilder<Label> transitiveLoadingRootCauses) throws DependencyEvaluationException {
+ boolean missedValues = env.valuesMissing();
+ boolean failed = false;
Iterable<SkyKey> depKeys = Iterables.transform(deps, TO_KEYS);
- Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValues =
+ Map<SkyKey, ValueOrException<ConfiguredValueCreationException>> depValuesOrExceptions =
env.getValuesOrThrow(depKeys, ConfiguredValueCreationException.class);
- Map<SkyKey, ConfiguredTarget> result = Maps.newHashMapWithExpectedSize(depValues.size());
- for (Map.Entry<SkyKey, ValueOrException<ConfiguredValueCreationException>> entry :
- depValues.entrySet()) {
- ConfiguredTargetValue depValue;
+ Map<SkyKey, ConfiguredTarget> result =
+ Maps.newHashMapWithExpectedSize(depValuesOrExceptions.size());
+ for (Map.Entry<SkyKey, ValueOrException<ConfiguredValueCreationException>> entry
+ : depValuesOrExceptions.entrySet()) {
try {
- depValue = (ConfiguredTargetValue) entry.getValue().get();
+ ConfiguredTargetValue depValue = (ConfiguredTargetValue) entry.getValue().get();
+ if (depValue == null) {
+ missedValues = true;
+ } else {
+ result.put(entry.getKey(), depValue.getConfiguredTarget());
+ transitivePackages.addTransitive(depValue.getTransitivePackages());
+ }
} catch (ConfiguredValueCreationException e) {
- throw new DependencyEvaluationException(e);
- }
- if (depValue == null) {
- ok = false;
- } else {
- result.put(entry.getKey(), depValue.getConfiguredTarget());
- transitivePackages.addTransitive(depValue.getTransitivePackages());
+ // TODO(ulfjack): If there is an analysis root cause, we drop all loading root causes.
+ if (e.getAnalysisRootCause() != null) {
+ throw new DependencyEvaluationException(e);
+ }
+ transitiveLoadingRootCauses.addTransitive(e.loadingRootCauses);
+ failed = true;
}
}
- if (!ok) {
+ if (missedValues) {
return null;
+ } else if (failed) {
+ throw new DependencyEvaluationException(
+ new ConfiguredValueCreationException(transitiveLoadingRootCauses.build()));
} else {
return result;
}
@@ -766,6 +788,7 @@
return null;
}
+ Preconditions.checkNotNull(depValueMap);
ConfiguredTarget configuredTarget = view.createConfiguredTarget(target, configuration,
analysisEnvironment, depValueMap, configConditions);
@@ -817,14 +840,34 @@
* a ConfiguredTargetValue.
*/
public static final class ConfiguredValueCreationException extends Exception {
+ private final NestedSet<Label> loadingRootCauses;
// TODO(ulfjack): Collect all analysis root causes, not just the first one.
- private final Label analysisRootCause;
+ @Nullable private final Label analysisRootCause;
public ConfiguredValueCreationException(String message, Label currentTarget) {
super(message);
+ this.loadingRootCauses = NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER);
this.analysisRootCause = Preconditions.checkNotNull(currentTarget);
}
+ public ConfiguredValueCreationException(String message, NestedSet<Label> rootCauses) {
+ super(message);
+ this.loadingRootCauses = rootCauses;
+ this.analysisRootCause = null;
+ }
+
+ public ConfiguredValueCreationException(NestedSet<Label> rootCauses) {
+ this("Loading failed", rootCauses);
+ }
+
+ public ConfiguredValueCreationException(String message) {
+ this(message, NestedSetBuilder.<Label>emptySet(Order.STABLE_ORDER));
+ }
+
+ public NestedSet<Label> getRootCauses() {
+ return loadingRootCauses;
+ }
+
public Label getAnalysisRootCause() {
return analysisRootCause;
}
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 284491d..878caec 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
@@ -101,6 +101,9 @@
buildViewProvider.getSkyframeBuildView().getHostConfiguration(ct.getConfiguration());
SkyframeDependencyResolver resolver =
buildViewProvider.getSkyframeBuildView().createDependencyResolver(env);
+ // We don't track root causes here - this function is only invoked for successfully analyzed
+ // targets - as long as we redo the exact same steps here as in ConfiguredTargetFunction, this
+ // can never fail.
deps =
resolver.dependentNodeMap(
ctgValue, hostConfiguration, /*aspect=*/ null, configConditions);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 60182d8..2802fad 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -499,6 +499,9 @@
Set<ConfigMatchingProvider> configConditions) throws InterruptedException {
Preconditions.checkState(enableAnalysis,
"Already in execution phase %s %s", target, configuration);
+ Preconditions.checkNotNull(analysisEnvironment);
+ Preconditions.checkNotNull(target);
+ Preconditions.checkNotNull(prerequisiteMap);
return factory.createConfiguredTarget(analysisEnvironment, artifactFactory, target,
configuration, getHostConfiguration(configuration), prerequisiteMap,
configConditions);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
index acff770..c609e99 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeDependencyResolver.java
@@ -17,7 +17,11 @@
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.Event;
+import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException;
+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.Package;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.TargetUtils;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -50,17 +54,45 @@
"label '%s' does not refer to a package group", label)));
}
+ @Override
+ protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+ if (e instanceof NoSuchTargetException) {
+ NoSuchTargetException nste = (NoSuchTargetException) e;
+ if (to.equals(nste.getLabel())) {
+ env.getListener().handle(
+ Event.error(
+ TargetUtils.getLocationMaybe(from),
+ TargetUtils.formatMissingEdge(from, to, e)));
+ }
+ } else if (e instanceof NoSuchPackageException) {
+ NoSuchPackageException nspe = (NoSuchPackageException) e;
+ if (nspe.getPackageId().equals(to.getPackageIdentifier())) {
+ env.getListener().handle(
+ Event.error(
+ TargetUtils.getLocationMaybe(from),
+ TargetUtils.formatMissingEdge(from, to, e)));
+ }
+ }
+ }
+
@Nullable
@Override
protected Target getTarget(Label label) throws NoSuchThingException {
+ // TODO(ulfjack): This swallows all loading errors without reporting. That's ok for now, as we
+ // generally run a loading phase first, and only analyze targets that load successfully.
if (env.getValue(TargetMarkerValue.key(label)) == null) {
return null;
}
SkyKey key = PackageValue.key(label.getPackageIdentifier());
- PackageValue packageValue = (PackageValue) env.getValue(key);
- if (packageValue == null || packageValue.getPackage().containsErrors()) {
+ PackageValue packageValue =
+ (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class);
+ if (packageValue == null) {
return null;
}
+ Package pkg = packageValue.getPackage();
+ if (pkg.containsErrors()) {
+ throw new BuildFileContainsErrorsException(label.getPackageIdentifier());
+ }
return packageValue.getPackage().getTarget(label.getName());
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
index 19ad653..e75ca93 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/DependencyResolverTest.java
@@ -64,6 +64,11 @@
throw new IllegalStateException();
}
+ @Override
+ protected void missingEdgeHook(Target from, Label to, NoSuchThingException e) {
+ throw new IllegalStateException(e);
+ }
+
@Nullable
@Override
protected Target getTarget(Label label) throws NoSuchThingException {
