gnodet-bot commented on code in PR #13695:
URL: https://github.com/apache/maven/pull/13695#discussion_r4368561632
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -190,31 +201,19 @@ protected int doInvoke(C context) throws Exception {
}
protected InvokerException.ExitException handleException(C context,
Exception e) {
- printErrors(
- context,
- context.options().showErrors().orElse(false),
- List.of(new Logger.Entry(Logger.Level.ERROR, e.getMessage(),
e.getCause())),
- context.logger);
+ printError(context, context.options().showErrors().orElse(false),
e.getMessage(), e.getCause());
return new InvokerException.ExitException(2);
}
- protected void printErrors(C context, boolean showStackTrace,
List<Logger.Entry> entries, Logger logger) {
- // if accumulating logger passed, this is "early failure", swap logger
for stdErr and use that to emit log
- if (logger instanceof AccumulatingLogger) {
- logger = new
SystemLogger(context.invokerRequest.stdErr().orElse(null));
- }
- // this is important message; many Maven IT assert for presence of
this message
- logger.error("Error executing " +
context.invokerRequest.parserRequest().commandName() + ".");
- for (Logger.Entry entry : entries) {
- if (showStackTrace) {
- logger.log(entry.level(), entry.message(), entry.error());
- } else {
- logger.error(entry.message());
- for (Throwable cause = entry.error();
- cause != null && cause != cause.getCause();
- cause = cause.getCause()) {
- logger.log(entry.level(), "Caused by: " +
cause.getMessage());
- }
+ protected void printError(C context, boolean showStackTrace, String
message, Throwable error) {
+ context.logger.error(
+ "Error executing " +
context.invokerRequest.parserRequest().commandName() + ".");
+ if (showStackTrace && error != null) {
+ context.logger.error(message != null ? message : "", error);
+ } else {
+ context.logger.error(message != null ? message : "");
+ for (Throwable cause = error; cause != null && cause !=
cause.getCause(); cause = cause.getCause()) {
+ context.logger.error("Caused by: " + (cause.getMessage() !=
null ? cause.getMessage() : ""));
}
}
}
Review Comment:
⚠️ **Silent-failure gap**: before `activateLogging()` runs,
`DefaultLoggerManager`'s sink is `AccumulatingLogSink` — events are buffered
and never flushed if an exception prevents `activateLogging()` from executing.
Six methods in `doInvoke()` precede `activateLogging()` (`validate`,
`pushCoreProperties`, `pushUserProperties`, `setupGuiceClassLoading`,
`configureLogging`, `createTerminal`), so any exception thrown there silently
discards the error message. For example, `configureLogging()` throws
`IllegalArgumentException` on an invalid `--color` value.
The rational for routing through `context.logger` (so `-l log.txt` captures
errors) only applies AFTER `activateLogging()` installs the real sink. For
pre-activation errors, `-l` is not configured yet — there is nothing to route
to. The stderr path is correct in both cases.
Simplest fix: check whether the current sink is still accumulating before
routing. Since `AccumulatingLogSink` is private to `DefaultLoggerManager`, use
`LoggerManager.get().getLogSink()` returning `null` doesn't work (it returns
the accumulating instance). The most practical fix without API changes: add
`LoggerManager.isReady()` returning `true` when a real sink is installed, or
restore the direct-to-stderr path:
```suggestion
protected void printError(C context, boolean showStackTrace, String
message, Throwable error) {
// Write to stderr directly — this is the fatal error exit path and
must
// be visible even if the logging infrastructure is not fully set up.
// Routing through context.logger is only safe AFTER
activateLogging() installs the
// real sink; before that, events go into AccumulatingLogSink and
are lost when
// the exception short-circuits activateLogging().
PrintWriter err = new
PrintWriter(context.invokerRequest.stdErr().orElse(System.err), true);
err.println("[ERROR] Error executing "
+ context.invokerRequest.parserRequest().commandName() +
".");
if (showStackTrace && error != null) {
err.println("[ERROR] " + (message != null ? message : ""));
error.printStackTrace(err);
} else {
err.println("[ERROR] " + (message != null ? message : ""));
for (Throwable cause = error; cause != null && cause !=
cause.getCause(); cause = cause.getCause()) {
err.println("[ERROR] Caused by: " + (cause.getMessage() !=
null ? cause.getMessage() : ""));
}
}
}
```
If the goal is to also capture these errors in `-l log.txt` when logging IS
active, route through `context.logger` in addition to stderr (not instead).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
