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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
