Fix: return an error from the analysis phase with keep_going in error cases. - if there are failed top-level aspects - if there are conflicting actions I ended up rewriting how errors are signaled from the SkyframeBuildView. I think this is safe, but please review carefully. -- MOS_MIGRATED_REVID=113150100
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 eed892c..d409560 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
@@ -507,7 +507,6 @@ int numTargetsToAnalyze = nodes.size(); int numSuccessful = skyframeAnalysisResult.getConfiguredTargets().size(); - boolean analysisSuccessful = (numSuccessful == numTargetsToAnalyze); if (0 < numSuccessful && numSuccessful < numTargetsToAnalyze) { String msg = String.format("Analysis succeeded for only %d of %d top-level targets", numSuccessful, numTargetsToAnalyze); @@ -515,6 +514,7 @@ LOG.info(msg); } + boolean analysisSuccessful = !skyframeAnalysisResult.hasError(); AnalysisResult result = createResult( eventHandler,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java index 7db9ca2..371ba8f 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeAnalysisResult.java
@@ -26,22 +26,29 @@ * Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe. */ public class SkyframeAnalysisResult { + private final boolean hasError; private final ImmutableList<ConfiguredTarget> configuredTargets; private final WalkableGraph walkableGraph; private final ImmutableList<AspectValue> aspects; private final ImmutableMap<PackageIdentifier, Path> packageRoots; public SkyframeAnalysisResult( + boolean hasError, ImmutableList<ConfiguredTarget> configuredTargets, WalkableGraph walkableGraph, ImmutableList<AspectValue> aspects, ImmutableMap<PackageIdentifier, Path> packageRoots) { + this.hasError = hasError; this.configuredTargets = configuredTargets; this.walkableGraph = walkableGraph; this.aspects = aspects; this.packageRoots = packageRoots; } + public boolean hasError() { + return hasError; + } + public Collection<ConfiguredTarget> getConfiguredTargets() { return configuredTargets; }
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 6d5b0bc..60182d8 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
@@ -280,6 +280,7 @@ if (!result.hasError() && badActions.isEmpty()) { setDeserializedArtifactOwners(); return new SkyframeAnalysisResult( + false, ImmutableList.copyOf(goodCts), result.getWalkableGraph(), ImmutableList.copyOf(goodAspects), @@ -402,6 +403,7 @@ } setDeserializedArtifactOwners(); return new SkyframeAnalysisResult( + result.hasError() || !badActions.isEmpty(), ImmutableList.copyOf(goodCts), result.getWalkableGraph(), ImmutableList.copyOf(goodAspects),
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java index c365cab..5719111 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ToplevelSkylarkAspectFunction.java
@@ -56,7 +56,7 @@ } catch (SkylarkImportFailedException e) { env.getListener().handle(Event.error(e.getMessage())); throw new LoadSkylarkAspectFunctionException( - new AspectCreationException(e.getMessage()), skyKey); + new AspectCreationException(e.getMessage())); } if (labelLookupMap == null) { return null; @@ -67,7 +67,7 @@ skylarkAspect = AspectFunction.loadSkylarkAspect( env, labelLookupMap.get(extensionFile), skylarkValueName); } catch (AspectCreationException e) { - throw new LoadSkylarkAspectFunctionException(e, skyKey); + throw new LoadSkylarkAspectFunctionException(e); } if (skylarkAspect == null) { return null; @@ -92,8 +92,8 @@ * Exceptions thrown from ToplevelSkylarkAspectFunction. */ public class LoadSkylarkAspectFunctionException extends SkyFunctionException { - public LoadSkylarkAspectFunctionException(AspectCreationException cause, SkyKey childKey) { - super(cause, childKey); + public LoadSkylarkAspectFunctionException(AspectCreationException cause) { + super(cause, Transience.PERSISTENT); } } }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java index d09f6fe0..69573f7 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -825,8 +825,7 @@ defaultFlags().with(Flag.KEEP_GOING), "//conflict:_objs/x/conflict/foo.pic.o", "//conflict:x"); - // TODO(ulfjack): We print an error message but don't return an error, apparently. - //assertThat(result.hasError()).isTrue(); + assertThat(result.hasError()).isTrue(); // Expect to reach this line without a Precondition-triggered NullPointerException. assertContainsEvent( "file 'conflict/_objs/x/conflict/foo.pic.o' is generated by these conflicting actions");
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java index 2e6935c..c8960cb 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java
@@ -48,6 +48,10 @@ */ @RunWith(JUnit4.class) public class SkylarkAspectsTest extends AnalysisTestCase { + protected boolean keepGoing() { + return false; + } + @Test public void testAspect() throws Exception { scratch.file( @@ -308,8 +312,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -336,8 +341,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -358,8 +364,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -384,8 +391,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -405,8 +413,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -420,8 +429,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -434,8 +444,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -450,8 +461,9 @@ reporter.removeHandler(failFastHandler); try { - update(ImmutableList.of("foo/aspect.bzl%MyAspect"), "//test:xxx"); - fail(); + AnalysisResult result = update(ImmutableList.of("foo/aspect.bzl%MyAspect"), "//test:xxx"); + assertThat(keepGoing()).isTrue(); + assertThat(result.hasError()).isTrue(); } catch (ViewCreationFailedException e) { // expect to fail. } @@ -525,7 +537,6 @@ assertNoEvents(); } - private ConfiguredTarget getConfiguredTargetForAspectFragment( String fullFieldName, String fragments, @@ -567,8 +578,25 @@ " attr = ['yyy'],", ")"); - update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + AnalysisResult result = update(ImmutableList.of("test/aspect.bzl%MyAspect"), "//test:xxx"); + if (result.hasError()) { + assertThat(keepGoing()).isTrue(); + throw new ViewCreationFailedException("Analysis failed"); + } return getConfiguredTarget("//test:xxx"); } + + @RunWith(JUnit4.class) + public static final class WithKeepGoing extends SkylarkAspectsTest { + @Override + protected FlagBuilder defaultFlags() { + return new FlagBuilder().with(Flag.KEEP_GOING); + } + + @Override + protected boolean keepGoing() { + return true; + } + } }