Update Skyframe builder to return exit code based on reported error map, when

keep_going is turned on

Current implementation only supports custom exit code from {@link
ActionExecutionException} in fail-fast mode. This change allows appropriate
error code to be returned based on all reported errors during build phase.

--
MOS_MIGRATED_REVID=124987173

2 files changed, 90 insertions, 16 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/BuildFailedException.java b/src/main/java/com/google/devtools/build/lib/actions/BuildFailedException.java
index 17861b08a9..e7d8ff09ac 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/BuildFailedException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/BuildFailedException.java
@@ -54,6 +54,10 @@ public class BuildFailedException extends Exception {
     this(message, false, null, ImmutableList.<Label>of());
   }
 
+  public BuildFailedException(String message, ExitCode exitCode) {
+    this(message, false, null, ImmutableList.<Label>of(), false, exitCode);
+  }
+
   public BuildFailedException(String message, boolean catastrophic) {
     this(message, catastrophic, null, ImmutableList.<Label>of());
   }
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
index 257b25922a..afc68b17b2 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/SkyframeBuilder.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.buildtool;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Range;
@@ -38,6 +39,7 @@ import com.google.devtools.build.lib.skyframe.AspectValue;
 import com.google.devtools.build.lib.skyframe.Builder;
 import com.google.devtools.build.lib.skyframe.SkyframeExecutor;
 import com.google.devtools.build.lib.util.AbruptExitException;
+import com.google.devtools.build.lib.util.ExitCode;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.ModifiedFileSet;
@@ -47,6 +49,10 @@ import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.SkyKey;
 
 import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.LinkedList;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -109,7 +115,7 @@ public class SkyframeBuilder implements Builder {
         .post(new ExecutionProgressReceiverAvailableEvent(executionProgressReceiver));
     ResourceManager.instance().setEventBus(skyframeExecutor.getEventBus());
 
-    boolean success = false;
+    List<ExitCode> exitCodes = new LinkedList<>();
     EvaluationResult<?> result;
 
     ActionExecutionStatusReporter statusReporter = ActionExecutionStatusReporter.create(
@@ -142,10 +148,10 @@ public class SkyframeBuilder implements Builder {
               actionCacheChecker,
               executionProgressReceiver);
       // progressReceiver is finished, so unsynchronized access to builtTargets is now safe.
-      success = processResult(reporter, result, keepGoing, skyframeExecutor);
+      Optional<ExitCode> exitCode = processResult(reporter, result, keepGoing, skyframeExecutor);
 
       Preconditions.checkState(
-          !success
+          exitCode != null
               || result.keyNames().size()
                   == (artifacts.size()
                       + targetsToBuild.size()
@@ -155,6 +161,10 @@ public class SkyframeBuilder implements Builder {
           result,
           artifacts);
 
+      if (exitCode != null) {
+        exitCodes.add(exitCode.orNull());
+      }
+
       // Run exclusive tests: either tagged as "exclusive" or is run in an invocation with
       // --test_output=streamed.
       isBuildingExclusiveArtifacts.set(true);
@@ -176,11 +186,16 @@ public class SkyframeBuilder implements Builder {
                 numJobs,
                 actionCacheChecker,
                 null);
-        boolean exclusiveSuccess = processResult(reporter, result, keepGoing, skyframeExecutor);
-        Preconditions.checkState(!exclusiveSuccess || !result.keyNames().isEmpty(),
+        exitCode = processResult(reporter, result, keepGoing, skyframeExecutor);
+        Preconditions.checkState(
+            exitCode != null || !result.keyNames().isEmpty(),
             "Build reported as successful but test %s not executed: %s",
-            exclusiveTest, result);
-        success &= exclusiveSuccess;
+            exclusiveTest,
+            result);
+
+        if (exitCode != null) {
+          exitCodes.add(exitCode.orNull());
+        }
       }
     } finally {
       watchdog.stop();
@@ -189,20 +204,33 @@ public class SkyframeBuilder implements Builder {
       statusReporter.unregisterFromEventBus();
     }
 
-    if (!success) {
-      throw new BuildFailedException();
+    if (!exitCodes.isEmpty()) {
+      if (keepGoing) {
+        // Use the exit code with the highest priority.
+        throw new BuildFailedException(
+            null, Collections.max(exitCodes, ExitCodeComparator.INSTANCE));
+      } else {
+        throw new BuildFailedException();
+      }
     }
   }
 
   /**
    * Process the Skyframe update, taking into account the keepGoing setting.
    *
-   * <p>Returns false if the update() failed, but we should continue. Returns true on success.
+   * <p> Returns optional {@link ExitCode} based on following conditions:
+   *    1. null, if result had no errors.
+   *    2. Optional.absent(), if result had errors but none of the errors specified an exit code.
+   *    3. Optional.of(e), if result had errors and one of them specified exit code 'e'.
    * Throws on fail-fast failures.
    */
-  private static boolean processResult(EventHandler eventHandler, EvaluationResult<?> result,
-      boolean keepGoing, SkyframeExecutor skyframeExecutor)
-          throws BuildFailedException, TestExecException {
+  @Nullable
+  private static Optional<ExitCode> processResult(
+      EventHandler eventHandler,
+      EvaluationResult<?> result,
+      boolean keepGoing,
+      SkyframeExecutor skyframeExecutor)
+      throws BuildFailedException, TestExecException {
     if (result.hasError()) {
       for (Map.Entry<SkyKey, ErrorInfo> entry : result.errorMap().entrySet()) {
         Iterable<CycleInfo> cycles = entry.getValue().getCycleInfo();
@@ -213,8 +241,26 @@ public class SkyframeBuilder implements Builder {
         rethrow(result.getCatastrophe());
       }
       if (keepGoing) {
-        // keepGoing doesn't throw if there were just ordinary errors.
-        return false;
+        // If build fails and keepGoing is true, an exit code is assigned using reported errors
+        // in the following order:
+        //   1. First infrastructure error with non-null exit code
+        //   2. First non-infrastructure error with non-null exit code
+        //   3. Null (later default to 1)
+        ExitCode exitCode = null;
+        for (Map.Entry<SkyKey, ErrorInfo> error : result.errorMap().entrySet()) {
+          Throwable cause = error.getValue().getException();
+          if (cause instanceof ActionExecutionException) {
+            ActionExecutionException actionExecutionCause = (ActionExecutionException) cause;
+            ExitCode code = actionExecutionCause.getExitCode();
+            // Update global exit code when current exit code is not null and global exit code has
+            // a lower 'reporting' priority.
+            if (ExitCodeComparator.INSTANCE.compare(code, exitCode) > 0) {
+              exitCode = code;
+            }
+          }
+        }
+
+        return Optional.fromNullable(exitCode);
       }
       ErrorInfo errorInfo = Preconditions.checkNotNull(result.getError(), result);
       Exception exception = errorInfo.getException();
@@ -229,7 +275,8 @@ public class SkyframeBuilder implements Builder {
         rethrow(exception);
       }
     }
-    return true;
+
+    return null;
   }
 
   /** Figure out why an action's execution failed and rethrow the right kind of exception. */
@@ -280,4 +327,27 @@ public class SkyframeBuilder implements Builder {
     }
     return count;
   }
+
+  /**
+   * A comparator to determine the reporting priority of {@link ExitCode}.
+   *
+   * <p> Priority: infrastructure exit codes > non-infrastructure exit codes > null exit codes.
+   */
+  private static class ExitCodeComparator implements Comparator<ExitCode> {
+    private static final ExitCodeComparator INSTANCE = new ExitCodeComparator();
+
+    @Override
+    public int compare(ExitCode c1, ExitCode c2) {
+      // returns POSITIVE result when the priority of c1 is HIGHER than the priority of c2
+      return getPriority(c1) - getPriority(c2);
+    }
+
+    private int getPriority(ExitCode code) {
+      if (code == null) {
+        return 0;
+      } else {
+        return code.isInfrastructureFailure() ? 2 : 1;
+      }
+    }
+  }
 }