Refactor message construction of ActionExecutionException
Note to Reviewer: Start your review looking at ActionExecutionException.java to see what this change is all about.
The constructor of ActionExecutionException always appends cause.getMessage() to it's own message whenever a cause is available. This behavior is unexpected by a caller and in practice often leads to clunky error messages. I found especially the pattern of new ActionExecutionException(cause.getMessage(), cause) to frequently and unintentionally lead to the duplication of error messages.
This change refactors ActionExecutionException to NOT automatically append cause.getMessage() to it's own message and also fixes relevant callsites to manually append cause.getMessage(). It does not fix the callsites that already call new ActionExecutionException(message, cause.getMessage()) where message.equals(cause.getMessage)) is true. So these callsites unfortunately won't show up as part of this change.
RELNOTES: None
PiperOrigin-RevId: 252790648
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionException.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionException.java
index a053c6e..f16d441 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionException.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionException.java
@@ -45,7 +45,7 @@
public ActionExecutionException(String message,
Throwable cause, Action action, boolean catastrophe) {
- super(message + ": " + cause.getMessage(), cause);
+ super(message, cause);
this.action = action;
this.rootCauses = rootCausesFromAction(action);
this.catastrophe = catastrophe;
@@ -55,7 +55,7 @@
public ActionExecutionException(String message,
Throwable cause, Action action, boolean catastrophe,
ExitCode exitCode) {
- super(message + ": " + cause.getMessage(), cause);
+ super(message, cause);
this.action = action;
this.rootCauses = rootCausesFromAction(action);
this.catastrophe = catastrophe;
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 e2decfd..e04d2af 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
@@ -52,7 +52,7 @@
public ActionExecutionException toActionExecutionException(
String messagePrefix, boolean verboseFailures, Action action) {
String message = messagePrefix + " failed";
- return new LostInputsActionExecutionException(message, this, action);
+ return new LostInputsActionExecutionException(message + ": " + getMessage(), this, action);
}
/** An {@link ActionExecutionException} wrapping a {@link LostInputsExecException}. */
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 6297601..db2cb5d 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
@@ -26,6 +26,7 @@
public ActionExecutionException toActionExecutionException(String messagePrefix,
boolean verboseFailures, Action action) {
String message = messagePrefix + " failed";
- return new ActionExecutionException(message, this, action, isCatastrophic());
+ return new ActionExecutionException(
+ message + ": " + getMessage(), this, action, isCatastrophic());
}
}
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 5b06d3e..f58f9e1 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
@@ -36,6 +36,7 @@
public ActionExecutionException toActionExecutionException(String messagePrefix,
boolean verboseFailures, Action action) {
String message = messagePrefix + " failed";
- return new ActionExecutionException(message, this, action, isCatastrophic());
+ return new ActionExecutionException(
+ message + ": " + getMessage(), this, action, isCatastrophic());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index a11655d..794015e 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -203,7 +203,9 @@
actionExecutionContext.getInputPath(volatileStatus), printStatusMap(volatileMap));
} catch (IOException e) {
throw new ActionExecutionException(
- "Failed to run workspace status command " + options.workspaceStatusCommand,
+ String.format(
+ "Failed to run workspace status command %s: %s",
+ options.workspaceStatusCommand, e.getMessage()),
e,
this,
true);
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 389fb21..4a3b367 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
@@ -33,11 +33,11 @@
protected final SpawnResult result;
protected final boolean forciblyRunRemotely;
- public SpawnExecException(
- String message, SpawnResult result, boolean forciblyRunRemotely) {
+ public SpawnExecException(String message, SpawnResult result, boolean forciblyRunRemotely) {
super(message, result.isCatastrophe());
- checkArgument(!Status.SUCCESS.equals(result.status()), "Can't create exception with successful"
- + " spawn result.");
+ checkArgument(
+ !Status.SUCCESS.equals(result.status()),
+ "Can't create exception with successful" + " spawn result.");
this.result = Preconditions.checkNotNull(result);
this.forciblyRunRemotely = forciblyRunRemotely;
}
@@ -60,29 +60,20 @@
}
@Override
- public ActionExecutionException toActionExecutionException(String messagePrefix,
- boolean verboseFailures, Action action) {
+ public ActionExecutionException toActionExecutionException(
+ String messagePrefix, boolean verboseFailures, Action action) {
if (messagePrefix == null) {
messagePrefix = action.describe();
}
// Note: we intentionally do not include the ExecException here, unless verboseFailures is true,
// because it creates unwieldy and useless messages. If users need more info, they can run with
// --verbose_failures.
- String message = result.getDetailMessage(
- messagePrefix, getMessage(), isCatastrophic(), forciblyRunRemotely);
+ String message =
+ result.getDetailMessage(messagePrefix, getMessage(), isCatastrophic(), forciblyRunRemotely);
if (verboseFailures) {
- return new ActionExecutionException(
- message,
- this,
- action,
- isCatastrophic(),
- getExitCode());
+ return new ActionExecutionException(message, this, action, isCatastrophic(), getExitCode());
} else {
- return new ActionExecutionException(
- message,
- action,
- isCatastrophic(),
- getExitCode());
+ return new ActionExecutionException(message, action, isCatastrophic(), getExitCode());
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 4f70a3b..5b396c4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -1403,7 +1403,8 @@
return depSet.read(actionExecutionContext.getInputPath(getDotdFile()));
} catch (IOException e) {
// Some kind of IO or parse exception--wrap & rethrow it to stop the build.
- throw new ActionExecutionException("error while parsing .d file", e, this, false);
+ throw new ActionExecutionException(
+ "error while parsing .d file: " + e.getMessage(), e, this, false);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java
index 2a69466..fd20c70 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkAction.java
@@ -70,7 +70,7 @@
symlink.createSymbolicLink(actionExecutionContext.getInputPath(entry.getValue()));
}
} catch (IOException e) {
- String message = "IO Error while creating symlink";
+ String message = "IO Error while creating symlink: " + e.getMessage();
throw new ActionExecutionException(message, e, this, false);
}
return ActionResult.EMPTY;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 487ad6f..157cd5f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -160,7 +160,9 @@
artifact = artifactResolver.resolveSourceArtifact(execPathFragment, repository);
} catch (LabelSyntaxException e) {
throw new ActionExecutionException(
- String.format("Could not find the external repository for %s", execPathFragment),
+ String.format(
+ "Could not find the external repository for %s: " + e.getMessage(),
+ execPathFragment),
e,
action,
false);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java
index 3fe9cea..05b9146 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LtoBackendAction.java
@@ -144,7 +144,10 @@
}
} catch (IOException e) {
throw new ActionExecutionException(
- "error iterating imports file " + actionExecutionContext.getInputPath(imports),
+ "error iterating imports file "
+ + actionExecutionContext.getInputPath(imports)
+ + ": "
+ + e.getMessage(),
e,
this,
false);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java
index 5b74079..eb617f2 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/SolibSymlinkAction.java
@@ -71,7 +71,8 @@
+ symlink.prettyPrint()
+ "' to target '"
+ getPrimaryInput()
- + "'",
+ + "': "
+ + e.getMessage(),
e,
this,
false);
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
index 2d5c9ba..45a54df 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionFunction.java
@@ -647,7 +647,10 @@
skyframeActionExecutor, env, metadataHandler, ImmutableMap.of());
} catch (IOException e) {
throw new ActionExecutionException(
- "Failed to update filesystem context: ", e, action, /*catastrophe=*/ false);
+ "Failed to update filesystem context: " + e.getMessage(),
+ e,
+ action,
+ /*catastrophe=*/ false);
}
state.discoveredInputs =
skyframeActionExecutor.discoverInputs(
@@ -721,7 +724,10 @@
state.updateFileSystemContext(skyframeActionExecutor, env, metadataHandler, expandedFilesets);
} catch (IOException e) {
throw new ActionExecutionException(
- "Failed to update filesystem context: ", e, action, /*catastrophe=*/ false);
+ "Failed to update filesystem context: " + e.getMessage(),
+ e,
+ action,
+ /*catastrophe=*/ false);
}
try (ActionExecutionContext actionExecutionContext =
@@ -747,7 +753,7 @@
state.discoveredInputs != null);
} catch (IOException e) {
throw new ActionExecutionException(
- "Failed to close action output", e, action, /*catastrophe=*/ false);
+ "Failed to close action output: " + e.getMessage(), e, action, /*catastrophe=*/ false);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 8f4f07e..7d0ba1c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -1207,7 +1207,10 @@
outputDir.createDirectoryAndParents();
} catch (IOException e) {
throw new ActionExecutionException(
- "failed to create output directory '" + outputDir + "'", e, action, false);
+ "failed to create output directory '" + outputDir + "': " + e.getMessage(),
+ e,
+ action,
+ false);
}
}
}
@@ -1230,8 +1233,13 @@
fileOutErr.getErrorPath().getParentDirectory().createDirectoryAndParents();
} catch (IOException e) {
throw new ActionExecutionException(
- "failed to create output directory for output streams'" + fileOutErr.getErrorPath() + "'",
- e, action, false);
+ "failed to create output directory for output streams'"
+ + fileOutErr.getErrorPath()
+ + "': "
+ + e.getMessage(),
+ e,
+ action,
+ false);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
index be58dc1..76fa9ad 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
@@ -117,8 +117,8 @@
} catch (RuntimeException | Error e) {
throw e;
} catch (Exception e) {
- throw new ActionExecutionException("TestAction failed due to exception",
- e, this, false);
+ throw new ActionExecutionException(
+ "TestAction failed due to exception: " + e.getMessage(), e, this, false);
}
try {
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 1286a6d..332cbf1 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
@@ -131,7 +131,14 @@
@Override
public ActionExecutionException toActionExecutionException(
String messagePrefix, boolean verboseFailures, Action action) {
- return new ActionExecutionException(messagePrefix + getMessage(), getCause(), action, true);
+ 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();
+ }
+ return new ActionExecutionException(message, getCause(), action, true);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
index 8df59da..ffe5aaf 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -1093,8 +1093,8 @@
} catch (RuntimeException e) {
throw new RuntimeException(e);
} catch (Exception e) {
- throw new ActionExecutionException("TestAction failed due to exception",
- e, this, false);
+ throw new ActionExecutionException(
+ "TestAction failed due to exception: " + e.getMessage(), e, this, false);
}
return ActionResult.EMPTY;
}
