diff options
author | Ulf Adams <ulfjack@google.com> | 2016-02-01 13:04:54 +0000 |
---|---|---|
committer | Damien Martin-Guillerez <dmarting@google.com> | 2016-02-02 14:55:00 +0000 |
commit | 2ac20962867aec785fb6f4616e6b51cbf5a3fb01 (patch) | |
tree | 0f72608db05852cf808cbae69218795ed28ed3b8 /src/main/java/com/google | |
parent | 77f9a05154fe4b2e7f57c201fc980d098db7f776 (diff) |
Implement proper error handling for interleaved loading and analysis.
Add test coverage by re-running BuildViewTest with the new Skyframe loading
phase runner.
--
MOS_MIGRATED_REVID=113517509
Diffstat (limited to 'src/main/java/com/google')
6 files changed, 116 insertions, 64 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 403528e020..35d3c123b1 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 @@ -809,7 +809,7 @@ public class BuildView { } @Override - protected Target getTarget(Label label) { + protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) { if (targetCache == null) { try { return LoadedPackageProvider.Bridge.getLoadedTarget( @@ -879,9 +879,13 @@ public class BuildView { } @Override - protected Target getTarget(Label label) throws NoSuchThingException { - return LoadedPackageProvider.Bridge.getLoadedTarget( - skyframeExecutor.getPackageManager(), eventHandler, label); + protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) { + try { + return LoadedPackageProvider.Bridge.getLoadedTarget( + skyframeExecutor.getPackageManager(), eventHandler, label); + } catch (NoSuchThingException e) { + throw new IllegalStateException(e); + } } } 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 8d30b12f47..2d7edd2419 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 @@ -157,17 +157,6 @@ public abstract class DependencyResolver { 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) @@ -615,13 +604,11 @@ public abstract class DependencyResolver { /** * Returns the target by the given label. * - * <p>Throws {@link NoSuchThingException} if the target is known not to exist. - * * <p>Returns null if the target is not ready to be returned at this moment. If getTarget returns * null once or more during a {@link #dependentNodeMap} call, the results of that call will be * incomplete. For use within Skyframe, where several iterations may be needed to discover * all dependencies. */ @Nullable - protected abstract Target getTarget(Label label) throws NoSuchThingException; + protected abstract Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses); } 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 dba5155f25..1224c3ec42 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 @@ -73,6 +73,7 @@ import com.google.devtools.build.skyframe.ValueOrException; import com.google.devtools.build.skyframe.ValueOrException2; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Map; @@ -85,6 +86,10 @@ import javax.annotation.Nullable; * SkyFunction for {@link ConfiguredTargetValue}s. */ final class ConfiguredTargetFunction implements SkyFunction { + // This construction is a bit funky, but guarantees that the Object reference here is globally + // unique. + static final Set<ConfigMatchingProvider> NO_CONFIG_CONDITIONS = + Collections.unmodifiableSet(ImmutableSet.<ConfigMatchingProvider>of()); /** * Exception class that signals an error during the evaluation of a dependency. @@ -138,19 +143,27 @@ final class ConfiguredTargetFunction implements SkyFunction { if (packageValue == null) { return null; } + + // TODO(ulfjack): This tries to match the logic in TransitiveTargetFunction / + // TargetMarkerFunction. Maybe we can merge the two? Package pkg = packageValue.getPackage(); - if (pkg.containsErrors()) { - throw new ConfiguredTargetFunctionException( - new BuildFileContainsErrorsException(lc.getLabel().getPackageIdentifier())); - } Target target; try { - target = packageValue.getPackage().getTarget(lc.getLabel().getName()); + target = pkg.getTarget(lc.getLabel().getName()); } catch (NoSuchTargetException e) { - throw new ConfiguredTargetFunctionException(e); + throw new ConfiguredTargetFunctionException( + new ConfiguredValueCreationException(e.getMessage())); } - - transitivePackages.add(packageValue.getPackage()); + if (pkg.containsErrors()) { + if (target == null) { + throw new ConfiguredTargetFunctionException(new ConfiguredValueCreationException( + new BuildFileContainsErrorsException( + lc.getLabel().getPackageIdentifier()).getMessage())); + } else { + transitiveLoadingRootCauses.add(lc.getLabel()); + } + } + transitivePackages.add(pkg); // TODO(bazel-team): This is problematic - we create the right key, but then end up with a value // that doesn't match; we can even have the same value multiple times. However, I think it's // only triggered in tests (i.e., in normal operation, the configuration passed in is already @@ -158,11 +171,9 @@ final class ConfiguredTargetFunction implements SkyFunction { if (!target.isConfigurable()) { configuration = null; } + TargetAndConfiguration ctgValue = new TargetAndConfiguration(target, configuration); SkyframeDependencyResolver resolver = view.createDependencyResolver(env); - - TargetAndConfiguration ctgValue = - new TargetAndConfiguration(target, configuration); try { // Get the configuration targets that trigger this rule's configurable attributes. Set<ConfigMatchingProvider> configConditions = getConfigConditions( @@ -171,6 +182,17 @@ final class ConfiguredTargetFunction implements SkyFunction { if (env.valuesMissing()) { return null; } + // TODO(ulfjack): ConfiguredAttributeMapper (indirectly used from computeDependencies) isn't + // safe to use if there are missing config conditions, so we stop here, but only if there are + // config conditions - though note that we can't check if configConditions is non-empty - it + // may be empty for other reasons. It would be better to continue here so that we can collect + // more root causes during computeDependencies. + // Note that this doesn't apply to AspectFunction, because aspects can't have configurable + // attributes. + if (!transitiveLoadingRootCauses.isEmpty() && configConditions != NO_CONFIG_CONDITIONS) { + throw new ConfiguredTargetFunctionException( + new ConfiguredValueCreationException(transitiveLoadingRootCauses.build())); + } ListMultimap<Attribute, ConfiguredTarget> depValueMap = computeDependencies( @@ -654,7 +676,7 @@ final class ConfiguredTargetFunction implements SkyFunction { NestedSetBuilder<Label> transitiveLoadingRootCauses) throws DependencyEvaluationException { if (!(target instanceof Rule)) { - return ImmutableSet.of(); + return NO_CONFIG_CONDITIONS; } ImmutableSet.Builder<ConfigMatchingProvider> configConditions = ImmutableSet.builder(); @@ -672,7 +694,7 @@ final class ConfiguredTargetFunction implements SkyFunction { } } if (configLabelMap.isEmpty()) { - return ImmutableSet.of(); + return NO_CONFIG_CONDITIONS; } // Collect the corresponding Skyframe configured target values. Abort early if they haven't 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 c2d17150cd..3f0aa18735 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 @@ -51,9 +51,12 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.Attribute; +import com.google.devtools.build.lib.packages.NoSuchPackageException; +import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.Package; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.Target; +import com.google.devtools.build.lib.pkgcache.LoadingFailureEvent; import com.google.devtools.build.lib.pkgcache.LoadingPhaseRunner; import com.google.devtools.build.lib.skyframe.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.skyframe.AspectFunction.AspectCreationException; @@ -336,39 +339,39 @@ public final class SkyframeBuildView { // --keep_going : We notify the error and return a ConfiguredTargetValue for (Map.Entry<SkyKey, ErrorInfo> errorEntry : result.errorMap().entrySet()) { // Only handle errors of configured targets, not errors of top-level aspects. + // TODO(ulfjack): this is quadratic - if there are a lot of CTs, this could be rather slow. if (!values.contains(errorEntry.getKey().argument())) { continue; } SkyKey errorKey = errorEntry.getKey(); ConfiguredTargetKey label = (ConfiguredTargetKey) errorKey.argument(); + Label topLevelLabel = label.getLabel(); ErrorInfo errorInfo = errorEntry.getValue(); assertSaneAnalysisError(errorInfo, errorKey); skyframeExecutor.getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), errorKey, eventHandler); Exception cause = errorInfo.getException(); - // We try to get the root cause key first from ErrorInfo rootCauses. If we don't have one - // we try to use the cycle culprit if the error is a cycle. Otherwise we use the top-level - // error key. - Label analysisRootCause; + Label analysisRootCause = null; if (cause instanceof ConfiguredValueCreationException) { - analysisRootCause = ((ConfiguredValueCreationException) cause).getAnalysisRootCause(); - } else if (!Iterables.isEmpty(errorEntry.getValue().getRootCauses())) { - SkyKey culprit = Preconditions.checkNotNull(Iterables.getFirst( - errorEntry.getValue().getRootCauses(), null)); - analysisRootCause = ((ConfiguredTargetKey) culprit.argument()).getLabel(); - } else { - analysisRootCause = maybeGetConfiguredTargetCycleCulprit(errorInfo.getCycleInfo()); - } - if (cause instanceof ActionConflictException) { + ConfiguredValueCreationException ctCause = (ConfiguredValueCreationException) cause; + for (Label rootCause : ctCause.getRootCauses()) { + eventBus.post(new LoadingFailureEvent(topLevelLabel, rootCause)); + } + analysisRootCause = ctCause.getAnalysisRootCause(); + } else if (!Iterables.isEmpty(errorInfo.getCycleInfo())) { + analysisRootCause = maybeGetConfiguredTargetCycleCulprit( + topLevelLabel, errorInfo.getCycleInfo()); + } else if (cause instanceof ActionConflictException) { ((ActionConflictException) cause).reportTo(eventHandler); } eventHandler.handle( Event.warn("errors encountered while analyzing target '" - + label.getLabel() + "': it will not be built")); - eventBus.post(new AnalysisFailureEvent( - LabelAndConfiguration.of(label.getLabel(), label.getConfiguration()), - analysisRootCause)); + + topLevelLabel + "': it will not be built")); + if (analysisRootCause != null) { + eventBus.post(new AnalysisFailureEvent( + LabelAndConfiguration.of(topLevelLabel, label.getConfiguration()), analysisRootCause)); + } } Collection<Exception> reportedExceptions = Sets.newHashSet(); @@ -413,14 +416,27 @@ public final class SkyframeBuildView { } @Nullable - private Label maybeGetConfiguredTargetCycleCulprit(Iterable<CycleInfo> cycleInfos) { + private static Label maybeGetConfiguredTargetCycleCulprit( + Label labelToLoad, Iterable<CycleInfo> cycleInfos) { for (CycleInfo cycleInfo : cycleInfos) { SkyKey culprit = Iterables.getFirst(cycleInfo.getCycle(), null); if (culprit == null) { continue; } if (culprit.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) { - return ((LabelAndConfiguration) culprit.argument()).getLabel(); + return ((ConfiguredTargetKey) culprit.argument()).getLabel(); + } else { + // For other types of cycles (e.g. file symlink cycles), the root cause is the furthest + // target dependency that itself depended on the cycle. + Label furthestTarget = labelToLoad; + for (SkyKey skyKey : cycleInfo.getPathToCycle()) { + if (skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) { + furthestTarget = (Label) skyKey.argument(); + } else { + break; + } + } + return furthestTarget; } } return null; @@ -436,7 +452,10 @@ public final class SkyframeBuildView { || cause instanceof ActionConflictException // For top-level aspects || cause instanceof AspectCreationException - || cause instanceof SkylarkImportFailedException, + || cause instanceof SkylarkImportFailedException + // Only if we run the reduced loading phase and then analyze with --nokeep_going. + || cause instanceof NoSuchTargetException + || cause instanceof NoSuchPackageException, "%s -> %s", key, errorInfo); 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 c609e99148..0655d47910 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 @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.skyframe; 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.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.packages.BuildFileContainsErrorsException; import com.google.devtools.build.lib.packages.NoSuchPackageException; @@ -77,22 +78,38 @@ public final class SkyframeDependencyResolver extends DependencyResolver { @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) { + protected Target getTarget(Target from, Label label, NestedSetBuilder<Label> rootCauses) { + SkyKey key = PackageValue.key(label.getPackageIdentifier()); + PackageValue packageValue; + try { + packageValue = (PackageValue) env.getValueOrThrow(key, NoSuchPackageException.class); + } catch (NoSuchPackageException e) { + rootCauses.add(label); + missingEdgeHook(from, label, e); return null; } - SkyKey key = PackageValue.key(label.getPackageIdentifier()); - 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()); + try { + Target target = pkg.getTarget(label.getName()); + if (pkg.containsErrors()) { + NoSuchPackageException e = + new BuildFileContainsErrorsException(label.getPackageIdentifier()); + missingEdgeHook(from, label, e); + if (target != null) { + rootCauses.add(label); + return target; + } else { + return null; + } + } + return target; + } catch (NoSuchTargetException e) { + rootCauses.add(label); + missingEdgeHook(from, label, e); + return null; } - return packageValue.getPackage().getTarget(label.getName()); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java index d7eda03665..bfc7fb3453 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java @@ -316,7 +316,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { Root buildDataDirectory, PackageFactory pkgFactory, Predicate<PathFragment> allowedMissingInputs) { - RuleClassProvider ruleClassProvider = pkgFactory.getRuleClassProvider(); + ConfiguredRuleClassProvider ruleClassProvider = + (ConfiguredRuleClassProvider) pkgFactory.getRuleClassProvider(); // We use an immutable map builder for the nice side effect that it throws if a duplicate key // is inserted. ImmutableMap.Builder<SkyFunctionName, SkyFunction> map = ImmutableMap.builder(); @@ -1632,8 +1633,10 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { return new CyclesReporter( new TransitiveTargetCycleReporter(packageManager), new ActionArtifactCycleReporter(packageManager), - new SkylarkModuleCycleReporter(), - new ConfiguredTargetCycleReporter(packageManager)); + // TODO(ulfjack): The SkylarkModuleCycleReporter swallows previously reported cycles + // unconditionally! Is that intentional? + new ConfiguredTargetCycleReporter(packageManager), + new SkylarkModuleCycleReporter()); } CyclesReporter getCyclesReporter() { |