gnodet-bot commented on code in PR #13832:
URL: https://github.com/apache/maven/pull/13832#discussion_r4477548998
##########
impl/maven-core/src/main/java/org/apache/maven/logging/BuildEventListener.java:
##########
@@ -30,7 +31,7 @@ public interface BuildEventListener {
void projectStarted(String projectId);
- void projectLogMessage(String projectId, String event);
+ void projectLogMessage(String projectId, LogEvent event);
Review Comment:
⚠️ **Undocumented binary-incompatible SPI change.** `BuildEventListener` is
a public interface in `maven-core` and embedded-Maven consumers implement it.
Changing `projectLogMessage(String, String)` to `projectLogMessage(String,
LogEvent)` produces a `NoSuchMethodError` at link time for any existing
embedder without recompilation.
If this SPI is intentionally being broken in 4.2 (new major API), document
it explicitly in the method Javadoc. If a compatibility window is needed, keep
the old signature as a `@Deprecated default` method that delegates to the new
one:
```suggestion
/**
* @deprecated Use {@link #projectLogMessage(String, LogEvent)} instead.
*/
@Deprecated(since = "4.2.0", forRemoval = true)
default void projectLogMessage(String projectId, String event) {}
void projectLogMessage(String projectId, LogEvent event);
```
##########
impl/maven-core/src/main/java/org/apache/maven/logging/ProjectBuildLogAppender.java:
##########
@@ -109,29 +114,102 @@ public static void setForkingMojoId(String
forkingMojoId) {
}
}
- public static void updateMdc() {
- String id = getProjectId();
- if (id != null) {
- MDC.put(KEY_PROJECT_ID, id);
- } else {
- MDC.remove(KEY_PROJECT_ID);
- }
+ private static final AtomicLong SEQUENCE = new AtomicLong();
+
+ private static volatile Consumer<LogEvent> reportCapture;
+
+ public static void setReportCapture(Consumer<LogEvent> capture) {
+ ProjectBuildLogAppender.reportCapture = capture;
}
private final BuildEventListener buildEventListener;
+ private final ConcurrentLinkedQueue<LogEvent> queue = new
ConcurrentLinkedQueue<>();
+ private final ReentrantLock drainLock = new ReentrantLock();
+ private volatile boolean needsMetadata;
public ProjectBuildLogAppender(BuildEventListener buildEventListener) {
this.buildEventListener = buildEventListener;
- MavenSimpleLogger.setLogSink(this::accept);
+ LoggerManager.get().setLogSink(new Sink());
+ }
+
+ public void setNeedsMetadata(boolean needsMetadata) {
+ this.needsMetadata = needsMetadata;
+ }
+
+ private class Sink implements LogSink {
+ @Override
+ public void accept(LogEvent event) {
+ final LogEvent full;
+ String projectId = event.projectId() != null ? event.projectId() :
getProjectId();
+ String mojoId = event.mojoId() != null ? event.mojoId() :
getMojoId();
+ if (event instanceof DefaultLogEvent && projectId ==
event.projectId() && mojoId == event.mojoId()) {
Review Comment:
⚠️ **String identity comparison (`==`) on `projectId`/`mojoId`** — this is
fragile. The check on line 143 reads `event.projectId()` and assigns it to
`projectId`/`mojoId` local variables only when `event.projectId()` is null;
otherwise the locals come from `getProjectId()`/`getMojoId()`. When the event
already carries non-null ids, `projectId` is the local copy of `getProjectId()`
— a potentially different `String` instance than `event.projectId()` even when
`String.equals()` would return true (e.g. ids constructed via concatenation,
intern mismatches). The optimisation silently falls through to the re-wrapping
branch when it shouldn't, adding unnecessary allocation.
Use `Objects.equals()`:
```suggestion
if (event instanceof DefaultLogEvent &&
Objects.equals(projectId, event.projectId()) && Objects.equals(mojoId,
event.mojoId())) {
```
##########
api/maven-api-log/src/main/java/org/apache/maven/api/logging/LogSink.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.api.logging;
+
+import org.apache.maven.api.annotations.Consumer;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Central dispatch point for log events.
+ * <p>
+ * All logging paths — the fluent {@link Logger} API, SLF4J
+ * ({@code MavenSimpleLogger}), and JUL ({@code MavenJulHandler}) — converge
+ * here. A single {@code LogSink} implementation controls level filtering and
+ * routing for all sources.
+ * <p>
+ * Implementations are installed at Maven startup and remain in place for the
+ * lifetime of the build. The sink is not DI-managed; it is set via a static
+ * setter in the logging infrastructure.
+ *
+ * @since 4.2.0
+ */
+@Experimental
+@Consumer
+public interface LogSink {
Review Comment:
💡 `LogSink` is used as a lambda target in `LookupInvoker`
(`currentSink.drainTo(event -> {...})`), but is not annotated
`@FunctionalInterface`. Without it, adding a second abstract method in a future
commit silently breaks all lambda usages without a compile-time warning.
```suggestion
@Experimental
@Consumer
@FunctionalInterface
public interface LogSink {
```
(The two `default` methods `needsMetadata()` and `drainTo()` don't prevent
this — only abstract methods count.)
##########
impl/maven-core/src/main/java/org/apache/maven/logging/SimpleBuildEventListener.java:
##########
@@ -38,8 +42,22 @@ public void sessionStarted(ExecutionEvent event) {}
public void projectStarted(String projectId) {}
@Override
- public void projectLogMessage(String projectId, String event) {
- log(event);
+ public void projectLogMessage(String projectId, LogEvent event) {
+ log("[" + renderLevel(event.level()) + "] " + event.message());
+ Throwable throwable = event.throwable();
+ if (throwable != null) {
+ StringWriter sw = new StringWriter();
+ throwable.printStackTrace(new PrintWriter(sw));
+ log(sw.toString().stripTrailing());
Review Comment:
💡 `new PrintWriter(sw)` is not closed. `StringWriter.close()` is a
documented no-op, so there is no actual resource leak, but the pattern fails
static analysis and sets a poor example in a public SPI that subclasses can
override. Use try-with-resources:
```suggestion
StringWriter sw = new StringWriter();
try (PrintWriter pw = new PrintWriter(sw)) {
throwable.printStackTrace(pw);
}
log(sw.toString().stripTrailing());
```
##########
impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLogger.java:
##########
@@ -0,0 +1,419 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.logging.impl;
+
+import java.lang.StackWalker.StackFrame;
+import java.time.Instant;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Supplier;
+
+import org.apache.maven.api.MonotonicClock;
+import org.apache.maven.api.logging.LogEvent;
+import org.apache.maven.api.logging.LogEventBuilder;
+import org.apache.maven.api.logging.LogLevel;
+import org.apache.maven.api.logging.LogSink;
+import org.apache.maven.api.logging.Logger;
+import org.apache.maven.api.logging.LoggerManager;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of both {@link Logger} (new fluent API)
+ * and the deprecated {@link org.apache.maven.api.plugin.Log} (backward
compatibility).
+ * <p>
+ * Builds fully-structured {@link LogEvent} objects — including {@code
projectId},
+ * {@code mojoId}, timestamp, and sequence number — and dispatches them to the
+ * current {@link LogSink} obtained from {@link LoggerManager#get()}.
+ * Level checks and sink lookup both go through the {@code LoggerManager},
+ * so the sink can be changed at runtime.
+ *
+ * @since 4.2.0
+ */
+@SuppressWarnings("deprecation")
+public class DefaultLogger implements Logger, org.apache.maven.api.plugin.Log {
+
+ private static final StackWalker WALKER = StackWalker.getInstance();
+ private static final String THIS_CLASS = DefaultLogger.class.getName();
+ private static final String BUILDER_CLASS =
DefaultLogEventBuilder.class.getName();
+
+ private static final AtomicLong SEQUENCE = new AtomicLong();
+
+ private final String name;
+ private final String projectId;
+ private final String mojoId;
+
+ public DefaultLogger(String name) {
+ this(name, null, null);
+ }
+
+ public DefaultLogger(String name, String projectId, String mojoId) {
+ this.name = requireNonNull(name, "name");
+ this.projectId = projectId;
+ this.mojoId = mojoId;
+ }
+
+ // ---- org.apache.maven.api.logging.Logger ----
+
+ @Override
+ public String name() {
+ return name;
+ }
+
+ private boolean isLevelEnabled(LogLevel level) {
+ return LoggerManager.get().isLevelEnabled(name, level);
+ }
+
+ @Override
+ public boolean isEnabled(LogLevel level) {
+ return isLevelEnabled(level);
+ }
+
+ @Override
+ public LogEventBuilder atTrace() {
+ return isLevelEnabled(LogLevel.TRACE)
+ ? new DefaultLogEventBuilder(this, LogLevel.TRACE)
+ : NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public LogEventBuilder atDebug() {
+ return isLevelEnabled(LogLevel.DEBUG)
+ ? new DefaultLogEventBuilder(this, LogLevel.DEBUG)
+ : NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public LogEventBuilder atInfo() {
+ return isLevelEnabled(LogLevel.INFO)
+ ? new DefaultLogEventBuilder(this, LogLevel.INFO)
+ : NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public LogEventBuilder atWarn() {
+ return isLevelEnabled(LogLevel.WARN)
+ ? new DefaultLogEventBuilder(this, LogLevel.WARN)
+ : NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public LogEventBuilder atError() {
+ return isLevelEnabled(LogLevel.ERROR)
+ ? new DefaultLogEventBuilder(this, LogLevel.ERROR)
+ : NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public LogEventBuilder at(LogLevel level) {
+ return isLevelEnabled(level) ? new DefaultLogEventBuilder(this, level)
: NopLogEventBuilder.INSTANCE;
+ }
+
+ @Override
+ public DefaultLogger child(String childName) {
+ requireNonNull(childName, "childName");
+ return new DefaultLogger(name + "." + childName, projectId, mojoId);
+ }
+
+ // ---- Internal dispatch ----
+
+ void doLog(LogLevel level, String message, Throwable throwable) {
+ doLog(level, message, null, null, null, throwable, null, null);
+ }
+
+ @SuppressWarnings("deprecation") // Thread.getId()
+ void doLog(
+ LogLevel level,
+ String message,
+ String messageTemplate,
+ Object[] messageArgs,
+ Map<String, Object> keyValues,
+ Throwable throwable) {
+ doLog(level, message, messageTemplate, messageArgs, keyValues,
throwable, null, null);
+ }
+
+ @SuppressWarnings({"deprecation", "checkstyle:ParameterNumber"}) //
Thread.getId()
+ void doLog(
+ LogLevel level,
+ String message,
+ String messageTemplate,
+ Object[] messageArgs,
+ Map<String, Object> keyValues,
+ Throwable throwable,
+ String projectIdOverride,
+ String mojoIdOverride) {
+ LoggerManager mgr = LoggerManager.get();
+ LogSink sink = mgr.getLogSink();
+ if (sink == null) {
+ return;
+ }
+
+ Instant timestamp = MonotonicClock.now();
+ long seq = SEQUENCE.getAndIncrement();
+
+ String sourceClass = null;
+ String sourceMethod = null;
+ long threadId = -1;
+ String threadName = null;
+
+ if (sink.needsMetadata()) {
+ sourceClass = name;
+ sourceMethod = resolveCallerMethod();
+ threadId = Thread.currentThread().getId();
Review Comment:
💡 `Thread.currentThread().getId()` is deprecated since JDK 19 in favour of
`Thread.currentThread().threadId()`. The `@SuppressWarnings("deprecation")`
suppresses the warning but leaves no breadcrumb for future maintainers. Add a
TODO when the baseline moves:
```suggestion
threadId = Thread.currentThread().getId(); // TODO: replace with
threadId() when baseline >= JDK 19
```
--
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]
