slachiewicz opened a new issue, #12704:
URL: https://github.com/apache/maven/issues/12704
### Affected version
4.0.0-rc-4 through 4.0.0-rc-6, `maven-4.0.x`, and `master`. Verified by
running a locally built `4.1.0-SNAPSHOT` from `master`; the rc range is
established by tag containment and code inspection, not by running the released
rc binaries.
### Bug description
Under the concurrent builder, when one module fails and the reactor halts,
an **unrelated** module that was already in flight is reported as skipped —
even when it completed all of its requested work successfully — and the log
states a reason that is not true:
```
X was not built because a module it depends on failed to build.
```
Both halves of that sentence can be false: `X` may have no dependency on the
failed module, and `X` may in fact have been built.
#### Reproduction
Two sibling modules with no dependency between them. `slow` has 4000
generated sources so its `compile` takes ~1.5s; `fails` has one source with a
syntax error so its `compile` fails almost immediately.
```
root/pom.xml <modules>slow, fails</modules>, packaging pom
root/slow/pom.xml plain jar module, no dependencies
root/fails/pom.xml plain jar module, no dependencies
```
```bash
mkdir -p slow/src/main/java/gen fails/src/main/java
for i in $(seq 0 3999); do
printf 'package gen;\npublic class C%s { public int v() { return %s; }
}\n' $i $i \
> slow/src/main/java/gen/C$i.java
done
printf 'public class Bad implements int {}\n' > fails/src/main/java/Bad.java
mvn compile -b concurrent -T5
```
Result (5 runs out of 5 — it is a race, but one that is won consistently):
```
[INFO] --- compiler:3.13.0:compile (default-compile) @ slow ---
[INFO] --- compiler:3.13.0:compile (default-compile) @ fails ---
[INFO] Compiling 1 source file with javac ... to target/classes
[INFO] Compiling 4001 source files with javac ... to target/classes
[ERROR] .../fails/src/main/java/Bad.java:[1,29] unexpected type
[INFO]
------------------------------------------------------------------------
[INFO] Skipping slow
[INFO] slow was not built because a module it depends on failed to build.
[INFO]
------------------------------------------------------------------------
```
`slow` compiled all 4001 sources and `slow/target/classes/gen` contains 4000
fresh `.class` files (the `target` directories were removed before every run).
It nonetheless gets `ProjectSkipped` and no `BuildSummary`.
The same project under the legacy builder (`mvn compile -T5`) prints no
`Skipping` line: the in-flight project runs to completion and is reported as
succeeded.
#### Mechanism
All references are to
`impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java`
on `master`.
1. `fails` throws a `LifecycleExecutionException`, so `handleBuildError`
takes the `REACTOR_FAIL_FAST` branch and calls `ReactorBuildStatus.halt()`
(line 881).
2. `processStep` then evaluates every remaining `CREATED` regular step of
**every** project, including unrelated ones that are mid-build:
```java
shouldExecute = !status.isHalted() && !status.isBlackListed(step.project)
&& allPredecessorsExecuted; // :423
```
`isHalted()` is now true, so each step transitions `CREATED -> SKIPPED`
(line 452). With `-X`:
```
[DEBUG] Skipping step BuildStep[project=...:slow, phase=ready] because
the build is halted
[DEBUG] Skipping step BuildStep[project=...:slow, phase=test-compile]
because the build is halted
[DEBUG] Skipping step BuildStep[project=...:slow, phase=package] because
the build is halted
[DEBUG] Skipping step BuildStep[project=...:slow, phase=install] because
the build is halted
[DEBUG] Skipping step BuildStep[project=...:slow, phase=deploy] because
the build is halted
```
3. `TEARDOWN` always runs (line 416) and computes:
```java
allStepsExecuted &= step == projectStep || projectStep.status.get() ==
EXECUTED; // :535
```
`calculateLifecycleMappings` creates before/at/after steps for **every**
lifecycle phase regardless of the requested goal. Steps outside the requested
scope are passed through `BuildStep.skip()`, which only sets a flag and clears
the mojo list (`BuildStep.java:106-109`) — the step stays in the plan with
status `CREATED` and normally executes as an empty no-op. So for `mvn compile`,
`slow`'s plan still holds `ready`, `test-compile`, `test`, `package`, `install`
and `deploy` steps. After a halt these become `SKIPPED` rather than `EXECUTED`,
`allStepsExecuted` is false, and with `failures == null` the code falls through
to `ProjectSkipped` (line 558) instead of `ProjectSucceeded` (line 556).
In other words `allStepsExecuted` currently means *"the project traversed
the entire lifecycle plan"*, not *"the requested work completed"*.
4. `ExecutionEventLogger.projectSkipped`
(`impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:319`)
then prints the single hardcoded reason, because the event carries no reason
of its own.
#### Why the legacy builder does not show this
`LifecycleModuleBuilder` fires `ProjectSkipped` *before* `ProjectStarted`
and only for projects that never started (`LifecycleModuleBuilder.java:91-94`).
The concurrent builder introduced a state that did not previously exist:
started, ran its requested goals fine, then got cut off. (The message was never
fully accurate even in the legacy builder — under fail-fast a halted project
also gets the "a module it depends on failed" text — but it was at least
limited to projects that genuinely produced nothing.)
#### Provenance
`ProjectSkipped` was added to `BuildPlanExecutor` by 3706aa1e5c, "[MNG-8670]
Fix concurrent builder missing/wrong project events" (#2251, for #9636), first
released in 4.0.0-rc-4. Before that commit `TEARDOWN` fired `ProjectSucceeded`
unconditionally, so this is a regression of the reporting for
halted-but-complete projects rather than a pre-existing gap.
### Impact on the IT suite
This makes `MavenITmng8648ProjectEventsTest` flaky. The test builds a
five-module reactor with `compile -b concurrent -T5` where `subproject-c` fails
on purpose, and asserts:
```java
verifier.verifyTextInLog("org.apache.maven.its.mng8648:subproject-b:jar:1-SNAPSHOT
ProjectSucceeded"); // :52
```
`subproject-b` does not depend on `subproject-c` (only `subproject-d` does),
so whether `b` gets `ProjectSucceeded` or `ProjectSkipped` depends on whether
`c` fails before `b`'s empty compile finishes. It went the wrong way on
`integration-tests (macos-latest, 21)` in
https://github.com/apache/maven/actions/runs/31221127749/job/93006825497
(attempt 1 of that run; attempt 2 was green, which is why the run now shows as
successful):
```
subproject-b:jar:1-SNAPSHOT ProjectStarted
[INFO] --- compiler:3.13.0:compile (default-compile) @ subproject-b ---
subproject-c:jar:1-SNAPSHOT ProjectFailed
[INFO] No sources to compile
subproject-b:jar:1-SNAPSHOT ProjectSkipped
[INFO] Skipping subproject-b
[INFO] subproject-b was not built because a module it depends on failed to
build.
```
Only 1 of 12 integration-test matrix jobs failed in that run.
### Suggested direction
1. **Event.** Base the outcome on whether the project's *in-scope* work
completed, i.e. compute `allStepsExecuted` over steps that were not
`skip()`-flagged (or equivalently require every mojo-bearing step to have
reached `EXECUTED`). A project halted with in-scope mojos still pending
genuinely did not complete and should stay `ProjectSkipped`; one that ran
everything asked of it should be `ProjectSucceeded`. This keeps the MNG-8670
use case intact — `subproject-d`, which really was never built, still gets
`ProjectStarted` + `ProjectSkipped` — and it matches what the MNG-8670 IT
itself asserts for `subproject-b`.
2. **Message.** `ProjectSkipped` now has at least two causes (blacklisted
because a dependency failed vs. reactor halted). The logger cannot tell them
apart today and always claims the first. The reason needs to reach
`ExecutionEventLogger`, so a halted project reads "the build was halted after
an earlier failure" instead.
3. **IT.** Fixing the event alone does not fully deflake
`MavenITmng8648ProjectEventsTest`: if `c` fails before `b`'s steps are even
scheduled, `b` is legitimately skipped and the assertion still fails. The test
needs an ordering guarantee — e.g. make `subproject-c` fail slowly so `b`
reliably finishes first — rather than a loosened assertion.
I am happy to put up a PR for 1 and 2 if the direction looks right.
--
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]
