Fix #12704: Do not report an independent, built module as skipped

[slachiewicz]

Fixes #12704.

1. The event

BuildPlanExecutor's TEARDOWN decided between ProjectSucceeded and ProjectSkipped on allStepsExecuted, which required every step of the project's plan to have reached EXECUTED. The plan holds a step for every phase of the lifecycle regardless of the requested tasks, so for mvn compile a project keeps empty package, install and deploy steps that normally run as no-ops. As soon as the reactor is halted those get skipped, allStepsExecuted turns false, and a project that had already finished everything asked of it is reported as skipped.

The outcome is now based on the steps that actually carry mojo executions. A project that ran all of its work is reported as succeeded; one that still had mojo-bearing steps pending stays skipped. A project whose SETUP step never ran was never started at all, so it stays skipped too — this keeps aggregators and other modules with no work of their own from being reported as succeeded when they were genuinely never built.

BuildStep.hasExecutions() is added for that predicate. (BuildStep.skip() looks like it was meant for this, but it has no callers, so out-of-scope phases are distinguishable only by having no mojos.)

2. The message

ExecutionEventLogger.projectSkipped always printed:

X was not built because a module it depends on failed to build.

That is the only reason the legacy builder ever produced, because LifecycleModuleBuilder fires ProjectSkipped before ProjectStarted and only for projects that never started. The concurrent builder also fires it for projects stopped by an unrelated failure, where the sentence is simply untrue. The logger now checks whether any upstream project actually failed and says which of the two happened; when the answer cannot be established the existing wording is kept.

Before / after

Two sibling modules with no dependency between them, plus one that does depend on the failing module. slow has 4000 generated sources so it is still compiling when fails breaks:

$ mvn compile -b concurrent -T5

before:

[INFO] Skipping dependent
[INFO] dependent was not built because a module it depends on failed to build.
[INFO] Skipping slow
[INFO] slow was not built because a module it depends on failed to build.

after:

[INFO] Skipping dependent
[INFO] dependent was not built because a module it depends on failed to build.

slow compiles its 4001 sources, writes its 4000 class files and is reported as succeeded; dependent is still skipped, with the reason that is true for it. Reproduced 5 runs out of 5 in both directions.

Testing

• Two new cases in ExecutionEventLoggerTest covering both skip reasons.
• MavenITmng8648ProjectEventsTest passes against the patched distribution — subproject-a and subproject-b succeed, subproject-c fails, subproject-d is skipped and keeps the dependency message.
• Unit tests of impl/maven-core and impl/maven-cli are green (635 tests).

Not addressed here

MavenITmng8648ProjectEventsTest remains timing-sensitive: if subproject-c fails before subproject-b's steps are scheduled at all, b is legitimately skipped and the assertion on ProjectSucceeded still fails. That needs an ordering guarantee in the test rather than a change in core, so I left it out of this PR. It is the flake described in #12704.

[Copilot]

Pull request overview

Fixes #12704 by improving how the concurrent builder reports project outcomes when the reactor halts: projects that completed all requested mojo work are now reported as succeeded (not skipped), and the CLI “skipped” message is refined to avoid blaming dependencies when the build was stopped due to an unrelated failure.

Changes:

• Add BuildStep.hasExecutions() to identify lifecycle-plan steps that actually contain mojo work.
• In BuildPlanExecutor TEARDOWN, compute “success vs skipped” based on completion of mojo-bearing steps plus whether the project’s SETUP actually ran.
• In ExecutionEventLogger, differentiate skip messaging between “dependency failed” vs “build stopped”; add unit tests for both reasons.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildStep.java	Adds hasExecutions() predicate to distinguish work steps from ordering/no-op steps.
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java	Adjusts TEARDOWN outcome logic to treat “all requested mojo work completed” as success, even if out-of-scope steps are later skipped due to halt.
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java	Updates skipped-project logging to choose an accurate reason instead of always blaming dependencies.
impl/maven-cli/src/test/java/org/apache/maven/cling/event/ExecutionEventLoggerTest.java	Adds unit tests covering the two skip reasons (dependency failure vs unrelated earlier failure).

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[slachiewicz]

Same as the other thread: the "summaries not recorded yet" state cannot be observed here, because the failed project's BuildFailure is added before the halt or blacklist that makes any other project skippable. A test for it would have to assert behaviour for a state the builder never produces.

I have added unit tests for the two reachable cases already, and BuildStep.hasExecutions() is now covered by BuildStepTest in d9a7f69.

[slachiewicz]

I checked this and the race it describes cannot happen.

A project can only be skipped after the failure that causes it, and handleBuildError records the failed project's summary before it halts or blacklists anything:

buildContext.getResult().addException(t);
buildContext.getResult().addBuildSummary(new BuildFailure(mavenProject, ...));   // first
...
buildContext.getReactorBuildStatus().blackList(mavenProject);                    // or halt(), after

The legacy builder does the same in BuilderCommon.handleBuildError. So by the time any other project can observe the halt or the blacklist and reach its own TEARDOWN, the BuildFailure is already in the result.

Visibility across threads is covered too: DefaultMavenExecutionResult.buildSummaries is a Collections.synchronizedMap(new IdentityHashMap<>()), so the write happens-before the read.

The "cannot be established" fallback in the javadoc is about the session, the dependency graph or the result being absent altogether, not about a summary arriving late — those are the three null checks above the stream.

[gnodet]

Well-scoped fix for the regression introduced in MNG-8670. The logic correctly distinguishes completed-but-halted projects from genuinely skipped ones by checking only mojo-bearing steps (hasExecutions()), and the improved skip messages accurately attribute the cause (dependency failure vs. halt).

The fix handles edge cases correctly:

• Aggregator/POM-only modules: If SETUP ran but the project has no mojo-bearing steps, it reports as succeeded (started, nothing to do). If SETUP never ran, it stays skipped.
• dependsOnFailedProject: Falls back to the original message wording when session state is unavailable, preserving backward compatibility.
• Ordering: The failing project's BuildFailure is added to the result before the skipped project's TEARDOWN fires, so the dependency check is reliable.

The PR description is exceptionally thorough — traces the root cause, explains the mechanism, documents before/after behavior, and scopes out remaining IT flakiness.

One minor note: hasExecutions() is a trivial public API addition (return !mojos.isEmpty()) with no dedicated unit test, but the behavior is covered indirectly by MavenITmng8648ProjectEventsTest.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet