Refactor ExecException to centralize logic for "messagePrefix" construction. Leads to a minor behavior change for error messages (additional colon). Don't think it's worth RELNOTES.
The fewer Action#describe() calls we have, the easier it will be to move them all to SkyframeActionExecutor#describeAction.
#11151
PiperOrigin-RevId: 336879499
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
index 218aae2..862337c 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
@@ -463,7 +463,7 @@
}
@Override
- public String describe() {
+ public final String describe() {
String progressMessage = getProgressMessage();
return progressMessage != null ? progressMessage : defaultProgressMessage();
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java b/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java
index 0e490e0..d91302a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/EnvironmentalExecException.java
@@ -14,11 +14,10 @@
package com.google.devtools.build.lib.actions;
-import com.google.common.base.Throwables;
+import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.Execution;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.lib.util.DetailedExitCode;
import java.io.IOException;
/**
@@ -35,10 +34,13 @@
* directory or denied file system access.
*/
public class EnvironmentalExecException extends ExecException {
+ private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
+
private final FailureDetail failureDetail;
public EnvironmentalExecException(IOException cause, FailureDetails.Execution.Code code) {
super("unexpected I/O exception", cause);
+ logger.atSevere().withCause(cause).log("Unexpected I/O exception");
this.failureDetail =
FailureDetail.newBuilder().setExecution(Execution.newBuilder().setCode(code)).build();
}
@@ -54,15 +56,7 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix, Action action) {
- String message =
- String.format(
- "%s failed due to %s%s",
- messagePrefix,
- getMessage(),
- getCause() == null ? "" : ("\n" + Throwables.getStackTraceAsString(getCause())));
- FailureDetail failureDetailWithPrefix = failureDetail.toBuilder().setMessage(message).build();
- return new ActionExecutionException(
- message, action, isCatastrophic(), DetailedExitCode.of(failureDetailWithPrefix));
+ protected FailureDetail getFailureDetail(String message) {
+ return failureDetail.toBuilder().setMessage(message).build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
index 27bc137..fb63b60 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ExecException.java
@@ -14,6 +14,11 @@
package com.google.devtools.build.lib.actions;
+import com.google.common.base.Strings;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.errorprone.annotations.ForOverride;
+
/**
* An exception indication that the execution of an action has failed OR could not be attempted OR
* could not be finished OR had something else wrong.
@@ -81,7 +86,7 @@
* @param action failed action
* @return ActionExecutionException object describing the action failure
*/
- public ActionExecutionException toActionExecutionException(Action action) {
+ public final ActionExecutionException toActionExecutionException(Action action) {
return toActionExecutionException("", action);
}
@@ -94,6 +99,28 @@
* @param action failed action
* @return ActionExecutionException object describing the action failure
*/
- public abstract ActionExecutionException toActionExecutionException(
- String messagePrefix, Action action);
+ public final ActionExecutionException toActionExecutionException(
+ String messagePrefix, Action action) {
+ String message =
+ String.format(
+ "%s failed: %s",
+ Strings.isNullOrEmpty(messagePrefix) ? action.describe() : messagePrefix,
+ getMessageForActionExecutionException());
+ return toActionExecutionException(
+ message, action, DetailedExitCode.of(getFailureDetail(message)));
+ }
+
+ @ForOverride
+ protected ActionExecutionException toActionExecutionException(
+ String message, Action action, DetailedExitCode code) {
+ return new ActionExecutionException(message, this, action, isCatastrophic(), code);
+ }
+
+ @ForOverride
+ protected String getMessageForActionExecutionException() {
+ return getMessage();
+ }
+
+ @ForOverride
+ protected abstract FailureDetail getFailureDetail(String message);
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java b/src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java
index 0f4eb65..0be3361 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/LostInputsExecException.java
@@ -65,18 +65,20 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix, Action action) {
- String message = String.format("%s failed: %s", messagePrefix, getMessage());
- DetailedExitCode code =
- DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setMessage(message)
- .setExecution(Execution.newBuilder().setCode(Code.ACTION_INPUT_LOST))
- .build());
+ protected ActionExecutionException toActionExecutionException(
+ String message, Action action, DetailedExitCode code) {
return new LostInputsActionExecutionException(
message, lostInputs, owners, action, /*cause=*/ this, code);
}
+ @Override
+ protected FailureDetail getFailureDetail(String message) {
+ return FailureDetail.newBuilder()
+ .setExecution(Execution.newBuilder().setCode(Code.ACTION_INPUT_LOST))
+ .setMessage(message)
+ .build();
+ }
+
public void combineAndThrow(LostInputsExecException other) throws LostInputsExecException {
// This uses a HashMap when merging the two lostInputs maps because key collisions are expected.
// In contrast, ImmutableMap.Builder treats collisions as errors. Collisions will happen when
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
index da5ea0f..149b2db 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
@@ -239,7 +239,6 @@
}
String getDetailMessage(
- String messagePrefix,
String message,
boolean catastrophe,
boolean forciblyRunRemotely);
@@ -378,13 +377,12 @@
@Override
public String getDetailMessage(
- String messagePrefix,
String message,
boolean catastrophe,
boolean forciblyRunRemotely) {
TerminationStatus status = new TerminationStatus(
exitCode(), status() == Status.TIMEOUT);
- String reason = " (" + status.toShortString() + ")"; // e.g " (Exit 1)"
+ String reason = "(" + status.toShortString() + ")"; // e.g "(Exit 1)"
String explanation = Strings.isNullOrEmpty(message) ? "" : ": " + message;
if (!status().isConsideredUserError()) {
@@ -412,7 +410,7 @@
if (!Strings.isNullOrEmpty(failureMessage)) {
explanation += " " + failureMessage;
}
- return messagePrefix + " failed" + reason + explanation;
+ return reason + explanation;
}
@Nullable
diff --git a/src/main/java/com/google/devtools/build/lib/actions/TestExecException.java b/src/main/java/com/google/devtools/build/lib/actions/TestExecException.java
index 73365d0..900579a 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/TestExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/TestExecException.java
@@ -16,7 +16,6 @@
import com.google.devtools.build.lib.server.FailureDetails;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.server.FailureDetails.TestAction;
-import com.google.devtools.build.lib.util.DetailedExitCode;
/** An TestExecException that is related to the failure of a TestAction. */
public final class TestExecException extends ExecException {
@@ -28,14 +27,10 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix, Action action) {
- String message = String.format("%s: %s", messagePrefix + " failed", getMessage());
- DetailedExitCode code =
- DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setMessage(message)
- .setTestAction(TestAction.newBuilder().setCode(detailedCode))
- .build());
- return new ActionExecutionException(message, this, action, isCatastrophic(), code);
+ protected FailureDetail getFailureDetail(String message) {
+ return FailureDetail.newBuilder()
+ .setTestAction(TestAction.newBuilder().setCode(detailedCode))
+ .setMessage(message)
+ .build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/UserExecException.java b/src/main/java/com/google/devtools/build/lib/actions/UserExecException.java
index 020aab3..cbfee2f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/UserExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/UserExecException.java
@@ -15,7 +15,6 @@
package com.google.devtools.build.lib.actions;
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
-import com.google.devtools.build.lib.util.DetailedExitCode;
/**
* An ExecException that is related to the failure of an Action and therefore very likely the user's
@@ -36,10 +35,7 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix, Action action) {
- String message = String.format("%s failed: %s", messagePrefix, getMessage());
- FailureDetail failureDetailWithPrefix = failureDetail.toBuilder().setMessage(message).build();
- return new ActionExecutionException(
- message, this, action, isCatastrophic(), DetailedExitCode.of(failureDetailWithPrefix));
+ protected FailureDetail getFailureDetail(String message) {
+ return failureDetail.toBuilder().setMessage(message).build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
index 8e255ba..fb6f4f2 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnExecException.java
@@ -14,15 +14,13 @@
package com.google.devtools.build.lib.exec;
import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkNotNull;
import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.actions.Action;
-import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
-import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
/**
* A specialization of {@link ExecException} that indicates something went wrong when trying to
@@ -38,7 +36,7 @@
checkArgument(
!Status.SUCCESS.equals(result.status()),
"Can't create exception with successful spawn result.");
- this.result = Preconditions.checkNotNull(result);
+ this.result = checkNotNull(result);
this.forciblyRunRemotely = forciblyRunRemotely;
}
@@ -46,7 +44,7 @@
public SpawnExecException(
String message, SpawnResult result, boolean forciblyRunRemotely, boolean catastrophe) {
super(message, catastrophe);
- this.result = Preconditions.checkNotNull(result);
+ this.result = checkNotNull(result);
this.forciblyRunRemotely = forciblyRunRemotely;
}
@@ -60,13 +58,12 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix, Action action) {
- if (messagePrefix == null) {
- messagePrefix = action.describe();
- }
- String message =
- result.getDetailMessage(messagePrefix, getMessage(), isCatastrophic(), forciblyRunRemotely);
- return new ActionExecutionException(
- message, this, action, isCatastrophic(), DetailedExitCode.of(result.failureDetail()));
+ protected String getMessageForActionExecutionException() {
+ return result.getDetailMessage(getMessage(), isCatastrophic(), forciblyRunRemotely);
+ }
+
+ @Override
+ protected FailureDetail getFailureDetail(String message) {
+ return checkNotNull(result.failureDetail(), this);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
index fee0304..6baaa8f 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
@@ -48,7 +48,7 @@
.build())
.setRunnerName("test")
.build();
- assertThat(r.getDetailMessage("", "", false, false))
+ assertThat(r.getDetailMessage("", false, false))
.contains("(failed due to timeout after 5.00 seconds.)");
}
@@ -64,7 +64,7 @@
.build())
.setRunnerName("test")
.build();
- assertThat(r.getDetailMessage("", "", false, false)).contains("(failed due to timeout.)");
+ assertThat(r.getDetailMessage("", false, false)).contains("(failed due to timeout.)");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
index 08815f3..f628d13 100644
--- a/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java
@@ -28,7 +28,6 @@
import com.google.common.eventbus.SubscriberExceptionHandler;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionExecutionException;
import com.google.devtools.build.lib.actions.ActionGraph;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ExecException;
@@ -95,7 +94,6 @@
import com.google.devtools.build.lib.testutil.TestUtils;
import com.google.devtools.build.lib.util.CommandBuilder;
import com.google.devtools.build.lib.util.CommandUtils;
-import com.google.devtools.build.lib.util.DetailedExitCode;
import com.google.devtools.build.lib.util.LoggingUtil;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.util.io.OutErr;
@@ -137,22 +135,11 @@
}
@Override
- public ActionExecutionException toActionExecutionException(
- String messagePrefix, Action action) {
- String message = messagePrefix + getMessage();
- // Append cause.getMessage() if it's different from getMessage(). It typically
- // isn't but if it is we'd like to surface cause.getMessage() as part of the
- // exception message.
- if (getCause() != null && !getMessage().equals(getCause().getMessage())) {
- message += ": " + getCause().getMessage();
- }
- // The detailed code doesn't matter, but it should be well-formed.
- DetailedExitCode code =
- DetailedExitCode.of(
- FailureDetail.newBuilder()
- .setSpawn(Spawn.newBuilder().setCode(Code.NON_ZERO_EXIT))
- .build());
- return new ActionExecutionException(message, getCause(), action, true, code);
+ protected FailureDetail getFailureDetail(String message) {
+ return FailureDetail.newBuilder()
+ .setSpawn(Spawn.newBuilder().setCode(Code.NON_ZERO_EXIT))
+ .setMessage(message)
+ .build();
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index 46dc051..94a1040 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -822,7 +822,7 @@
SpawnResult result = runner.exec(spawn, policy);
assertThat(result.exitCode()).isEqualTo(ExitCode.REMOTE_ERROR.getNumericExitCode());
- assertThat(result.getDetailMessage("", "", false, false)).contains("reasons");
+ assertThat(result.getDetailMessage("", false, false)).contains("reasons");
}
@Test
@@ -842,7 +842,7 @@
SpawnResult result = runner.exec(spawn, policy);
assertThat(result.exitCode()).isEqualTo(ExitCode.REMOTE_ERROR.getNumericExitCode());
- assertThat(result.getDetailMessage("", "", false, false)).contains("reasons");
+ assertThat(result.getDetailMessage("", false, false)).contains("reasons");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
index 074f254..bd11f56 100644
--- a/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/standalone/StandaloneSpawnStrategyTest.java
@@ -315,7 +315,8 @@
@Test
public void testVerboseFailures() {
ExecException e = assertThrows(ExecException.class, () -> run(createSpawn(getFalseCommand())));
- ActionExecutionException actionExecutionException = e.toActionExecutionException("", null);
+ ActionExecutionException actionExecutionException =
+ e.toActionExecutionException("messagePrefix", null);
assertWithMessage("got: " + actionExecutionException.getMessage())
.that(actionExecutionException.getMessage().contains("failed: error executing command"))
.isTrue();
