You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "slachiewicz (via GitHub)" <gi...@apache.org> on 2026/08/08 07:25:04 UTC
[PR] Fix #12704: Do not report an independent, built module as skipped [maven]
slachiewicz opened a new pull request, #12705:
URL: https://github.com/apache/maven/pull/12705
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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #12704: Do not report an independent, built module as skipped [maven]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on code in PR #12705:
URL: https://github.com/apache/maven/pull/12705#discussion_r3740278624
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -316,12 +316,34 @@ public void projectSkipped(ExecutionEvent event) {
infoLine('-');
String name = event.getProject().getName();
infoMain("Skipping " + name);
- logger.info("{} was not built because a module it depends on failed to build.", name);
+ if (dependsOnFailedProject(event)) {
+ logger.info("{} was not built because a module it depends on failed to build.", name);
+ } else {
+ logger.info("{} was not built because the build was stopped after an earlier failure.", name);
+ }
infoLine('-');
}
}
+ /**
+ * A project can be skipped for two different reasons: one of the modules it depends on failed,
+ * or the reactor was stopped after an unrelated module failed. Only the first one lets us blame
+ * a dependency, so tell them apart instead of always reporting the same cause.
+ * When the answer cannot be established, the dependency wording is kept.
+ */
+ private boolean dependsOnFailedProject(ExecutionEvent event) {
+ MavenSession session = event.getSession();
+ MavenProject project = event.getProject();
+ if (session == null || project == null || session.getProjectDependencyGraph() == null) {
+ return true;
+ }
+ MavenExecutionResult result = session.getResult();
+ return result == null
+ || session.getProjectDependencyGraph().getUpstreamProjects(project, true).stream()
+ .anyMatch(upstream -> result.getBuildSummary(upstream) instanceof BuildFailure);
+ }
Review Comment:
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:
```java
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.
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/event/ExecutionEventLoggerTest.java:
##########
@@ -446,6 +447,66 @@ void testSessionEndedFailureMultimoduleWithSeparatedFailures() {
inOrder.verify(logger).info("------------------------------------------------------------------------");
}
+ @Test
+ void testProjectSkippedBecauseADependencyFailed() {
+ // prepare
+ MavenProject failed = generateMavenProject("Maven Project artifact1");
+ MavenProject skipped = generateMavenProject("Maven Project artifact2");
+
+ DefaultMavenExecutionResult executionResult = new DefaultMavenExecutionResult();
+ executionResult.addBuildSummary(new BuildFailure(failed, 1000, new Exception("Failure")));
+
+ ExecutionEvent event = skipEvent(skipped, executionResult, Arrays.asList(failed));
+
+ // execute
+ executionEventLogger.projectSkipped(event);
+
+ // verify
+ InOrder inOrder = inOrder(logger);
+ inOrder.verify(logger).info("Skipping Maven Project artifact2");
+ inOrder.verify(logger)
+ .info("{} was not built because a module it depends on failed to build.", "Maven Project artifact2");
+ }
+
+ @Test
+ void testProjectSkippedBecauseTheBuildWasStopped() {
Review Comment:
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.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #12704: Do not report an independent, built module as skipped [maven]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #12705:
URL: https://github.com/apache/maven/pull/12705#discussion_r3740261514
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/event/ExecutionEventLoggerTest.java:
##########
@@ -446,6 +447,66 @@ void testSessionEndedFailureMultimoduleWithSeparatedFailures() {
inOrder.verify(logger).info("------------------------------------------------------------------------");
}
+ @Test
+ void testProjectSkippedBecauseADependencyFailed() {
+ // prepare
+ MavenProject failed = generateMavenProject("Maven Project artifact1");
+ MavenProject skipped = generateMavenProject("Maven Project artifact2");
+
+ DefaultMavenExecutionResult executionResult = new DefaultMavenExecutionResult();
+ executionResult.addBuildSummary(new BuildFailure(failed, 1000, new Exception("Failure")));
+
+ ExecutionEvent event = skipEvent(skipped, executionResult, Arrays.asList(failed));
+
+ // execute
+ executionEventLogger.projectSkipped(event);
+
+ // verify
+ InOrder inOrder = inOrder(logger);
+ inOrder.verify(logger).info("Skipping Maven Project artifact2");
+ inOrder.verify(logger)
+ .info("{} was not built because a module it depends on failed to build.", "Maven Project artifact2");
+ }
+
+ @Test
+ void testProjectSkippedBecauseTheBuildWasStopped() {
Review Comment:
The new skip-reason logic has an important “unknown” case described in the PR: when upstream projects exist but their outcomes cannot be established yet, the logger should keep the dependency wording. Adding a unit test for this case will prevent regressions (and will fail with the current implementation that treats “no BuildFailure summaries yet” as “build was stopped”).
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/event/ExecutionEventLogger.java:
##########
@@ -316,12 +316,34 @@ public void projectSkipped(ExecutionEvent event) {
infoLine('-');
String name = event.getProject().getName();
infoMain("Skipping " + name);
- logger.info("{} was not built because a module it depends on failed to build.", name);
+ if (dependsOnFailedProject(event)) {
+ logger.info("{} was not built because a module it depends on failed to build.", name);
+ } else {
+ logger.info("{} was not built because the build was stopped after an earlier failure.", name);
+ }
infoLine('-');
}
}
+ /**
+ * A project can be skipped for two different reasons: one of the modules it depends on failed,
+ * or the reactor was stopped after an unrelated module failed. Only the first one lets us blame
+ * a dependency, so tell them apart instead of always reporting the same cause.
+ * When the answer cannot be established, the dependency wording is kept.
+ */
+ private boolean dependsOnFailedProject(ExecutionEvent event) {
+ MavenSession session = event.getSession();
+ MavenProject project = event.getProject();
+ if (session == null || project == null || session.getProjectDependencyGraph() == null) {
+ return true;
+ }
+ MavenExecutionResult result = session.getResult();
+ return result == null
+ || session.getProjectDependencyGraph().getUpstreamProjects(project, true).stream()
+ .anyMatch(upstream -> result.getBuildSummary(upstream) instanceof BuildFailure);
+ }
Review Comment:
`dependsOnFailedProject` can misclassify dependency-related skips as “build was stopped” when upstream build summaries have not been recorded yet (common in concurrent execution). In that situation the cause is not established, and per the method javadoc/PR description the logger should keep the dependency wording instead of switching to the “stopped” wording.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix #12704: Do not report an independent, built module as skipped [maven]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz merged PR #12705:
URL: https://github.com/apache/maven/pull/12705
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org