diff options
9 files changed, 114 insertions, 35 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java index 63abf1be0b..58d67f8309 100644 --- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java +++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetParsingCompleteEvent.java @@ -72,6 +72,7 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint } private final ImmutableList<String> originalTargetPattern; + private final ImmutableList<String> failedTargetPatterns; private final ImmutableSet<ThinTarget> targets; private final ImmutableSet<ThinTarget> filteredTargets; private final ImmutableSet<ThinTarget> testFilteredTargets; @@ -87,12 +88,14 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint Collection<Target> filteredTargets, Collection<Target> testFilteredTargets, List<String> originalTargetPattern, - Collection<Target> expandedTargets) { + Collection<Target> expandedTargets, + List<String> failedTargetPatterns) { this.targets = asThinTargets(targets); this.filteredTargets = asThinTargets(filteredTargets); this.testFilteredTargets = asThinTargets(testFilteredTargets); this.originalTargetPattern = ImmutableList.copyOf(originalTargetPattern); this.expandedTargets = asThinTargets(expandedTargets); + this.failedTargetPatterns = ImmutableList.copyOf(failedTargetPatterns); } @VisibleForTesting @@ -102,7 +105,16 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint ImmutableSet.<Target>of(), ImmutableSet.<Target>of(), ImmutableList.<String>of(), - targets); + targets, + ImmutableList.<String>of()); + } + + public ImmutableList<String> getOriginalTargetPattern() { + return originalTargetPattern; + } + + public ImmutableList<String> getFailedTargetPatterns() { + return failedTargetPatterns; } /** @return the parsed targets, which will subsequently be loaded */ @@ -137,6 +149,10 @@ public class TargetParsingCompleteEvent implements BuildEventWithOrderConstraint @Override public Collection<BuildEventId> getChildrenEvents() { ImmutableList.Builder<BuildEventId> childrenBuilder = ImmutableList.builder(); + for (String failedTargetPattern : failedTargetPatterns) { + childrenBuilder.add( + BuildEventId.targetPatternExpanded(ImmutableList.of(failedTargetPattern))); + } for (ThinTarget target : expandedTargets) { // Test suits won't produce target configuration and target-complete events, so do not // announce here completion as children. diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java index 773682af18..6af82547ca 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/BuildEventStreamer.java @@ -62,6 +62,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetView; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.events.Reporter; +import com.google.devtools.build.lib.pkgcache.TargetParsingCompleteEvent; import com.google.devtools.build.lib.util.Pair; import java.util.ArrayList; import java.util.Collection; @@ -591,6 +592,15 @@ public class BuildEventStreamer implements EventHandler { return true; } + if (event instanceof TargetParsingCompleteEvent) { + // If there is only one pattern and we have one failed pattern, then we already posted a + // pattern expanded error, so we don't post the completion event. + // TODO(ulfjack): This is brittle. It would be better to always post one PatternExpanded event + // for each pattern given on the command line instead of one event for all of them combined. + return ((TargetParsingCompleteEvent) event).getOriginalTargetPattern().size() == 1 + && !((TargetParsingCompleteEvent) event).getFailedTargetPatterns().isEmpty(); + } + return false; } 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())) diff --git a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java index 215c99224d..09fe4cd829 100644 --- a/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/pkgcache/LoadingPhaseRunnerTest.java @@ -105,6 +105,23 @@ public class LoadingPhaseRunnerTest { return false; } + private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception { + try { + tester.load(targetPattern); + fail(); + } catch (TargetParsingException e) { + // Expected. + tester.assertContainsError("circular symlinks detected"); + } + } + + private LoadingResult assertNoErrors(LoadingResult loadingResult) { + assertThat(loadingResult.hasTargetPatternError()).isFalse(); + assertThat(loadingResult.hasLoadingError()).isFalse(); + tester.assertNoEvents(); + return loadingResult; + } + @Test public void testSmoke() throws Exception { tester.addFile("base/BUILD", @@ -140,7 +157,32 @@ public class LoadingPhaseRunnerTest { tester.assertContainsWarning("Target pattern parsing failed."); PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); assertThat(err.getPattern()).containsExactly("//base:missing"); - assertThat(err.getSkipped()).isTrue(); + } + + @Test + public void testNonExistentPackageWithKeepGoing() throws Exception { + tester.addFile("base/BUILD", + "filegroup(name = 'hello', srcs = ['foo.txt'])"); + tester.loadKeepGoing("//base:hello", "//base:missing"); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//base:missing"); + TargetParsingCompleteEvent event = tester.findPostOnce(TargetParsingCompleteEvent.class); + assertThat(event.getOriginalTargetPattern()).containsExactly("//base:hello", "//base:missing"); + if (useSkyframeTargetPatternEval()) { + // The legacy code path wasn't updated to provide this information, and will be deleted soon. + assertThat(event.getFailedTargetPatterns()).containsExactly("//base:missing"); + } + } + + @Test + public void testNonExistentPackageWithoutKeepGoing() throws Exception { + try { + tester.load("//does/not/exist"); + fail(); + } catch (TargetParsingException expected) { + } + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//does/not/exist"); } @Test @@ -155,7 +197,6 @@ public class LoadingPhaseRunnerTest { tester.assertContainsWarning("Target pattern parsing failed."); PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); assertThat(err.getPattern()).containsExactly("//base:missing"); - assertThat(err.getSkipped()).isTrue(); } @Test @@ -567,7 +608,6 @@ public class LoadingPhaseRunnerTest { tester.assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1); PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); assertThat(err.getPattern()).containsExactly("//bad"); - assertThat(err.getSkipped()).isFalse(); } @Test @@ -678,24 +718,6 @@ public class LoadingPhaseRunnerTest { tester.assertContainsEventWithFrequency("cycle detected in extension", 1); PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); assertThat(err.getPattern()).containsExactly("//test:cycle1"); - assertThat(err.getSkipped()).isFalse(); - } - - private void assertCircularSymlinksDuringTargetParsing(String targetPattern) throws Exception { - try { - tester.load(targetPattern); - fail(); - } catch (TargetParsingException e) { - // Expected. - tester.assertContainsError("circular symlinks detected"); - } - } - - private LoadingResult assertNoErrors(LoadingResult loadingResult) { - assertThat(loadingResult.hasTargetPatternError()).isFalse(); - assertThat(loadingResult.hasLoadingError()).isFalse(); - tester.assertNoEvents(); - return loadingResult; } private static class LoadingPhaseTester { diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh index 5a8263d1f1..0c6b0e7a6d 100755 --- a/src/test/shell/integration/build_event_stream_test.sh +++ b/src/test/shell/integration/build_event_stream_test.sh @@ -673,7 +673,6 @@ function test_loading_failure() { # reason for the target expansion event not resulting in targets # being expanded. (bazel build --build_event_text_file=$TEST_log \ - --noexperimental_skyframe_target_pattern_evaluator \ //does/not/exist && fail "build failure expected") || true expect_log_once 'aborted' expect_log_once 'reason: LOADING_FAILURE' |