Fixes #13135, ensure completude of the reactor summary but priviledge failures to be last to stay human efficient by rmannibucau · Pull Request #13136 · apache/maven

rmannibucau · 2026-09-15T08:12:14Z

Goal is to ensure the reactor summary stays complete so we see some modules were built and not just the one(s) failling.

Following this checklist to help us incorporate your
contribution quickly and easily:

Your pull request should address just one issue, without pulling in other changes.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
Run mvn verify to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
[-] You have run the Core IT successfully.

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

gnodet-bot

The goal of showing complete reactor summaries is sound — the old behavior of hiding successful modules on failure was indeed confusing. The grouping approach (skipped→success→failure) is clean. But there's a behavioral regression that needs addressing before merge.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-15T08:18:29Z

            }

-            if (shouldSkip) {
+            if (group == 0 && entry.buildSummary() == null) {


⚠️ Behavioral regression — SKIPPED modules silently dropped on successful partial builds

This condition unconditionally filters out all null-buildSummary entries regardless of whether the build succeeded or failed. The old code gated this on result.hasExceptions():

// Old boolean shouldSkip = result.hasExceptions(); // only true when build failed

With the new code, running mvn -pl moduleA install (or any --also-make / --resume-from subset) on a multi-module project produces a reactor summary that completely omits the modules that were never built. Their buildSummary is null and they silently vanish — no SKIPPED line, no ... prefix, nothing. This is a regression from Maven 3 / pre-#11977 behavior.

Fix: carry result.hasExceptions() into ReactorSummaryRequest (or pass it as a parameter) and only suppress null-buildSummary entries when there are exceptions:

Suggested change

if (group == 0 && entry.buildSummary() == null) {

if (group == 0 && entry.buildSummary() == null && request.hasExceptions()) {

…and add boolean hasExceptions to ReactorSummaryRequest, set from result.hasExceptions() at call site.

gnodet-bot · 2026-09-15T08:18:30Z

            }

-            buffer.append(project.getName());
+            StringBuilder buffer = request.buffer();


🔧 Minor: request.buffer() re-assigned on every iteration — misleading

The StringBuilder is fetched from the record on every loop iteration, producing a new local variable buffer each time that refers to the same shared object. It reads like a fresh allocation each time but is not — a reader might wonder why it's inside the loop rather than hoisted before it. Move it before the loop:

Suggested change

StringBuilder buffer = request.buffer();

StringBuilder buffer = request.buffer();

(i.e., hoist this line to just before the for loop, and remove it from inside the loop body)

gnodet-bot · 2026-09-15T08:18:30Z

        inOrder.verify(logger).info("Reactor Summary for Maven Project artifact1 3.5.4-SNAPSHOT:");
        inOrder.verify(logger).info("");
        inOrder.verify(logger).info("...");
+        inOrder.verify(logger).info("Maven Project artifact1 ............................ SUCCESS [  1.000 s]");


📋 Missing test: successful partial build with some modules not built

The only test scenarios covering null-buildSummary entries are failure scenarios. There's no test for:

mvn -pl moduleA install // moduleB, moduleC → null buildSummary, no exceptions

With the current implementation, those modules silently disappear from the summary (see the regression comment on logReactorSummaryGroup). A test like testSessionEndedSuccessWithSkippedModules would have caught it:

project1 → BuildSuccess

project2 → no build summary (null)

project3 → BuildSuccess

No exception added

Expected: project1 SUCCESS, project2 SKIPPED, project3 SUCCESS (in build order)

slawekjaranowski · 2026-09-15T08:30:56Z

Skipping modules for which the build passed was intentional, according to issue

[MNG-7008] skip reactor logging #8027

Now, when we have a project with hundreds of modules, we will have a full list and need to scroll a long way up to see the root error.

Maybe this behavior should be parameterized?

slawekjaranowski · 2026-09-15T08:34:36Z

Another idea: show a summary with only the number of successful and skipped modules, and print the details only for the ones that failed.

For pass build we print all details as is now.

rmannibucau · 2026-09-15T09:28:11Z

@slawekjaranowski

Now, when we have a project with hundreds of modules, we will have a full list and need to scroll a long way up to see the root error.

not at all, this PR solves the original issue by sorting by status, failures last.

note that having a single module in the report for hundreds of module built is at least as much bothering than scrolling (looks like a bug in the execution and not only the report)

guess the proposal solves both cases and don't need a parameter since you don't need to scroll anymore (and colors help) and you have the failures immediately at the end (where you terminal is if it autoscroll like most of them)

on code feedback: will handle the hasException and buffer points

gnodet-bot

Re-review after updated commit.

All three findings from the previous REQUEST_CHANGES review are addressed:

✅ Behavioral regression (null buildSummary always suppressed): fixed — request.hasExceptions() now gates suppression correctly
✅ buffer fetched inside the loop: fixed — hoisted before the for loop
✅ Missing test for success-with-skipped-modules: testSessionEndedSuccessWithSkippedModules added, and existing failure tests updated to reflect the new ordering

Two new observations below.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

slawekjaranowski · 2026-09-15T10:53:39Z

@slawekjaranowski

Now, when we have a project with hundreds of modules, we will have a full list and need to scroll a long way up to see the root error.

not at all, this PR solves the original issue by sorting by status, failures last.

note that having a single module in the report for hundreds of module built is at least as much bothering than scrolling (looks like a bug in the execution and not only the report)

guess the proposal solves both cases and don't need a parameter since you don't need to scroll anymore (and colors help) and you have the failures immediately at the end (where you terminal is if it autoscroll like most of them)

on code feedback: will handle the hasException and buffer points

Not exactly, look at my screen shots

Maven 4.0.0-rc-6:

I see everything on one screen

Your proposition:

I need scroll terminal up to see description of errors

slawekjaranowski

In original issue we have:

Can we have a way to skip reactor logging, i.e. the reactor build order and/or reactor summary.

I have hundreds of modules in my reactor build and I always have to scroll back pages to see the error message.

#8027

here we remove requirements of skipping ... so issue should be reopened or closed as not planed if we merge this one.

slawekjaranowski · 2026-09-15T12:10:24Z

I am convinced that skipping correctly built modules when an error occurs is a good solution.
I would like to hear the opinions of others, including the author of the original issue - @delanym

for me -1 sorting/grouping introduce the problem again

rmannibucau · 2026-09-15T12:30:25Z

@slawekjaranowski ok I get your point but this is only relevant to a very particular case: monothreaded build, for all other cases it is counter productive. Also your two screenshots highlights the issue: "why did it build a single module? it is obviously why the test failed", so this is more misleading in most cases IMHO. I'd also like to emphasis we already had a solution to skip the summary: -Dmaven.logger.log.org.apache.maven.cling.event.ExecutionEventLogger=warn so adjusting the log level to the status of the build would make sense. can it be a compromise which will make everything happy?

side note: sorting solves the issue "where is the failed module" when you have hundreds of module (camel, hop, tomee, nifi etc), doesnt solve "present me all errors" but we'll never solve this one by design until it is a trivial case = single module with a single simple error just cause the errors can take more than a screen and you can get a chain of errors (think JAXRS for ex), so don't think we should fight the issue you mention but only the summary one.

…failures to be last to stay human efficient

gnodet · 2026-09-15T12:45:33Z

Thanks for the fix. Worth noting that #12697 (and its dependency chain starting at #12695) is introducing new event loggers (PlainExecutionEventLogger, RichBuildEventListener, etc.) that replace ExecutionEventLogger. The reactor summary behaviour you're fixing here would need to be ported to those new loggers as well — or alternatively, this fix could be implemented there directly if the new chain lands first. Just flagging so the work isn't duplicated or lost.

gnodet-bot

Re-review after updated commit.

Previous findings status:

✅ Behavioral regression (null buildSummary always suppressed): fixed — gated on request.hasExceptions() ✓
✅ buffer fetched inside the for loop: fixed — hoisted to top of logReactorSummaryGroup ✓
✅ Missing test for success-with-skipped-modules: testSessionEndedSuccessWithSkippedModules added ✓
❌ Mutable StringBuilder inside ReactorSummaryRequest record: still present (see inline)
❌ UNKNOWN buildSummary silently suppressed on failure: still present (see inline)

Note on design debate: @slawekjaranowski has a CHANGES_REQUESTED review open on the overall grouping/ordering approach. That is a design-level question between maintainers — not something I can adjudicate. The two open technical findings below apply regardless of which design direction is chosen.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-15T12:47:57Z

    }

+    private record ReactorSummaryRequest(
+            List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean isSingleVersion, boolean hasExceptions) {}


🔧 Same mutable-StringBuilder-in-record issue as compat copy — apply the same fix (allocate locally in logReactorSummaryGroup, drop the field from ReactorSummaryRequest).

rmannibucau · 2026-09-15T12:49:33Z

@gnodet good call, guess this will need some agreement on the target now before we get the new ones :(. Right now I needed to revert several builds to rc5 due to that so hope we dont take too much time 🤞 .

delanym · 2026-09-15T13:01:04Z

@rmannibucau for a long running build -Dmaven.logger.log.org.apache.maven.cling.event.ExecutionEventLogger=warn or -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.event.ExecutionEventLogger=warn leaves you staring at a blank screen wondering whether its progressing or frozen. We do do that, and then get echo-plugin to print [INFO] Building a:b:c. Bit of a hack.

rmannibucau · 2026-09-15T14:04:19Z

@delanym the org.apache.maven.cli.event.ExecutionEventLogger.detail logger hack sounds like solving that and global build status stays on org.apache.maven.cli.event.ExecutionEventLogger logger. But looks better than current state where it looks like the build just didn't work (not failed, didnt work)

gnodet · 2026-09-15T14:31:35Z

@gnodet good call, guess this will need some agreement on the target now before we get the new ones :(. Right now I needed to revert several builds to rc5 due to that so hope we dont take too much time 🤞 .

If there's a regression, we can fix it in rc7. The PR I pointed at are all for 4.1.0.

gnodet · 2026-09-15T14:33:44Z

In original issue we have:

Can we have a way to skip reactor logging, i.e. the reactor build order and/or reactor summary.
I have hundreds of modules in my reactor build and I always have to scroll back pages to see the error message.

[MNG-7008] skip reactor logging #8027

here we remove requirements of skipping ... so issue should be reopened or closed as not planed if we merge this one.

But this issue does not advertise fixing #8027. It just links to it because it's related IIUC.

rmannibucau · 2026-09-15T14:46:31Z

@gnodet my understand of the issue was that there are too much summary lines so you dont see the failed one fast enough, this part is solved by this PR. Slawomir completed it was also a goal to see the test failure inline immediately so solve space -> this one I think will never really be solved by design until you do yet another "logger" (listener) which does filter and there we'll not get an agreement of the default so my proposal is to stay conservative on the default, keep a consistent output (all or nothing, sorted if it helps, but never a filtered summary which looks just like maven was broken for the run) and accept to scroll for test cause you will scroll anyway since the suite is executed in general (once again some edge cases would benefit from the code which led to the regression but these are corner cases).

gnodet-bot · 2026-09-15T15:03:14Z

Thanks @rmannibucau for the fix — the sorting approach is the right direction.

One issue remains in the latest commit: modules with buildSummary == null (i.e. modules that were never built because the build was aborted after a failure upstream) are still silently suppressed when hasExceptions() is true:

if (group == 0 && entry.buildSummary() == null && request.hasExceptions()) {
    lastWasSkipped = true;
    continue;  // ← these modules disappear from the summary
}

This means the reactor summary is still incomplete on failure — just for a different category of modules than before. Users with a 100-module project where 80 built successfully and 19 were never reached won't see those 19 in the summary at all, which is exactly the confusion #13135 describes.

The fix is simply to remove those three lines. With group-based sorting, buildSummary == null modules land in group 0 (SKIPPED), appear first, and failures still end up at the bottom where the terminal cursor sits. No need to hide anything.

Also, the detailLogger split adds complexity without clear benefit — the core value of this PR is the sorting, not the logger separation.

gnodet-bot · 2026-09-15T15:03:46Z

Note that the new event logger infrastructure coming in 4.1.0 (see #12697 and its dependency chain starting at #12695 — PlainExecutionEventLogger, RichBuildEventListener, etc.) will replace ExecutionEventLogger entirely. That's a better place to implement smarter reactor summary behaviour (collapsible sections, richer formatting, etc.) if we want to go further down the road. For now, keeping this fix minimal and correct for the current logger makes sense.

gnodet-bot

The sorting approach is right — failures at the bottom of the terminal is exactly what we want. One remaining issue below.

gnodet-bot · 2026-09-15T15:36:25Z

This block silently suppresses modules that were never built (buildSummary == null), which is the same incompleteness that #13135 was trying to fix — just for a different category of modules. With the group-based sorting, these modules already land in group 0 and display as SKIPPED before the successes and failures. There's no reason to hide them.

Dropping these 4 lines gives a complete summary on failure: SKIPPED (not built) → SUCCESS → FAILURE, with failures always last where the terminal cursor sits.

Keep SKIPPED modules visible regardless of build failure and rely on group ordering (skipped, success, failure) to bring failures last. Remove the separate .detail logger added earlier as it brought no benefit, and drop the now unused lastWasSkipped placeholder handling.

gnodet-bot

Re-review after latest commit (75f5320).

Previous findings status:

✅ Behavioral regression (null buildSummary always suppressed): fully resolved — no suppression at all now, all entries always rendered
✅ Missing test for successful partial build with skipped modules: testSessionEndedSuccessWithSkippedModules added
✅ UNKNOWN → group = 0 suppression concern: moot — the concern was about the old shouldSkip gating. With no suppression remaining, UNKNOWN entries always render in group 0 (before successes). Visible, not lost.
🔄 Mutable StringBuilder inside record: still present — see inline comment below.

One open issue remains.

This review was generated by an AI agent, Hermès on behalf of @gnodet.

gnodet-bot · 2026-09-15T15:48:23Z

    }

+    private record ReactorSummaryRequest(
+            List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean isSingleVersion) {}


🔧 Mutable StringBuilder in record — still present, checkstyle argument doesn't hold

The author previously responded that the StringBuilder is in the record as a checkstyle workaround. But checkstyle method-length rules apply to the method body where the code lives — in this case the relevant restriction would be on logReactorSummary, which is already short. The StringBuilder allocation belongs in logReactorSummaryGroup, which is a newly added, short method — no checkstyle constraint applies there.

The fix:

Suggested change

List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean isSingleVersion) {}

private record ReactorSummaryRequest(

List<ReactorSummaryEntry> entries, boolean isSingleVersion) {}

Then in logReactorSummaryGroup, change StringBuilder buffer = request.buffer(); to StringBuilder buffer = new StringBuilder(128);, and update the call site in logReactorSummary:

ReactorSummaryRequest request = new ReactorSummaryRequest(entries, isSingleVersion);

A record carrying mutable shared state that is mutated by three consecutive callers is a correctness trap — any future refactor that calls logReactorSummaryGroup twice in parallel or reorders the calls will silently corrupt the buffer. Please fix.

gnodet-bot · 2026-09-15T15:48:24Z

    }

+    private record ReactorSummaryRequest(
+            List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean isSingleVersion) {}


🔧 Same mutable-StringBuilder-in-record issue as the compat copy. Apply the same fix: drop StringBuilder buffer from ReactorSummaryRequest, allocate locally at the top of logReactorSummaryGroup.

rmannibucau · 2026-09-16T06:14:58Z

are we good now like that and we revisit with the new listeners - one being summary-free maybe?

slawekjaranowski · 2026-09-16T07:39:13Z

Never mind of technical implementation, we should listen users what they need.
Here @delanym need do some a hack to achieve expected result.
For this change we have opinion only of two maintainer, maybe we can try simple discussion on user ML to better know wat users needs.

rmannibucau · 2026-09-16T07:42:08Z

@slawekjaranowski a very gently reminder I'm an user too and this is a bug for the user I am ;) - no technical concern there, if you want to do it using asm i'm fine

delanym · 2026-09-16T07:46:03Z

@slawekjaranowski it sounds like bigger and better things are coming, so that's just fine. I'm playing the long game.

rmannibucau · 2026-09-16T08:06:02Z

FTR: don't get me wrong, I'm not against enabling that use case (this was the logger code in the PR I removed after Guillaume's feedback), but I'm against an impacting regression. 100% aligned what Guillaume prepared should cover everyone needs so stability short term, enhancement mid terms.

gnodet

I think this is the correct way to go until we can do more in 4.1.0.
The problem here, not having the info, is worse than having too much info.

Before merging, the technical aspect needs to be finalized, and the other issue closed as not planned anymore.

rmannibucau · 2026-09-16T13:47:39Z

@gnodet can you highlight the tech aspect you have in mind (happy to discuss on slack if it helps), latest report of claude review were not relevant from my point of view (or intended to be more exact) and the skipped line issue was fixed IIRC. About the other issue, do you reference #8027 ? Think we can keep open and see if the new listeners can fix it, no?

gnodet · 2026-09-16T15:42:10Z

@gnodet can you highlight the tech aspect you have in mind (happy to discuss on slack if it helps), latest report of claude review were not relevant from my point of view (or intended to be more exact) and the skipped line issue was fixed IIRC. About the other issue, do you reference #8027 ? Think we can keep open and see if the new listeners can fix it, no?

That one seemed relevant, you disagree with the analysis ?

#13136 (comment)

rmannibucau · 2026-09-16T16:12:49Z

@gnodet (on the phone) thought i fixed this one but yeah I agree

gnodet · 2026-09-16T16:48:57Z

@gnodet (on the phone) thought i fixed this one but yeah I agree

I'll push a fix.

gnodet · 2026-09-16T17:32:41Z

@gnodet (on the phone) thought i fixed this one but yeah I agree

I'll push a fix.

You actually fixed it, you were right.

LGTM

slawekjaranowski

ok, in favor of new console modes ... I will not block it.

please be aware of similar change in 4.0.x and 3.10.x branches

some nit to consider or confirm that is intended:

order: skipped, success, failure - is different that old
with error log level - we have inconsistent indentation in the messages

rmannibucau · 2026-09-16T18:29:19Z

@slawekjaranowski ordering is intended to help identify failed modules faster (not the error but which ones which is already a baby step forward), indentation is a side effect. I think the benefit > the lost this way but on my side no strong issue to revert the level if desired.

rmannibucau added the mvn4 label Sep 15, 2026

rmannibucau requested a review from slawekjaranowski September 15, 2026 08:12

gnodet-bot suggested changes Sep 15, 2026

View reviewed changes

rmannibucau force-pushed the fix/13135 branch from c61e988 to e2f14e2 Compare September 15, 2026 09:32

gnodet-bot reviewed Sep 15, 2026

View reviewed changes

slawekjaranowski requested changes Sep 15, 2026

View reviewed changes

rmannibucau mentioned this pull request Sep 15, 2026

[MNG-7008] skip reactor logging #8027

Open

Fixes 13135, ensure completude of the reactor summary but priviledge …

a36f758

…failures to be last to stay human efficient

rmannibucau force-pushed the fix/13135 branch from e2f14e2 to a36f758 Compare September 15, 2026 12:44

gnodet-bot reviewed Sep 15, 2026

View reviewed changes

Log reactor summary per-module detail on a nested .detail logger

3ef7ff9

gnodet changed the title ~~Fixes 13135, ensure completude of the reactor summary but priviledge failures to be last to stay human efficient~~ Fixes #13135, ensure completude of the reactor summary but priviledge failures to be last to stay human efficient Sep 15, 2026

gnodet-bot reviewed Sep 15, 2026

View reviewed changes

gnodet approved these changes Sep 16, 2026

View reviewed changes

slawekjaranowski approved these changes Sep 16, 2026

View reviewed changes

gnodet added the backport-to-4.0.x label Sep 16, 2026

gnodet added this to the 4.1.0 milestone Sep 16, 2026

	if (group == 0 && entry.buildSummary() == null) {
	if (group == 0 && entry.buildSummary() == null && request.hasExceptions()) {

	StringBuilder buffer = request.buffer();
	StringBuilder buffer = request.buffer();

	List<ReactorSummaryEntry> entries, StringBuilder buffer, boolean isSingleVersion) {}
	private record ReactorSummaryRequest(
	List<ReactorSummaryEntry> entries, boolean isSingleVersion) {}

Conversation

rmannibucau commented Sep 15, 2026

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

slawekjaranowski commented Sep 15, 2026

Uh oh!

slawekjaranowski commented Sep 15, 2026

Uh oh!

rmannibucau commented Sep 15, 2026

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slawekjaranowski commented Sep 15, 2026

Uh oh!

slawekjaranowski left a comment

Choose a reason for hiding this comment

Uh oh!

slawekjaranowski commented Sep 15, 2026

Uh oh!

rmannibucau commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnodet commented Sep 15, 2026

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rmannibucau commented Sep 15, 2026

Uh oh!

delanym commented Sep 15, 2026

Uh oh!

rmannibucau commented Sep 15, 2026

Uh oh!

gnodet commented Sep 15, 2026

Uh oh!

gnodet commented Sep 15, 2026

Uh oh!

rmannibucau commented Sep 15, 2026

Uh oh!

gnodet-bot commented Sep 15, 2026

Uh oh!

gnodet-bot commented Sep 15, 2026

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet-bot left a comment

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet-bot Sep 15, 2026

Choose a reason for hiding this comment

Uh oh!

rmannibucau commented Sep 15, 2026 •

edited

Loading

gnodet left a comment •

edited

Loading