From 32563ca1728a69437b26efa19d18eebfcecc4765 Mon Sep 17 00:00:00 2001
From: Googler <leba@google.com>
Date: Mon, 18 Sep 2023 03:10:53 -0700
Subject: [PATCH] [Skymeld] Avoid printing extra WARNINGS for execution
 failures in -k.

This is a divergence from the non-skymeld behavior: we should only _log_ unusual
exceptions in the execution phase and don't _print_ any warning.

PiperOrigin-RevId: 566246387
Change-Id: I1fb26dab8f40cef2f179d54ccbf3365d71df478f
---
 .../lib/skyframe/SkyframeErrorProcessor.java  | 33 ++++++++++++-------
 .../SkymeldBuildIntegrationTest.java          | 18 ++++++++++
 .../events/util/EventCollectionApparatus.java |  4 +++
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
index de69e9c220fd08..b1389964479004 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeErrorProcessor.java
@@ -311,7 +311,7 @@ static ErrorProcessingResult processErrors(
       boolean isExecutionException = isExecutionException(cause);
       if (keepGoing) {
         aggregatingResultBuilder.aggregateSingleResult(individualErrorProcessingResult);
-        printWarningMessage(isExecutionException, label, eventHandler);
+        logOrPrintWarnings(isExecutionException, label, eventHandler, cause);
       } else {
         noKeepGoingAnalysisExceptionAspect =
             throwOrReturnAspectAnalysisException(
@@ -569,17 +569,29 @@ private static DetailedExitCode sendBugReportAndCreateUnknownExecutionDetailedEx
     return createDetailedExecutionExitCode(message, UNKNOWN_EXECUTION);
   }
 
-  private static void printWarningMessage(
+  private static void logOrPrintWarnings(
       boolean isExecutionException,
       @Nullable Label topLevelLabel,
-      ExtendedEventHandler eventHandler) {
-    String warningMsg =
-        isExecutionException
-            ? String.format("errors encountered while building target '%s'", topLevelLabel)
-            : String.format(
+      ExtendedEventHandler eventHandler,
+      Exception cause) {
+    // For execution exceptions, we don't print any extra warning.
+    if (isExecutionException) {
+      if (isExecutionCauseWorthLogging(cause)) {
+        logger.atWarning().withCause(cause).log(
+            "Non-action-execution/input-error exception while building target %s", topLevelLabel);
+      }
+      return;
+    }
+    eventHandler.handle(
+        Event.warn(
+            String.format(
                 "errors encountered while analyzing target '%s': it will not be built",
-                topLevelLabel);
-    eventHandler.handle(Event.warn(warningMsg));
+                topLevelLabel)));
+  }
+
+  private static boolean isExecutionCauseWorthLogging(Throwable cause) {
+    return !(cause instanceof ActionExecutionException)
+        && !(cause instanceof InputFileErrorException);
   }
 
   private static boolean isValidErrorKeyType(Object errorKey) {
@@ -842,8 +854,7 @@ private static DetailedExitCode getDetailedExitCodeKeepGoing(EvaluationResult<?>
         detailedExitCode =
             DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie(
                 detailedExitCode, ((DetailedException) cause).getDetailedExitCode());
-        if (!(cause instanceof ActionExecutionException)
-            && !(cause instanceof InputFileErrorException)) {
+        if (isExecutionCauseWorthLogging(cause)) {
           logger.atWarning().withCause(cause).log(
               "Non-action-execution/input-error exception for %s", error);
         }
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java
index 3fdd186861e6cb..fee3d53a5ff557 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/SkymeldBuildIntegrationTest.java
@@ -628,6 +628,24 @@ public void targetAnalysisFailureNullBuild_correctErrorsPropagated(
             + " exist");
   }
 
+  // Regression test for b/300391729.
+  @Test
+  public void executionFailure_keepGoing_doesNotSpamWarnings() throws Exception {
+    addOptions("--keep_going");
+    writeExecutionFailureAspectBzl();
+    write(
+        "foo/BUILD",
+        "cc_library(name = 'foo', srcs = ['foo.cc'], deps = [':bar'])",
+        "cc_library(name = 'bar', srcs = ['bar.cc'])");
+    write("foo/foo.cc");
+    write("foo/bar.cc");
+    addOptions("--aspects=//foo:aspect.bzl%execution_err_aspect", "--output_groups=files");
+
+    assertThrows(BuildFailedException.class, () -> buildTarget("//foo/..."));
+    // No warnings.
+    events.assertNoWarnings();
+  }
+
   private void assertSingleAnalysisPhaseCompleteEventWithLabels(String... labels) {
     assertThat(eventsSubscriber.getAnalysisPhaseCompleteEvents()).hasSize(1);
     AnalysisPhaseCompleteEvent analysisPhaseCompleteEvent =
diff --git a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
index cc734fb69207b1..d11bdb12a31d67 100644
--- a/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/events/util/EventCollectionApparatus.java
@@ -159,6 +159,10 @@ public void assertNoWarningsOrErrors() {
     MoreAsserts.assertNoEvents(errors());
   }
 
+  public void assertNoWarnings() {
+    MoreAsserts.assertNoEvents(warnings());
+  }
+
   /**
    * Utility method: Assert that the {@link #collector()} has received an
    * info message with the {@code expectedMessage}.