diff options
author | ulfjack <ulfjack@google.com> | 2018-06-07 00:42:22 -0700 |
---|---|---|
committer | Copybara-Service <copybara-piper@google.com> | 2018-06-07 00:43:39 -0700 |
commit | ef77f872d4bc1325c2405d6f260c3e98f22d895e (patch) | |
tree | 30f678efc98d81ebf3cf6e218172859240ed5c3b /src/main/java/com/google/devtools/build/lib/skyframe | |
parent | 602cc856051b02328ed56e2af808b829dafecd4b (diff) |
Rewrite TargetPattern failure reporting
Report failures in TargetPatternFunction, rather than in its callers. Since we
can't distinguish between keep_going and nokeep_going modes, we otherwise end
up double-reporting errors. In the particular case that's covered by the
build_event_stream_test.sh, we end up reporting the same target pattern as both
skipped and failed.
Unfortunately, this means we cannot report whether the target pattern was
skipped or failed, so the pattern_skipped event is now unused (if we agree that
this is acceptable, I'll remove the corresponding infrastructure).
PiperOrigin-RevId: 199593700
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib/skyframe')
5 files changed, 43 insertions, 11 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java index 00aa64e5e3..5587284333 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/LegacyLoadingPhaseRunner.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.skyframe; import com.google.common.base.Preconditions; import com.google.common.base.Stopwatch; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Sets; import com.google.devtools.build.lib.cmdline.Label; @@ -220,7 +221,8 @@ public final class LegacyLoadingPhaseRunner extends LoadingPhaseRunner { filteredTargets, testFilteredTargets, targetPatterns, - expandedTargetsToLoad)); + expandedTargetsToLoad, + ImmutableList.of())); eventHandler.post(new TargetParsingPhaseTimeEvent(targetPatternEvalTime)); if (callback != null) { callback.notifyTargets(expandedTargetsToLoad); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java b/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java index 99a76c840c..b6145a56b7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PatternExpandingError.java @@ -47,6 +47,12 @@ public final class PatternExpandingError implements BuildEvent { return new PatternExpandingError(pattern, message, false); } + public static PatternExpandingError failed(String term, String message) { + return new PatternExpandingError(ImmutableList.of(term), message, false); + } + + // This is unused right now - when we generate the error, we don't know if we're in keep_going + // mode or not. public static PatternExpandingError skipped(String term, String message) { return new PatternExpandingError(ImmutableList.of(term), message, true); } 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 2b4da1b4a0..d52b9ed16e 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 @@ -2258,6 +2258,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { if (!Iterables.isEmpty(errorInfo.getCycleInfo())) { exc = new TargetParsingException("cycles detected during target parsing"); getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler); + // Fallback: we don't know which patterns failed, specifically, so we report the entire + // set as being in error. + eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage())); } else { // TargetPatternPhaseFunction never directly throws. Thus, the only way // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled @@ -2270,8 +2273,12 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { (e instanceof TargetParsingException) ? (TargetParsingException) e : new TargetParsingException(e.getMessage(), e); + if (!(e instanceof TargetParsingException)) { + // If it's a TargetParsingException, then the TargetPatternPhaseFunction has already + // reported the error, so we don't need to report it again. + eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage())); + } } - eventHandler.post(PatternExpandingError.failed(targetPatterns, exc.getMessage())); throw exc; } long timeMillis = timer.stop().elapsed(TimeUnit.MILLISECONDS); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java index 322061fe69..754917d201 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternFunction.java @@ -74,6 +74,8 @@ public class TargetPatternFunction implements SkyFunction { /*blacklistedSubdirectories=*/ ImmutableSet.of(), excludedSubdirectories, callback, + // The exception type here has to match the one on the BatchCallback. Since the callback + // defined above never throws, the exact type here is not really relevant. RuntimeException.class); resolvedTargets = ResolvedTargets.<Target>builder().addAll(results).build(); } catch (TargetParsingException e) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java index c087cc369c..2598dd8e9c 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java @@ -77,7 +77,8 @@ final class TargetPatternPhaseFunction implements SkyFunction { } // Determine targets to build: - ResolvedTargets<Target> targets = getTargetsToBuild(env, options); + List<String> failedPatterns = new ArrayList<String>(); + ResolvedTargets<Target> targets = getTargetsToBuild(env, options, failedPatterns); // If the --build_tests_only option was specified or we want to run tests, we need to determine // the list of targets to test. For that, we remove manual tests and apply the command-line @@ -164,7 +165,6 @@ final class TargetPatternPhaseFunction implements SkyFunction { LoadingPhaseRunner.maybeReportDeprecation(env.getListener(), targets.getTargets()); - boolean preExpansionError = targets.hasError(); ResolvedTargets.Builder<Label> expandedLabelsBuilder = ResolvedTargets.builder(); for (Target target : targets.getTargets()) { if (TargetUtils.isTestSuiteRule(target) && options.isExpandTestSuites()) { @@ -207,7 +207,8 @@ final class TargetPatternPhaseFunction implements SkyFunction { filteredTargets, testFilteredTargets, options.getTargetPatterns(), - expandedTargets.getTargets())); + expandedTargets.getTargets(), + failedPatterns)); return result; } @@ -217,7 +218,7 @@ final class TargetPatternPhaseFunction implements SkyFunction { * @param options the command-line arguments in structured form */ private static ResolvedTargets<Target> getTargetsToBuild( - Environment env, TargetPatternPhaseKey options) + Environment env, TargetPatternPhaseKey options, List<String> failedPatterns) throws InterruptedException { List<TargetPatternKey> patternSkyKeys = new ArrayList<>(); ResolvedTargets.Builder<Target> builder = ResolvedTargets.builder(); @@ -231,6 +232,11 @@ final class TargetPatternPhaseFunction implements SkyFunction { try { patternSkyKeys.add(keyOrException.getSkyKey()); } catch (TargetParsingException e) { + failedPatterns.add(keyOrException.getOriginalPattern()); + // We post a PatternExpandingError here - the pattern could not be parsed, so we don't even + // get to run TargetPatternFunction. + env.getListener().post( + PatternExpandingError.failed(keyOrException.getOriginalPattern(), e.getMessage())); // We generally skip patterns that don't parse. We report a parsing failed exception to the // event bus here, but not in determineTests below, which goes through the same list. Note // that the TargetPatternFunction otherwise reports these events (but only if the target @@ -249,12 +255,9 @@ final class TargetPatternPhaseFunction implements SkyFunction { builder.setError(); } } + Map<SkyKey, ValueOrException<TargetParsingException>> resolvedPatterns = env.getValuesOrThrow(patternSkyKeys, TargetParsingException.class); - if (env.valuesMissing()) { - return null; - } - for (TargetPatternKey pattern : patternSkyKeys) { TargetPatternValue value; try { @@ -262,20 +265,32 @@ final class TargetPatternPhaseFunction implements SkyFunction { } catch (TargetParsingException e) { String rawPattern = pattern.getPattern(); String errorMessage = e.getMessage(); - env.getListener().post(PatternExpandingError.skipped(rawPattern, errorMessage)); + failedPatterns.add(rawPattern); + env.getListener().post(PatternExpandingError.failed(rawPattern, errorMessage)); env.getListener().handle(Event.error("Skipping '" + rawPattern + "': " + errorMessage)); builder.setError(); continue; } + if (value == null) { + continue; + } // TODO(ulfjack): This is terribly inefficient. ResolvedTargets<Target> asTargets = TestSuiteExpansionFunction.labelsToTargets( env, value.getTargets().getTargets(), value.getTargets().hasError()); + if (asTargets == null) { + continue; + } if (pattern.isNegative()) { builder.filter(Predicates.not(Predicates.in(asTargets.getTargets()))); } else { builder.merge(asTargets); } } + // Only check for missing values after reporting errors. Otherwise we will miss errors in the + // nokeep_going case. + if (env.valuesMissing()) { + return null; + } ResolvedTargets<Target> result = builder .filter(TargetUtils.tagFilter(options.getBuildTargetFilter())) |