diff options
author | Ulf Adams <ulfjack@google.com> | 2016-01-28 15:05:16 +0000 |
---|---|---|
committer | Kristina Chodorow <kchodorow@google.com> | 2016-01-28 15:30:46 +0000 |
commit | 849017381db007108b3a9e25b1dbf07b7587d31c (patch) | |
tree | bb1b03a339fedb20d87b9a80e3af8d74b8a8b4f7 /src/main/java/com/google/devtools/build/lib | |
parent | b6fbab7115c1567705e7bddc7b79bdee6313fce3 (diff) |
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
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib')
7 files changed, 278 insertions, 106 deletions
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 d409560a67..403528e020 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 @@ public class BuildView { } @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 @@ public class BuildView { 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 17d9d23b19..8d30b12f47 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.BuildConfiguration; 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 @@ public abstract class DependencyResolver { 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 @@ public abstract class DependencyResolver { 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 abstract class DependencyResolver { */ 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 @@ public abstract class DependencyResolver { } 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 @@ public abstract class DependencyResolver { 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 @@ public abstract class DependencyResolver { } 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 @@ public abstract class DependencyResolver { 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 2d0549b163..4917d28397 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.BuildConfiguration; 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 @@ public final class AspectFunction implements SkyFunction { 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 @@ public final class AspectFunction implements SkyFunction { "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 @@ public final class AspectFunction implements SkyFunction { 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 @@ public final class AspectFunction implements SkyFunction { 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 @@ public final class AspectFunction implements SkyFunction { * 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 final class AspectFunction implements SkyFunction { 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 f4a74460bd..dba5155f25 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.HostTransition; 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.SkyFunctionException; 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { } } 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { // 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 @@ final class ConfiguredTargetFunction implements SkyFunction { } } - 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 @@ final class ConfiguredTargetFunction implements SkyFunction { 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 @@ final class ConfiguredTargetFunction implements SkyFunction { @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 @@ final class ConfiguredTargetFunction implements SkyFunction { // 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 @@ final class ConfiguredTargetFunction implements SkyFunction { } Map<SkyKey, ConfiguredTarget> configValues = resolveConfiguredTargetDependencies( - env, configValueNames, transitivePackages); + env, configValueNames, transitivePackages, transitiveLoadingRootCauses); if (configValues == null) { return null; } @@ -714,30 +727,39 @@ final class ConfiguredTargetFunction implements SkyFunction { */ @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 @@ final class ConfiguredTargetFunction implements SkyFunction { return null; } + Preconditions.checkNotNull(depValueMap); ConfiguredTarget configuredTarget = view.createConfiguredTarget(target, configuration, analysisEnvironment, depValueMap, configConditions); @@ -817,14 +840,34 @@ final class ConfiguredTargetFunction implements SkyFunction { * 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 284491d996..878caeca08 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 @@ public class PostConfiguredTargetFunction implements SkyFunction { 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 60182d8b5d..2802fadc40 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 @@ public final class SkyframeBuildView { 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 acff770241..c609e99148 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.DependencyResolver; 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 @@ public final class SkyframeDependencyResolver extends DependencyResolver { "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()); } } |