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

3 files changed, 33 insertions, 26 deletions

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<AspectValueKey> 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<ConfiguredTarget> configuredTargets,
-      Collection<AspectValue> aspects,
-      final WalkableGraph graph,
-      ImmutableMap<PackageIdentifier, Path> packageRoots,
-      boolean analysisSuccessful)
-      throws InterruptedException {
+      SkyframeAnalysisResult skyframeAnalysisResult)
+          throws InterruptedException {
     Collection<Target> testsToRun = loadingResult.getTestsToRun();
+    Collection<ConfiguredTarget> configuredTargets = skyframeAnalysisResult.getConfiguredTargets();
     Collection<ConfiguredTarget> 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<Artifact> 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<ConfiguredTarget> configuredTargets;
   private final WalkableGraph walkableGraph;
   private final ImmutableList<AspectValue> aspects;
   private final ImmutableMap<PackageIdentifier, Path> packageRoots;
 
   public SkyframeAnalysisResult(
-      boolean hasError,
+      boolean hasLoadingError,
+      boolean hasAnalysisError,
       ImmutableList<ConfiguredTarget> configuredTargets,
       WalkableGraph walkableGraph,
       ImmutableList<AspectValue> aspects,
       ImmutableMap<PackageIdentifier, Path> 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<ConfiguredTarget> 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<SkyKey, ErrorInfo> 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(),