gnodet-bot commented on code in PR #13180:
URL: https://github.com/apache/maven/pull/13180#discussion_r4060528420
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -262,24 +253,19 @@ private void logReactorSummaryGroup(ReactorSummaryRequest
request, int group) {
buffer.append(' ');
}
- buffer.append(entry.statusMessage());
- if (entry.buildSummary() != null) {
- formatBuildTime(buffer, entry.buildSummary());
+ buffer.append(statusMessage);
+ if (buildSummary != null) {
+ formatBuildTime(buffer, buildSummary);
}
- if (entry.buildSummary() instanceof BuildFailure) {
- logger.error(buffer.toString());
- } else {
- logger.info(buffer.toString());
- }
+ logger.info(buffer.toString());
Review Comment:
⚠️ **Behavioral regression: per-module FAILURE lines downgraded from ERROR
to INFO level**
The old code used `logger.error()` for `BuildFailure` module summary lines,
producing `[ERROR] Maven Project artifact2 .... FAILURE [2.0s]`. This PR
changes it to `logger.info()` for all reactor summary lines, producing `[INFO]
Maven Project artifact2 .... FAILURE [2.0s]`.
The ANSI `failure()` coloring still makes the text red on terminals, but the
log **level** and the **visual color** serve different consumers:
- CI scripts using `grep -i error` on log files to detect failures will
silently miss these lines
- Maven wrappers, IDE build parsers, and test framework reporters that
classify severity by level will not flag them
- Tools like `mvnlog` that filter by level for post-mortem analysis will
miss module-level failures
This was raised in the previous review on the predecessor PR. The tests have
been updated to expect `INFO`, indicating this is intentional — but it's a
breaking change in observable log output that will affect existing CI pipelines.
If this is intentional, please document the behavior change in the PR
description and consider whether `logback`/`log4j` appenders configured to send
`ERROR` events to separate targets should be taken into account.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/RichBuildEventListener.java:
##########
@@ -0,0 +1,785 @@
+/*
+ * 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.cling.event;
+
+import java.io.PrintWriter;
+import java.time.Duration;
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.maven.api.MonotonicClock;
+import org.apache.maven.api.build.report.LogEvent;
+import org.apache.maven.api.build.report.LogLevel;
+import org.apache.maven.execution.ExecutionEvent;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.logging.BuildEventListener;
+import org.apache.maven.project.MavenProject;
+import org.eclipse.aether.transfer.TransferEvent;
+import org.jline.terminal.Terminal;
+import org.jline.utils.Display;
+
+/**
+ * A rich terminal build event listener using JLine's {@link Display} in
+ * non-fullscreen mode — the same approach as mvnd.
+ * <p>
+ * The status area is rendered at the current cursor position using
+ * {@link Display#updateAnsi}. When log output arrives, the display is
+ * cleared (updated with empty lines), the log line is printed normally,
+ * and then the status is redrawn below it. JLine handles all the cursor
+ * math (moving up, erasing changed lines, etc.) and only repaints what
+ * actually changed.
+ * <p>
+ * At the end of the build, the display is cleared and nothing remains
+ * on screen — the summary then prints as normal scrolling text.
+ * <p>
+ * The status area has a fixed height based on the degree of concurrency,
+ * so the separator and summary line stay anchored at the bottom. Active
+ * projects are packed to the top of the slot area; empty lines fill the
+ * gap between the last active project and the separator.
+ * <p>
+ * Falls back to simple log passthrough on dumb terminals.
+ *
+ * @since 4.1.0
+ * @see PlainExecutionEventLogger
+ * @see ExecutionEventLogger
+ */
+public class RichBuildEventListener implements BuildEventListener {
+
+ // ---- ANSI colors ----
+
+ private static final String ESC = "\033[";
+ private static final String CYAN = ESC + "36m";
+ private static final String YELLOW = ESC + "33m";
+ private static final String BLUE = ESC + "34m";
+ private static final String GREEN = ESC + "32m";
+ private static final String RED = ESC + "31m";
+ private static final String BOLD = ESC + "1m";
+ private static final String DIM = ESC + "2m";
+ private static final String RESET = ESC + "0m";
+
+ // ---- Terminal & output ----
+
+ private final Terminal terminal;
+ private final PrintWriter writer;
+ private final boolean supported;
+
+ // ---- JLine Display ----
+
+ /** JLine display in non-fullscreen mode — handles cursor math. */
+ private volatile Display display;
+ /** Whether the display is currently active. */
+ private volatile boolean displayActive;
+ /** Fixed number of lines in the status area (set once in initReactor). */
+ private volatile int statusHeight;
+
+ // ---- Reactor state ----
+
+ private volatile int totalProjects;
+ private volatile int completedProjects;
+ private volatile Instant buildStartTime;
+ /** One-line header shown at the top of the status area. */
+ private volatile String headerLine;
+
+ // ---- Project display ----
+
+ private final Map<String, ProjectState> activeProjects = new
ConcurrentHashMap<>();
+ private final List<String> projectOrder = new ArrayList<>();
+ private final Map<String, String> projectNames = new ConcurrentHashMap<>();
+
+ // ---- Active downloads ----
+
+ private final Map<String, TransferInfo> activeTransfers = new
ConcurrentHashMap<>();
+
+ // ---- Synchronization ----
+
+ /** Guards all terminal output and slot mutations. */
+ private final Object outputLock = new Object();
+
+ // ---- Periodic refresh ----
+
+ /** Scheduler for 1-second display refresh so elapsed timers stay live. */
+ private volatile ScheduledExecutorService refreshScheduler;
+ /** Handle for the periodic refresh task. */
+ private volatile ScheduledFuture<?> refreshFuture;
+
+ // ---- Warning tracking ----
+
+ /** Number of WARN-level messages seen during the build. */
+ private final AtomicInteger warningCount = new AtomicInteger();
+
+ /** Number of ERROR-level messages seen during the build. */
+ private final AtomicInteger errorCount = new AtomicInteger();
+
+ // ---- Constructor ----
+
+ /**
+ * Creates a new RichBuildEventListener.
+ *
+ * @param terminal the JLine terminal for output
+ * @param output fallback output consumer (unused — kept for API compat)
+ */
+ public RichBuildEventListener(Terminal terminal,
java.util.function.Consumer<String> output) {
+ this.terminal = terminal;
+ this.writer = terminal.writer();
+ // Support ANSI if terminal type is not "dumb" and has reasonable size
+ String type = terminal.getType();
+ this.supported = type != null && !Terminal.TYPE_DUMB.equals(type) &&
terminal.getWidth() > 0;
+ }
+
+ // ---- Reactor lifecycle ----
+
+ /**
+ * Initialize reactor state from the session. Called by {@link
RichExecutionEventLogger}
+ * during {@code sessionStarted}.
+ */
+ public void initReactor(MavenSession session) {
+ List<MavenProject> allProjects = session.getAllProjects();
+ List<MavenProject> projects = session.getProjects();
+
+ this.totalProjects = allProjects.size();
+ this.completedProjects = allProjects.size() - projects.size();
+ this.buildStartTime = MonotonicClock.now();
+
+ for (MavenProject project : allProjects) {
+ projectOrder.add(project.getArtifactId());
+ projectNames.put(project.getArtifactId(), project.getName());
+ }
+
+ // Build header line
+ this.headerLine = buildHeaderLine(session);
+
+ // Slot count = degree of concurrency (capped for sanity)
+ int concurrency = 1;
+ try {
+ concurrency = Math.max(1,
session.getRequest().getDegreeOfConcurrency());
+ } catch (Exception e) {
+ // fallback to 1
+ }
+ int slotCount = Math.min(concurrency, 8);
+ // Fixed height: 1 header + N project slots + 1 separator + 1 summary
+ this.statusHeight = slotCount + 3;
+
+ if (supported) {
+ setupDisplay();
+ }
+ }
+
+ private String buildHeaderLine(MavenSession session) {
+ StringBuilder h = new StringBuilder();
+ h.append(' ').append(BOLD);
+
+ // Maven version
+ String mavenVersion = null;
+ if (session.getSystemProperties() != null) {
+ mavenVersion =
session.getSystemProperties().getProperty("maven.version");
+ }
+ if (mavenVersion != null) {
+ h.append("Maven ").append(mavenVersion);
+ } else {
+ h.append("Maven");
+ }
+ h.append(RESET);
+
+ // Project name
+ MavenProject top = session.getTopLevelProject();
+ if (top != null) {
+ h.append(DIM).append(" ─ ").append(RESET);
+ h.append("building ");
+ h.append(CYAN).append(top.getName()).append(RESET);
+ if (top.getVersion() != null) {
+ h.append('
').append(DIM).append(top.getVersion()).append(RESET);
+ }
+ }
+
+ // Goals
+ List<String> goals = session.getGoals();
+ if (goals != null && !goals.isEmpty()) {
+ h.append(DIM).append(" ─ ").append(RESET);
+ h.append(YELLOW).append(String.join(" ", goals)).append(RESET);
+ }
+
+ return h.toString();
+ }
+
+ /**
+ * Set up the JLine Display in non-fullscreen mode.
+ */
+ private void setupDisplay() {
+ synchronized (outputLock) {
+ display = new Display(terminal, false);
+ display.resize(statusHeight, terminal.getWidth());
+ displayActive = true;
+ display.updateAnsi(buildStatusLines(), 0);
+ }
+
+ // Start a 1-second periodic refresh so that elapsed-time counters
+ // stay live even when no build events are arriving (e.g. during
+ // a slow mojo execution with no log output).
+ ScheduledThreadPoolExecutor executor = new
ScheduledThreadPoolExecutor(1, r -> {
+ Thread t = new Thread(r, "maven-rich-display-refresh");
+ t.setDaemon(true);
+ return t;
+ });
+ executor.setRemoveOnCancelPolicy(true);
+ refreshScheduler = executor;
+ refreshFuture = refreshScheduler.scheduleAtFixedRate(this::redraw, 1,
1, TimeUnit.SECONDS);
+ }
+
+ /**
+ * Tear down the status display. Called by {@link RichExecutionEventLogger}
+ * during {@code sessionEnded} before printing the summary.
+ * <p>
+ * The flush at the end is critical: {@link Display} writes through
+ * {@link Terminal#writer()} (a {@code PrintWriter} that does not
auto-flush),
+ * while subsequent log output from SLF4J goes through {@code System.out}
+ * (which <em>does</em> auto-flush on {@code println}). Without the flush,
+ * the clear sequences sit in the writer's buffer while the summary text
+ * reaches the terminal first via {@code System.out} — then the belated
+ * clear erases the summary the user was supposed to see.
+ */
+ public void tearDown() {
+ // Stop the periodic refresh first (outside outputLock to avoid
deadlock)
+ if (refreshFuture != null) {
+ refreshFuture.cancel(false);
+ refreshFuture = null;
+ }
+ if (refreshScheduler != null) {
+ refreshScheduler.shutdownNow();
+ refreshScheduler = null;
+ }
+
+ synchronized (outputLock) {
+ if (!displayActive) {
+ return;
+ }
+ displayActive = false;
+
+ // Clear the display area: update with empty lines, cursor at top
+ display.updateAnsi(Collections.nCopies(statusHeight, ""), 0);
+ // Erase from cursor to end of screen — removes any leftover
artifacts
+ writer.print("\033[J");
+ // Flush immediately so the clear reaches the terminal BEFORE
+ // any subsequent log output that goes through System.out
+ writer.flush();
+ }
+ }
+
+ // ---- BuildEventListener interface ----
+
+ @Override
+ public void sessionStarted(ExecutionEvent event) {
+ // Reactor init is handled via initReactor() called from
RichExecutionEventLogger
+ }
+
+ @Override
+ public void projectStarted(String projectId) {
+ activeProjects.put(projectId, new ProjectState(projectId,
MonotonicClock.now()));
+ redraw();
+ }
+
+ @Override
+ public void projectFinished(String projectId) {
+ activeProjects.remove(projectId);
+ completedProjects++;
Review Comment:
✖️ **Bug: `completedProjects++` race in parallel builds**
`completedProjects` is `volatile int` (declared at line 103). The `++`
operator is a non-atomic compound read-modify-write: read → increment → write.
In a parallel build (`-T N`), two threads can invoke `projectFinished()`
concurrently, both read the same value, and one increment is silently lost. The
display will show `[11/12]` when 12 modules have actually completed, and never
reach `[12/12]`.
The `redraw()` call acquires `outputLock`, but the `++` happens before it —
outside the lock:
```java
public void projectFinished(String projectId) {
activeProjects.remove(projectId);
completedProjects++; // ← RACE: outside outputLock
redraw(); // ← outputLock acquired here
}
```
Fix: increment inside `outputLock`, or use `AtomicInteger` (consistent with
`warningCount`/`errorCount`):
```suggestion
public void projectFinished(String projectId) {
activeProjects.remove(projectId);
synchronized (outputLock) {
activeProjects.remove(projectId);
completedProjects++;
if (displayActive) {
display.updateAnsi(buildStatusLines(), 0);
}
}
}
```
Alternatively, change `completedProjects` to `AtomicInteger` and use
`completedProjects.incrementAndGet()` before `redraw()`.
--
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]
