gnodet-bot commented on code in PR #12694:
URL: https://github.com/apache/maven/pull/12694#discussion_r4012507615
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/LookupInvoker.java:
##########
@@ -332,6 +332,16 @@ protected final void createTerminal(C context) {
ProjectBuildLogAppender projectBuildLogAppender =
new
ProjectBuildLogAppender(determineBuildEventListener(context));
context.closeables.add(projectBuildLogAppender);
+
+ // Now that the logSink (and any -l log-file writer) is installed,
+ // replay early log messages that were accumulated before
+ // activateLogging() ran. This ensures messages such as
+ // "Enabled to break the build on log level WARN." reach the log
+ // file rather than stdout (MavenITmng6065 regression fix).
+ if (context.pendingEarlyLogs != null) {
+ context.pendingEarlyLogs.forEach(e ->
context.logger.log(e.level(), e.message(), e.error()));
+ context.pendingEarlyLogs = null;
Review Comment:
**[high] `pendingEarlyLogs` drain is dead code — `activateLogging()` has not
run yet when this executes**
`doInvoke()` calls `createTerminal()` at line 159, then `activateLogging()`
at line 160. The drain block below executes at the end of `createTerminal()`,
but `context.pendingEarlyLogs` is set by `activateLogging()` (line 518), which
hasn't been called yet. So `context.pendingEarlyLogs` is always `null` here,
the `if` branch is never taken, and the MavenITmng6065 fix (early messages
reaching the log file) is completely non-functional.
The fix has two correct options:
**Option A** — move the drain to the start of `activateLogging()` *after*
installing `ProjectBuildLogAppender` (but that's the wrong place since the
appender is installed in `createTerminal()`).
**Option B** — move the drain call to occur *after* `activateLogging()`
returns, i.e. call it from `doInvoke()` directly:
```java
protected int doInvoke(C context) throws Exception {
...
configureLogging(context);
createTerminal(context); // installs ProjectBuildLogAppender /
logSink
activateLogging(context); // sets context.pendingEarlyLogs
// Now that both the logSink AND pendingEarlyLogs are set, drain:
if (context.pendingEarlyLogs != null) {
context.pendingEarlyLogs.forEach(e ->
context.logger.log(e.level(), e.message(), e.error()));
context.pendingEarlyLogs = null;
}
helpOrVersionAndMayExit(context);
...
}
```
The drain block currently in `createTerminal()` should be removed.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/plugin/Log.java:
##########
@@ -87,8 +87,6 @@ default void trace(Throwable error) {}
* The supplier is only evaluated if trace is enabled.
* <p>
* The default implementation is a no-op for backward compatibility.
- *
- * @param content the message supplier
*/
default void trace(Supplier<String> content) {}
Review Comment:
**[low] Missing `@param content` tag — inconsistent with all other
overloads**
The diff removes `@param content the message supplier` from
`trace(Supplier<String> content)`. Every other supplier-based method in this
interface (`debug`, `info`, `warn`, `error`) retains its `@param content` and
`@param error` tags. The removal makes the Javadoc incomplete and inconsistent.
```suggestion
/**
* Sends a lazily-computed message at the <b>trace</b> error level.
* The supplier is only evaluated if trace is enabled.
* <p>
* The default implementation is a no-op for backward compatibility
* with existing {@code Log} implementations.
*
* @param content the message supplier
*/
default void trace(Supplier<String> content) {}
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/plugin/Log.java:
##########
@@ -97,9 +95,6 @@ default void trace(Supplier<String> content) {}
* The supplier is only evaluated if trace is enabled.
* <p>
* The default implementation is a no-op for backward compatibility.
- *
- * @param content the message supplier
- * @param error the error that caused this log
*/
default void trace(Supplier<String> content, Throwable error) {}
Review Comment:
**[low] Missing `@param content` and `@param error` tags — inconsistent with
all other overloads**
Same issue: the diff removes both `@param` tags from
`trace(Supplier<String>, Throwable)`. Every other supplier+throwable method in
this interface documents both parameters.
```suggestion
/**
* Sends a lazily-computed message (and accompanying exception) at the
<b>trace</b> error level.
* The supplier is only evaluated if trace is enabled.
* <p>
* The default implementation is a no-op for backward compatibility
* with existing {@code Log} implementations.
*
* @param content the message supplier
* @param error the error that caused this log
*/
default void trace(Supplier<String> content, Throwable error) {}
```
--
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]
