From 53abece6ff52869587d907a1884243e64a82592c Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Wed, 3 Feb 2016 08:30:07 +0000 Subject: Correctly flag loading errors in the interleaved case. In the interleaved case, loading errors can now also be discovered during the analysis phase. Add a boolean flag to the SkyframeAnalysisResult to indicate that such an error happened, and pass it through in BuildView. Also refactor BuildView to simplify the code a bit - simply pass the SkyframeAnalysisResult to the createResult method. There is already an integration test for this - I'm adding a faster unit test in BuildViewTest, as this is part of the BuildView contract. -- MOS_MIGRATED_REVID=113716870 --- .../devtools/build/lib/analysis/BuildView.java | 32 ++++++++-------------- .../build/lib/skyframe/SkyframeAnalysisResult.java | 22 +++++++++++---- .../build/lib/skyframe/SkyframeBuildView.java | 5 +++- 3 files changed, 33 insertions(+), 26 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib') 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 35d3c123b1..3712e54e53 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 @@ -451,7 +451,6 @@ public class BuildView { List aspectKeys = new ArrayList<>(); for (String aspect : aspects) { - // Syntax: label%aspect int delimiterPosition = aspect.indexOf('%'); if (delimiterPosition >= 0) { @@ -514,18 +513,13 @@ public class BuildView { LOG.info(msg); } - boolean analysisSuccessful = !skyframeAnalysisResult.hasError(); AnalysisResult result = createResult( eventHandler, loadingResult, topLevelOptions, viewOptions, - skyframeAnalysisResult.getConfiguredTargets(), - skyframeAnalysisResult.getAspects(), - skyframeAnalysisResult.getWalkableGraph(), - skyframeAnalysisResult.getPackageRoots(), - analysisSuccessful); + skyframeAnalysisResult); LOG.info("Finished analysis"); return result; } @@ -535,13 +529,10 @@ public class BuildView { LoadingResult loadingResult, TopLevelArtifactContext topLevelOptions, BuildView.Options viewOptions, - Collection configuredTargets, - Collection aspects, - final WalkableGraph graph, - ImmutableMap packageRoots, - boolean analysisSuccessful) - throws InterruptedException { + SkyframeAnalysisResult skyframeAnalysisResult) + throws InterruptedException { Collection testsToRun = loadingResult.getTestsToRun(); + Collection configuredTargets = skyframeAnalysisResult.getConfiguredTargets(); Collection allTargetsToTest = null; if (testsToRun != null) { // Determine the subset of configured targets that are meant to be run as tests. @@ -584,12 +575,13 @@ public class BuildView { // Tests. This must come last, so that the exclusive tests are scheduled after everything else. scheduleTestsIfRequested(parallelTests, exclusiveTests, topLevelOptions, allTargetsToTest); - String error = !loadingResult.hasLoadingError() - ? (analysisSuccessful - ? null - : "execution phase succeeded, but not all targets were analyzed") - : "execution phase succeeded, but there were loading phase errors"; + String error = loadingResult.hasLoadingError() || skyframeAnalysisResult.hasLoadingError() + ? "execution phase succeeded, but there were loading phase errors" + : skyframeAnalysisResult.hasAnalysisError() + ? "execution phase succeeded, but not all targets were analyzed" + : null; + final WalkableGraph graph = skyframeAnalysisResult.getWalkableGraph(); final ActionGraph actionGraph = new ActionGraph() { @Nullable @Override @@ -605,7 +597,7 @@ public class BuildView { }; return new AnalysisResult( configuredTargets, - aspects, + skyframeAnalysisResult.getAspects(), allTargetsToTest, error, actionGraph, @@ -613,7 +605,7 @@ public class BuildView { parallelTests, exclusiveTests, topLevelOptions, - packageRoots); + skyframeAnalysisResult.getPackageRoots()); } private static NestedSet getBaselineCoverageArtifacts( 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 371ba8ff88..7db70f3e62 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,27 +26,39 @@ import java.util.Collection; * Encapsulates the raw analysis result of top level targets and aspects coming from Skyframe. */ public class SkyframeAnalysisResult { - private final boolean hasError; + private final boolean hasLoadingError; + private final boolean hasAnalysisError; private final ImmutableList configuredTargets; private final WalkableGraph walkableGraph; private final ImmutableList aspects; private final ImmutableMap packageRoots; public SkyframeAnalysisResult( - boolean hasError, + boolean hasLoadingError, + boolean hasAnalysisError, ImmutableList configuredTargets, WalkableGraph walkableGraph, ImmutableList aspects, ImmutableMap packageRoots) { - this.hasError = hasError; + this.hasLoadingError = hasLoadingError; + this.hasAnalysisError = hasAnalysisError; this.configuredTargets = configuredTargets; this.walkableGraph = walkableGraph; this.aspects = aspects; this.packageRoots = packageRoots; } - public boolean hasError() { - return hasError; + /** + * If the new simplified loading phase is enabled, then we can also see loading errors during the + * analysis phase. This method returns true if any such errors were encountered. However, you also + * always need to check if the loading result has an error! These will be merged eventually. + */ + public boolean hasLoadingError() { + return hasLoadingError; + } + + public boolean hasAnalysisError() { + return hasAnalysisError; } public Collection getConfiguredTargets() { 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 3f0aa18735..f97dae1f8a 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 @@ -283,7 +283,7 @@ public final class SkyframeBuildView { if (!result.hasError() && badActions.isEmpty()) { setDeserializedArtifactOwners(); return new SkyframeAnalysisResult( - false, + /*hasLoadingError=*/false, /*hasAnalysisError=*/false, ImmutableList.copyOf(goodCts), result.getWalkableGraph(), ImmutableList.copyOf(goodAspects), @@ -336,6 +336,7 @@ public final class SkyframeBuildView { throw new ViewCreationFailedException(errorMsg); } + boolean hasLoadingError = false; // --keep_going : We notify the error and return a ConfiguredTargetValue for (Map.Entry errorEntry : result.errorMap().entrySet()) { // Only handle errors of configured targets, not errors of top-level aspects. @@ -356,6 +357,7 @@ public final class SkyframeBuildView { if (cause instanceof ConfiguredValueCreationException) { ConfiguredValueCreationException ctCause = (ConfiguredValueCreationException) cause; for (Label rootCause : ctCause.getRootCauses()) { + hasLoadingError = true; eventBus.post(new LoadingFailureEvent(topLevelLabel, rootCause)); } analysisRootCause = ctCause.getAnalysisRootCause(); @@ -408,6 +410,7 @@ public final class SkyframeBuildView { } setDeserializedArtifactOwners(); return new SkyframeAnalysisResult( + hasLoadingError, result.hasError() || !badActions.isEmpty(), ImmutableList.copyOf(goodCts), result.getWalkableGraph(), -- cgit v1.2.3