gnodet-bot commented on code in PR #13832: URL: https://github.com/apache/maven/pull/13832#discussion_r4500496977
########## its/core-it-suite/src/test/resources/gh-13522-logger-context/src/main/java/org/apache/maven/its/gh13522/CheckContextMojo.java: ########## @@ -0,0 +1,105 @@ +/* + * 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.its.gh13522; + +import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Properties; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.maven.api.di.Inject; +import org.apache.maven.api.logging.LogEvent; +import org.apache.maven.api.logging.Logger; +import org.apache.maven.api.logging.LoggerManager; +import org.apache.maven.api.plugin.annotations.Mojo; +import org.apache.maven.api.plugin.annotations.Parameter; + +/** + * Verifies that a DI-injected {@link Logger} carries the correct + * {@code projectId}, {@code mojoId}, and {@code pluginId} on each + * {@link LogEvent} it emits. + * + * <p>The mojo wraps the global {@link LoggerManager} sink, captures + * the first event it emits, extracts the context fields, and writes + * them to {@code target/log-context.properties} so the IT verifier + * can assert their values.</p> + */ +@Mojo(name = "check-context", defaultPhase = org.apache.maven.api.Lifecycle.Phase.VALIDATE, projectRequired = false) Review Comment: ⚠️ `projectRequired = false` is inconsistent with the mojo's behavior — `${project.build.directory}` is a project-scoped expression, and `Paths.get(buildDirectory)` has no null guard. If this mojo were invoked without a project, `buildDirectory` would be `null` and throw `NullPointerException` at the `Paths.get()` call. The IT always provides a project so the test passes, but the annotation should be `projectRequired = true`. ```suggestion @Mojo(name = "check-context", defaultPhase = org.apache.maven.api.Lifecycle.Phase.VALIDATE, projectRequired = true) ``` ########## impl/maven-core/src/main/java/org/apache/maven/logging/ProjectBuildLogAppender.java: ########## @@ -109,29 +115,105 @@ 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 + && Objects.equals(projectId, event.projectId()) + && Objects.equals(mojoId, event.mojoId())) { + full = event; + } else { + full = new DefaultLogEvent( + event.timestamp() != null ? event.timestamp() : MonotonicClock.now(), + event.level(), + event.message(), + event.loggerName(), + event.throwable(), + event.sourceClassName(), + event.sourceMethodName(), + event.threadId(), + SEQUENCE.getAndIncrement(), + event.threadName(), + projectId, + mojoId, + event.pluginId(), + event.keyValues(), + event.messageTemplate(), + event.messageArgs()); + } + Review Comment: ⚠️ **`renderedLine` is dropped in the enrichment copy.** The 16-argument `DefaultLogEvent` constructor always sets `renderedLine = null`. When an event is enriched here (its `projectId`/`mojoId` didn't match the thread-local context), the copied event loses the pre-rendered console line. `SimpleBuildEventListener.projectLogMessage()` then falls back to `event.message()` (the semantic text), stripping ANSI colors for every enriched event. Use the 17-argument constructor and preserve the original `renderedLine`: ```suggestion full = new DefaultLogEvent( event.timestamp() != null ? event.timestamp() : MonotonicClock.now(), event.level(), event.message(), event.loggerName(), event.throwable(), event.sourceClassName(), event.sourceMethodName(), event.threadId(), SEQUENCE.getAndIncrement(), event.threadName(), projectId, mojoId, event.pluginId(), event.keyValues(), event.messageTemplate(), event.messageArgs(), event.renderedLine()); ``` ########## api/maven-api-log/src/main/java/org/apache/maven/api/logging/LoggerManager.java: ########## @@ -0,0 +1,243 @@ +/* + * 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 java.util.ServiceConfigurationError; +import java.util.ServiceLoader; + +import org.apache.maven.api.annotations.Experimental; +import org.apache.maven.api.annotations.Nonnull; +import org.apache.maven.api.annotations.Nullable; +import org.apache.maven.api.annotations.Provider; + +/** + * Central entry point for Maven's logging system. + * <p> + * Provides access to loggers, the active {@link LogSink}, level configuration, + * and severity tracking for {@code --fail-on-severity}. + * <p> + * The singleton is resolved lazily on the first {@link #get()} call using + * {@link ServiceLoader}. If no provider is found, a buffering fallback is + * installed that accumulates events until a real implementation is set via + * {@link #set(LoggerManager)}. + * + * @since 4.2.0 + */ +@Experimental +@Provider +public interface LoggerManager { + + /** + * Returns a {@link Logger} instance for the given name. + * + * @since 4.2.0 + */ + @Nonnull + Logger getLogger(@Nonnull String name); + + /** + * Returns a {@link Logger} instance for the given class. + * + * @since 4.2.0 + */ + @Nonnull + default Logger getLogger(@Nonnull Class<?> clazz) { + return getLogger(clazz.getName()); + } + + /** + * Returns the active log sink, or {@code null} if none is installed. + * + * @since 4.2.0 + */ + @Nullable + LogSink getLogSink(); + + /** + * Installs or removes the active log sink. + * + * @since 4.2.0 + */ + void setLogSink(@Nullable LogSink sink); + + /** + * Returns whether logging at {@code level} is enabled for the named logger. + * + * @since 4.2.0 + */ + boolean isLevelEnabled(@Nonnull String loggerName, @Nonnull LogLevel level); + + /** + * Sets the root logger level (typically from {@code -X}, {@code -q} CLI flags). + * The new level takes effect immediately. + * + * @since 4.2.0 + */ + void setRootLoggerLevel(@Nonnull LogLevel level); + + /** + * Reloads level configuration and resets severity tracking state. + * + * @since 4.2.0 + */ + void reconfigure(); + + /** + * Returns {@code true} if any log event at or above {@code level} + * has been emitted since the last {@link #reconfigure()}. + * <p> + * Used by {@code --fail-on-severity}: the CLI stores the threshold + * and checks at build end. + * + * @since 4.2.0 + */ + boolean hasReached(@Nonnull LogLevel level); + + /** + * Registers a classloader as belonging to a specific plugin. + * Used by the plugin manager to enable attribution of log events + * to the plugin that produced them. + * + * @param classLoader the plugin's classloader + * @param pluginId the plugin identifier (e.g. {@code "groupId:artifactId:version"}) + * @since 4.2.0 + */ + default void registerPluginClassLoader(@Nonnull ClassLoader classLoader, @Nonnull String pluginId) {} + + /** + * Returns the plugin identifier for the given classloader, or {@code null} + * if the classloader is not a registered plugin classloader. + * + * @param classLoader the classloader to look up + * @return the plugin identifier, or {@code null} + * @since 4.2.0 + */ + @Nullable + default String getPluginId(@Nullable ClassLoader classLoader) { + return null; + } + + /** + * Returns the global {@code LoggerManager} instance. + * <p> + * On the first call, synchronously discovers an implementation via + * {@link ServiceLoader}. If none is found, a buffering fallback is + * installed that accumulates events until a real implementation is + * set via {@link #set(LoggerManager)}. + * + * @return the global manager, never {@code null} + * @since 4.2.0 + */ + @Nonnull + static LoggerManager get() { + LoggerManager mgr = Holder.instance; + if (mgr != null) { + return mgr; + } + return Holder.discover(); + } + + /** + * Installs the global {@code LoggerManager} instance. + * <p> + * Typically used by embedders or tests to override the + * {@link ServiceLoader}-discovered implementation. + * <p> + * If the previous manager was the bootstrap buffering manager, + * any accumulated events are drained to the new manager's + * {@link LogSink}. + * + * @param manager the manager to install, or {@code null} to clear + * @since 4.2.0 + */ + static void set(LoggerManager manager) { + LoggerManager old = Holder.instance; + Holder.instance = manager; + if (old instanceof BufferingLoggerManager buffering && manager != null && old != manager) { + LogSink sink = manager.getLogSink(); + if (sink != null) { + buffering.drainTo(sink); + } else { + // The new manager has no sink yet — bootstrap events cannot be forwarded. + // Emit a warning to stderr so the loss is not silent. + System.err.println("[WARNING] LoggerManager installed without an active LogSink;" + + " bootstrap log events accumulated before LogSink installation are discarded."); + } + } + } + + /** + * @hidden + */ + final class Holder { + private Holder() {} + + static volatile LoggerManager instance; + + private static final ThreadLocal<Boolean> DISCOVERING = new ThreadLocal<>(); + + /** + * Bootstrap buffering manager — shared across all callers that arrive before + * the real {@link ServiceLoader} provider is installed (re-entrant calls during + * loading, and the no-provider fallback). Events buffered here are drained into + * the real sink by {@link LoggerManager#set(LoggerManager)}. + */ + static final BufferingLoggerManager BOOTSTRAP = new BufferingLoggerManager(); + + static synchronized LoggerManager discover() { + LoggerManager mgr = instance; + if (mgr != null) { + return mgr; + } + + // Re-entrancy guard: a log call emitted while ServiceLoader is loading a + // provider would recurse back into discover(). Return the shared bootstrap + // buffer so those events are captured rather than silently dropped. + // Do NOT write to `instance` here — the outer discover() call will install + // the real implementation once ServiceLoader finishes. + if (DISCOVERING.get() == Boolean.TRUE) { + return BOOTSTRAP; + } + + DISCOVERING.set(Boolean.TRUE); + try { + for (LoggerManager discovered : + ServiceLoader.load(LoggerManager.class, LoggerManager.class.getClassLoader())) { + instance = discovered; + // Drain any events buffered during re-entrant calls while ServiceLoader + // was initialising the provider. mirror the same logic as set(). + LogSink sink = discovered.getLogSink(); + if (sink != null) { + BOOTSTRAP.drainTo(sink); + } Review Comment: 💡 **`discover()` drain has no warning when `getLogSink()` returns `null`.** `set()` (above) now emits a `[WARNING]` to stderr when the new manager has no sink at drain time. This newly-added drain in `discover()` silently skips the drain with no warning — if a `ServiceLoader`-discovered provider hasn't installed its sink yet, bootstrap events are discarded silently. Add the same warning for consistency: ```suggestion LogSink sink = discovered.getLogSink(); if (sink != null) { BOOTSTRAP.drainTo(sink); } else { System.err.println("[WARNING] Discovered LoggerManager has no active LogSink;" + " bootstrap log events accumulated before LogSink installation are discarded."); } ``` -- 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]
