Improve error messages where there's an IOException reading a source …

…file: report the final status to the user as coming from an IOException, not a "missing file." We still had the right failure detail, but the error message is misleading. Also print out top-level "missing input" failure messages in keep-going mode, not just no-keep-going. Noticed as part of this that there were no non-test callers of BuildFailedException#getRootCauses(), and got rid of it. Unfortunately, had to do that in this CL, since without the top-level printing in keep-going mode, we actually weren't ever printing those messages in some cases. PiperOrigin-RevId: 363204012
bazelbuild · Mar 16, 2021 · 8e9d300 · 8e9d300
1 parent 203a506
commit 8e9d300
Show file tree

Hide file tree

Showing 13 changed files with 292 additions and 185 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
@@ -16,10 +16,6 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import com.google.devtools.build.lib.causes.Cause;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.skyframe.DetailedException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
@@ -41,28 +37,24 @@
 @ThreadSafe
 public class BuildFailedException extends Exception implements DetailedException {
   private final boolean catastrophic;
-  private final NestedSet<Cause> rootCauses;
   private final boolean errorAlreadyShown;
   private final DetailedExitCode detailedExitCode;
 
   public BuildFailedException(String message, DetailedExitCode detailedExitCode) {
     this(
         message,
         /*catastrophic=*/ false,
-        NestedSetBuilder.emptySet(Order.STABLE_ORDER),
         /*errorAlreadyShown=*/ false,
         detailedExitCode);
   }
 
   public BuildFailedException(
       String message,
       boolean catastrophic,
-      NestedSet<Cause> rootCauses,
       boolean errorAlreadyShown,
       DetailedExitCode detailedExitCode) {
     super(message);
     this.catastrophic = catastrophic;
-    this.rootCauses = rootCauses;
     this.errorAlreadyShown = errorAlreadyShown;
     this.detailedExitCode = checkNotNull(detailedExitCode);
   }
@@ -71,10 +63,6 @@ public boolean isCatastrophic() {
     return catastrophic;
   }
 
-  public NestedSet<Cause> getRootCauses() {
-    return rootCauses;
-  }
-
   public boolean isErrorAlreadyShown() {
     return errorAlreadyShown || getMessage() == null;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/actions/InputFileErrorException.java b/src/main/java/com/google/devtools/build/lib/actions/InputFileErrorException.java
@@ -0,0 +1,27 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.actions;
+
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import java.io.IOException;
+
+/**
+ * Signals that a source file requested by a top-level target (not via an action) was missing or
+ * that access threw an {@link IOException}.
+ */
+public class InputFileErrorException extends BuildFailedException {
+  public InputFileErrorException(String message, DetailedExitCode detailedExitCode) {
+    super(message, /*catastrophic=*/ false, /*errorAlreadyShown=*/ true, detailedExitCode);
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/MissingInputFileException.java b/src/main/java/com/google/devtools/build/lib/actions/MissingInputFileException.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.actions;
 
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.skyframe.DetailedException;
 import com.google.devtools.build.lib.util.DetailedExitCode;
 import net.starlark.java.syntax.Location;
 
@@ -25,14 +26,21 @@
  * <p>If a missing input file is an input to an action, an {@link ActionExecutionException} is
  * thrown instead.
  */
-public class MissingInputFileException extends BuildFailedException {
+public class MissingInputFileException extends Exception implements DetailedException {
+  private final DetailedExitCode detailedExitCode;
   private final Location location;
 
   public MissingInputFileException(FailureDetail failureDetail, Location location) {
-    super(failureDetail.getMessage(), DetailedExitCode.of(failureDetail));
+    super(failureDetail.getMessage());
+    this.detailedExitCode = DetailedExitCode.of(failureDetail);
     this.location = location;
   }
 
+  @Override
+  public DetailedExitCode getDetailedExitCode() {
+    return detailedExitCode;
+  }
+
   /**
    * Return a location where this input file is referenced. If there are multiple such locations,
    * one is chosen arbitrarily. If there are none, return null.

diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -408,9 +408,12 @@ public BuildResult processRequest(BuildRequest request, TargetValidator validato
       buildTargets(request, result, validator);
       detailedExitCode = DetailedExitCode.success();
     } catch (BuildFailedException e) {
-      if (e.isErrorAlreadyShown()) {
-        // The actual error has already been reported by the Builder.
-      } else {
+      if (!e.isErrorAlreadyShown()) {
+        // The actual error has not already been reported by the Builder.
+        // TODO(janakr): This is wrong: --keep_going builds with errors don't have a message in
+        //  this BuildFailedException, so any error message that is only reported here will be
+        //  missing for --keep_going builds. All error reporting should be done at the site of the
+        //  error, if only for clearer behavior.
         reportExceptionError(e);
       }
       if (e.isCatastrophic()) {

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
@@ -28,8 +28,8 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.BuildFailedException;
 import com.google.devtools.build.lib.actions.Executor;
+import com.google.devtools.build.lib.actions.InputFileErrorException;
 import com.google.devtools.build.lib.actions.MetadataProvider;
-import com.google.devtools.build.lib.actions.MissingInputFileException;
 import com.google.devtools.build.lib.actions.ResourceManager;
 import com.google.devtools.build.lib.actions.TestExecException;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
@@ -289,9 +289,9 @@ private static DetailedExitCode processResult(
                 DetailedExitCodeComparator.chooseMoreImportantWithFirstIfTie(
                     detailedExitCode, ((DetailedException) cause).getDetailedExitCode());
             if (!(cause instanceof ActionExecutionException)
-                && !(cause instanceof MissingInputFileException)) {
+                && !(cause instanceof InputFileErrorException)) {
               logger.atWarning().withCause(cause).log(
-                  "Non-action-execution/missing-input exception for %s", error);
+                  "Non-action-execution/input-error exception for %s", error);
             }
           } else {
             undetailedCause = cause;
@@ -355,12 +355,11 @@ public static void rethrow(Throwable cause) throws BuildFailedException, TestExe
       throw new BuildFailedException(
           message,
           actionExecutionCause.isCatastrophe(),
-          actionExecutionCause.getRootCauses(),
           /*errorAlreadyShown=*/ !actionExecutionCause.showError(),
           actionExecutionCause.getDetailedExitCode());
     }
-    if (cause instanceof MissingInputFileException) {
-      throw (MissingInputFileException) cause;
+    if (cause instanceof InputFileErrorException) {
+      throw (InputFileErrorException) cause;
     }
     if (cause instanceof BuildFileNotFoundException) {
       // Sadly, this can happen because we may load new packages during input discovery. Any

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -16,6 +16,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
+import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableCollection;
@@ -76,7 +77,7 @@
 import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
 import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.skyframe.ActionRewindStrategy.RewindPlan;
-import com.google.devtools.build.lib.skyframe.ArtifactFunction.MissingFileArtifactValue;
+import com.google.devtools.build.lib.skyframe.ArtifactFunction.SourceFileInErrorArtifactValue;
 import com.google.devtools.build.lib.skyframe.ArtifactNestedSetFunction.ArtifactNestedSetEvalException;
 import com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.ActionPostprocessing;
 import com.google.devtools.build.lib.util.DetailedExitCode;
@@ -105,6 +106,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.IntFunction;
 import javax.annotation.Nullable;
@@ -968,7 +970,7 @@ private DiscoveredState addDiscoveredInputs(
       } else if (retrievedMetadata instanceof ActionExecutionValue) {
         inputData.putWithNoDepOwner(
             input, ((ActionExecutionValue) retrievedMetadata).getExistingFileArtifactValue(input));
-      } else if (retrievedMetadata instanceof MissingFileArtifactValue) {
+      } else if (retrievedMetadata instanceof SourceFileInErrorArtifactValue) {
         inputData.putWithNoDepOwner(input, FileArtifactValue.MISSING_FILE_MARKER);
       } else if (retrievedMetadata instanceof FileArtifactValue) {
         inputData.putWithNoDepOwner(input, (FileArtifactValue) retrievedMetadata);
@@ -1129,7 +1131,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
     // some deps are still missing.
     boolean populateInputData = !env.valuesMissing();
     // Errors unexpected: save garbage on initialization.
-    List<LabelCause> missingArtifactCauses = Lists.newArrayListWithCapacity(0);
+    List<LabelCause> sourceArtifactErrorCauses = Lists.newArrayListWithCapacity(0);
     List<NestedSet<Cause>> transitiveCauses = Lists.newArrayListWithCapacity(0);
     ImmutableList<Artifact> allInputsList = allInputs.toList();
     S inputArtifactData =
@@ -1182,7 +1184,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
                   "Unexpected IOException for generated artifact: " + input + ", " + action, e));
         }
         if (mandatory) {
-          missingArtifactCauses.add(
+          sourceArtifactErrorCauses.add(
               createLabelCause(
                   input,
                   ArtifactFunction.makeIOExceptionSourceInputFileValue(input, e),
@@ -1208,11 +1210,11 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
             e);
       }
 
-      if (value instanceof MissingFileArtifactValue) {
+      if (value instanceof SourceFileInErrorArtifactValue) {
         if (mandatory) {
-          missingArtifactCauses.add(
+          sourceArtifactErrorCauses.add(
               createLabelCause(
-                  input, (MissingFileArtifactValue) value, action.getOwner().getLabel()));
+                  input, (SourceFileInErrorArtifactValue) value, action.getOwner().getLabel()));
           continue;
         } else {
           value = FileArtifactValue.MISSING_FILE_MARKER;
@@ -1232,20 +1234,21 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
       }
     }
 
-    if (!missingArtifactCauses.isEmpty()) {
-      for (LabelCause missingInput : missingArtifactCauses) {
+    if (!sourceArtifactErrorCauses.isEmpty()) {
+      for (LabelCause missingInput : sourceArtifactErrorCauses) {
         skyframeActionExecutor.printError(missingInput.getMessage(), action);
       }
     }
     // We need to rethrow first exception because it can contain useful error message
     if (firstActionExecutionException != null) {
-      if (missingArtifactCauses.isEmpty() && (checkNotNull(transitiveCauses, action).size() == 1)) {
+      if (sourceArtifactErrorCauses.isEmpty()
+          && (checkNotNull(transitiveCauses, action).size() == 1)) {
         // In the case a single action failed, just propagate the exception upward. This avoids
         // having to copy the root causes to the upwards transitive closure.
         throw firstActionExecutionException;
       }
       NestedSetBuilder<Cause> allCauses =
-          NestedSetBuilder.<Cause>stableOrder().addAll(missingArtifactCauses);
+          NestedSetBuilder.<Cause>stableOrder().addAll(sourceArtifactErrorCauses);
       transitiveCauses.forEach(allCauses::addTransitive);
       throw new ActionExecutionException(
           firstActionExecutionException.getMessage(),
@@ -1256,8 +1259,8 @@ private <S extends ActionInputMapSink, R> R accumulateInputs(
           firstActionExecutionException.getDetailedExitCode());
     }
 
-    if (!missingArtifactCauses.isEmpty()) {
-      throw createMissingInputsException(action, missingArtifactCauses);
+    if (!sourceArtifactErrorCauses.isEmpty()) {
+      throw throwSourceErrorException(action, sourceArtifactErrorCauses);
     }
     return accumulateInputResultsFactory.create(
         inputArtifactData,
@@ -1321,10 +1324,10 @@ private <S extends ActionInputMapSink, R> R accumulateInputsWithNestedSet(
 
     for (Artifact input : allInputsList) {
       SkyValue value = ArtifactNestedSetFunction.getInstance().getValueForKey(Artifact.key(input));
-      if (value instanceof MissingFileArtifactValue) {
+      if (value instanceof SourceFileInErrorArtifactValue) {
         if (actionExecutionFunctionExceptionHandler.isMandatory(input)) {
           actionExecutionFunctionExceptionHandler.accumulateMissingFileArtifactValue(
-              input, (MissingFileArtifactValue) value);
+              input, (SourceFileInErrorArtifactValue) value);
           continue;
         } else {
           value = FileArtifactValue.MISSING_FILE_MARKER;
@@ -1341,7 +1344,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputsWithNestedSet(
           env);
     }
     // After accumulating the inputs, we might find some mandatory artifact with
-    // MissingFileArtifactValue.
+    // SourceFileInErrorArtifactValue.
     actionExecutionFunctionExceptionHandler.maybeThrowException();
 
     return accumulateInputResultsFactory.create(
@@ -1353,7 +1356,7 @@ private <S extends ActionInputMapSink, R> R accumulateInputsWithNestedSet(
   }
 
   static LabelCause createLabelCause(
-      Artifact input, MissingFileArtifactValue missingValue, Label labelInCaseOfBug) {
+      Artifact input, SourceFileInErrorArtifactValue missingValue, Label labelInCaseOfBug) {
     Label inputLabel = input.getOwner();
     if (inputLabel == null) {
       BugReport.sendBugReport(
@@ -1602,7 +1605,7 @@ private void handleIOExceptionFromSkykey(SkyKey key, IOException e) {
       }
     }
 
-    void accumulateMissingFileArtifactValue(Artifact input, MissingFileArtifactValue value) {
+    void accumulateMissingFileArtifactValue(Artifact input, SourceFileInErrorArtifactValue value) {
       missingArtifactCauses.add(createLabelCause(input, value, action.getOwner().getLabel()));
     }
 
@@ -1632,7 +1635,7 @@ void maybeThrowException() throws ActionExecutionException {
         for (LabelCause missingInput : missingArtifactCauses) {
           skyframeActionExecutor.printError(missingInput.getMessage(), action);
         }
-        throw createMissingInputsException(action, missingArtifactCauses);
+        throw throwSourceErrorException(action, missingArtifactCauses);
       }
     }
 
@@ -1671,19 +1674,69 @@ private void handleIOExceptionPerArtifact(Artifact input, IOException e) {
     }
   }
 
-  private static ActionExecutionException createMissingInputsException(
-      Action action, List<LabelCause> missingArtifactCauses) {
+  /**
+   * Called when there are no action execution errors (whose reporting hides missing sources), but
+   * there was at least one missing/io exception-triggering source artifact. Returns a {@link
+   * DetailedExitCode} constructed from {@code sourceArtifactErrorCauses} specific to a single such
+   * artifact and an error message suitable as the message to a thrown exception that summarizes the
+   * findings.
+   */
+  static Pair<DetailedExitCode, String> createSourceErrorCodeAndMessage(
+      List<? extends Cause> sourceArtifactErrorCauses, Object debugInfo) {
+    AtomicBoolean sawIOException = new AtomicBoolean();
+    AtomicBoolean sawMissingFile = new AtomicBoolean();
     DetailedExitCode prioritizedDetailedExitCode =
-        missingArtifactCauses.stream()
-            .map(LabelCause::getDetailedExitCode)
+        sourceArtifactErrorCauses.stream()
+            .map(Cause::getDetailedExitCode)
+            .peek(
+                code -> {
+                  if (code.getFailureDetail() == null) {
+                    BugReport.sendBugReport(
+                        new NullPointerException(
+                            "Code " + code + " had no failure detail for " + debugInfo));
+                    return;
+                  }
+                  switch (code.getFailureDetail().getExecution().getCode()) {
+                    case SOURCE_INPUT_IO_EXCEPTION:
+                      sawIOException.set(true);
+                      break;
+                    case SOURCE_INPUT_MISSING:
+                      sawMissingFile.set(true);
+                      break;
+                    default:
+                      BugReport.sendBugReport(
+                          new IllegalStateException(
+                              "Unexpected error code in " + code + " for " + debugInfo));
+                  }
+                })
             .max(DetailedExitCodeComparator.INSTANCE)
             .get();
-    return new ActionExecutionException(
-        missingArtifactCauses.size() + " input file(s) do not exist",
-        action,
-        NestedSetBuilder.wrap(Order.STABLE_ORDER, missingArtifactCauses),
-        /*catastrophe=*/ false,
-        prioritizedDetailedExitCode);
+    String errorMessage =
+        sourceArtifactErrorCauses.size()
+            + " input file(s) "
+            + Joiner.on(" or ")
+                .skipNulls()
+                .join(
+                    sawIOException.get() ? "are in error" : null,
+                    sawMissingFile.get() ? "do not exist" : null);
+    return Pair.of(prioritizedDetailedExitCode, errorMessage);
+  }
+
+  private ActionExecutionException throwSourceErrorException(
+      Action action, List<? extends Cause> sourceArtifactErrorCauses)
+      throws ActionExecutionException {
+    Pair<DetailedExitCode, String> codeAndMessage =
+        createSourceErrorCodeAndMessage(sourceArtifactErrorCauses, action);
+    ActionExecutionException ex =
+        new ActionExecutionException(
+            codeAndMessage.getSecond(),
+            action,
+            NestedSetBuilder.wrap(Order.STABLE_ORDER, sourceArtifactErrorCauses),
+            /*catastrophe=*/ false,
+            codeAndMessage.getFirst());
+    skyframeActionExecutor.printError(ex.getMessage(), action);
+    // Don't actually return: throw exception directly so caller can't get it wrong.
+    throw ex;
   }
 
   private static DetailedExitCode createDetailedExitCodeForMissingDiscoveredInput(String message) {