From b11d760369b3ad14c32f25dbd7a56f6b5c826733 Mon Sep 17 00:00:00 2001 From: Benjamin Peterson Date: Wed, 7 Mar 2018 00:19:00 -0800 Subject: Post PatternExpandingError from the skyframe target pattern evaluator as needed. This fixes https://github.com/bazelbuild/bazel/issues/4731. Also, add test coverage for posting of ParsingFailedEvent and handling of cycles in LoadingPhaseRunnerTest. Change-Id: I88c9d33417b9c3c7a06c92a6137d58f37b991b0c PiperOrigin-RevId: 188138972 --- .../build/lib/skyframe/PatternExpandingError.java | 8 +++ .../build/lib/skyframe/SkyframeExecutor.java | 30 ++++---- .../lib/skyframe/TargetPatternPhaseFunction.java | 1 + .../build/lib/pkgcache/LoadingPhaseRunnerTest.java | 83 ++++++++++++++++++---- 4 files changed, 95 insertions(+), 27 deletions(-) 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 1d94871501..f24f095ef2 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 @@ -35,6 +35,14 @@ public final class PatternExpandingError implements BuildEvent { this.skipped = skipped; } + public List getPattern() { + return pattern; + } + + public boolean getSkipped() { + return skipped; + } + public static PatternExpandingError failed(List pattern, String message) { return new PatternExpandingError(pattern, message, false); } 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 1514290052..4a8bf8ed80 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 @@ -2090,23 +2090,25 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory { buildDriver.evaluate(ImmutableList.of(key), keepGoing, /*numThreads=*/ 10, eventHandler); if (evalResult.hasError()) { ErrorInfo errorInfo = evalResult.getError(key); + TargetParsingException exc; if (!Iterables.isEmpty(errorInfo.getCycleInfo())) { - String errorMessage = "cycles detected during target parsing"; + exc = new TargetParsingException("cycles detected during target parsing"); getCyclesReporter().reportCycles(errorInfo.getCycleInfo(), key, eventHandler); - throw new TargetParsingException(errorMessage); - } - if (errorInfo.getException() != null) { - Exception e = errorInfo.getException(); - Throwables.propagateIfInstanceOf(e, TargetParsingException.class); - if (!keepGoing) { - // This is the same code as in SkyframeTargetPatternEvaluator; we allow any exception - // and turn it into a TargetParsingException here. - throw new TargetParsingException(e.getMessage()); - } - throw new IllegalStateException("Unexpected Exception type from TargetPatternPhaseValue " - + "for '" + targetPatterns + "'' with root causes: " - + Iterables.toString(errorInfo.getRootCauses()), e); + } else { + // TargetPatternPhaseFunction never directly throws. Thus, the only way + // evalResult.hasError() && keepGoing can hold is if there are cycles, which is handled + // above. + Preconditions.checkState(!keepGoing); + // Following SkyframeTargetPatternEvaluator, we convert any exception into a + // TargetParsingException. + Exception e = Preconditions.checkNotNull(errorInfo.getException()); + exc = + (e instanceof TargetParsingException) + ? (TargetParsingException) e + : new TargetParsingException(e.getMessage(), e); } + 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/TargetPatternPhaseFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/TargetPatternPhaseFunction.java index 28803ff1bb..ee6cddcb35 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 @@ -250,6 +250,7 @@ final class TargetPatternPhaseFunction implements SkyFunction { } catch (TargetParsingException e) { String rawPattern = pattern.getPattern(); String errorMessage = e.getMessage(); + env.getListener().post(PatternExpandingError.skipped(rawPattern, errorMessage)); env.getListener().handle(Event.error("Skipping '" + rawPattern + "': " + errorMessage)); builder.setError(); continue; 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 cf7c61bba0..de8523eecf 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 @@ -24,6 +24,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; +import com.google.common.collect.Iterators; +import com.google.common.collect.MoreCollectors; import com.google.devtools.build.lib.actions.ActionKeyContext; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.BuildView; @@ -43,6 +45,7 @@ import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.util.MockToolsConfig; import com.google.devtools.build.lib.skyframe.BazelSkyframeExecutorConstants; import com.google.devtools.build.lib.skyframe.DiffAwareness; +import com.google.devtools.build.lib.skyframe.PatternExpandingError; import com.google.devtools.build.lib.skyframe.SequencedSkyframeExecutor; import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker; import com.google.devtools.build.lib.skyframe.SkyframeExecutor; @@ -133,6 +136,9 @@ public class LoadingPhaseRunnerTest { assertThat(loadingResult.getTestsToRun()).isNull(); tester.assertContainsError("Skipping '//base:missing': no such package 'base'"); tester.assertContainsWarning("Target pattern parsing failed."); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//base:missing"); + assertThat(err.getSkipped()).isTrue(); } @Test @@ -145,6 +151,9 @@ public class LoadingPhaseRunnerTest { assertThat(loadingResult.getTestsToRun()).isNull(); tester.assertContainsError("Skipping '//base:missing': no such target '//base:missing'"); tester.assertContainsWarning("Target pattern parsing failed."); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//base:missing"); + assertThat(err.getSkipped()).isTrue(); } @Test @@ -158,6 +167,8 @@ public class LoadingPhaseRunnerTest { + "invalid package name 'foo//bar': " + "package names may not contain '//' path separators"); } + ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); + assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @Test @@ -182,6 +193,8 @@ public class LoadingPhaseRunnerTest { "invalid target format 'foo//bar:missing': " + "invalid package name 'foo//bar': " + "package names may not contain '//' path separators"); + ParsingFailedEvent err = tester.findPostOnce(ParsingFailedEvent.class); + assertThat(err.getPattern()).isEqualTo("foo//bar:missing"); } @Test @@ -540,6 +553,9 @@ public class LoadingPhaseRunnerTest { } catch (TargetParsingException expected) { } 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 @@ -607,11 +623,50 @@ public class LoadingPhaseRunnerTest { public void testParsingFailureReported() throws Exception { LoadingResult loadingResult = tester.loadKeepGoing("//does_not_exist"); assertThat(loadingResult.hasTargetPatternError()).isTrue(); - ParsingFailedEvent event = tester.findPost(ParsingFailedEvent.class); - assertThat(event).isNotNull(); + ParsingFailedEvent event = tester.findPostOnce(ParsingFailedEvent.class); assertThat(event.getPattern()).isEqualTo("//does_not_exist"); assertThat(event.getMessage()).contains("BUILD file not found on package path"); - assertThat(Iterables.filter(tester.getPosts(), ParsingFailedEvent.class)).hasSize(1); + } + + @Test + public void testCyclesKeepGoing() throws Exception { + tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')"); + tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')"); + tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')"); + if (useSkyframeTargetPatternEval()) { + // The skyframe target pattern evaluator isn't able to provide partial results in the presence + // of cycles, so it simply raises an exception rather than returning an empty LoadingResult. + try { + tester.load("//test:cycle1"); + fail(); + } catch (TargetParsingException e) { + assertThat(e).hasMessageThat().contains("cycles detected"); + } + } else { + LoadingResult loadingResult = tester.loadKeepGoing("//test:cycle1"); + assertThat(loadingResult.hasTargetPatternError()).isTrue(); + } + tester.assertContainsEventWithFrequency("cycle detected in extension", 1); + PatternExpandingError err = tester.findPostOnce(PatternExpandingError.class); + assertThat(err.getPattern()).containsExactly("//test:cycle1"); + assertThat(err.getSkipped()).isEqualTo(!useSkyframeTargetPatternEval()); + } + + @Test + public void testCyclesNoKeepGoing() throws Exception { + tester.addFile("test/BUILD", "load(':cycle1.bzl', 'make_cycle')"); + tester.addFile("test/cycle1.bzl", "load(':cycle2.bzl', 'make_cycle')"); + tester.addFile("test/cycle2.bzl", "load(':cycle1.bzl', 'make_cycle')"); + try { + tester.load("//test:cycle1"); + fail(); + } catch (TargetParsingException e) { + assertThat(e).hasMessageThat().contains("cycles detected"); + } + 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 { @@ -870,17 +925,19 @@ public class LoadingPhaseRunnerTest { filteredEvents(), expectedMessage, expectedFrequency); } - public Iterable getPosts() { - return storedErrors.getPosts(); - } - public T findPost(Class clazz) { - for (Postable p : storedErrors.getPosts()) { - if (clazz.isInstance(p)) { - return clazz.cast(p); - } - } - return null; + return Iterators.getNext( + storedErrors.getPosts().stream().filter(clazz::isInstance).map(clazz::cast).iterator(), + null); + } + + public T findPostOnce(Class clazz) { + return storedErrors + .getPosts() + .stream() + .filter(clazz::isInstance) + .map(clazz::cast) + .collect(MoreCollectors.onlyElement()); } } } -- cgit v1.2.3