gnodet-bot commented on code in PR #13270:
URL: https://github.com/apache/maven/pull/13270#discussion_r4103535405
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java:
##########
@@ -234,7 +234,12 @@ public List<MojoExecution>
calculateMojoExecutions(MavenSession session, MavenPr
calculateLifecycleMappings(session, project,
lifecyclePhase);
for (List<MojoExecution> mojoExecutionsFromLifecycle :
phaseToMojoMapping.values()) {
- mojoExecutions.addAll(mojoExecutionsFromLifecycle);
+ List<String> containedLifeCycles = mojoExecutions.stream()
+ .map(MojoExecution::getLifecyclePhase)
+ .toList();
+ mojoExecutions.addAll(mojoExecutionsFromLifecycle.stream()
+ .filter(e ->
!containedLifeCycles.contains(e.getLifecyclePhase()))
+ .toList());
Review Comment:
🔴 **Critical — wrong dedup semantics for reverse-order phases.**
The JIRA [MNG-5885](https://issues.apache.org/jira/browse/MNG-5885)
explicitly states:
> `package compile` is weird due to its order, but should **not** be
optimized.
But this code unconditionally filters out any mojo whose lifecycle phase is
already present — regardless of task order. So `mvn package compile` would
silently drop the second `compile` because the `package` task already added all
phases through `package` (which includes `compile`). The user explicitly asked
for `compile` to run again, and this code swallows that intent.
The correct approach would be to deduplicate only when the **later lifecycle
task's phases are a subset** of an earlier one (i.e., when the later phase
appears earlier in the lifecycle ordering than a phase already processed).
Alternatively, track which phases were contributed by each task and only
deduplicate within the same lifecycle when a later task's target phase is
higher in the lifecycle.
The concurrent executor (`BuildPlanExecutor`) handles this correctly via
`BuildPlan.then()` + merge semantics, where each task creates an independent
`BuildPlan` that is merged. Consider aligning the legacy executor with the same
approach.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java:
##########
@@ -234,7 +234,12 @@ public List<MojoExecution>
calculateMojoExecutions(MavenSession session, MavenPr
calculateLifecycleMappings(session, project,
lifecyclePhase);
for (List<MojoExecution> mojoExecutionsFromLifecycle :
phaseToMojoMapping.values()) {
- mojoExecutions.addAll(mojoExecutionsFromLifecycle);
+ List<String> containedLifeCycles = mojoExecutions.stream()
Review Comment:
💡 **Naming — `containedLifeCycles` is misleading.**
This variable holds lifecycle **phase names** (e.g. `compile`, `test`,
`package`), not lifecycle names (e.g. `default`, `clean`, `site`). Should be
named `containedPhases` or `processedPhases`. Also, `LifeCycles` uses
inconsistent camelCase — Maven codebase consistently uses `lifecycle`
(lowercase `c`).
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java:
##########
@@ -234,7 +234,12 @@ public List<MojoExecution>
calculateMojoExecutions(MavenSession session, MavenPr
calculateLifecycleMappings(session, project,
lifecyclePhase);
for (List<MojoExecution> mojoExecutionsFromLifecycle :
phaseToMojoMapping.values()) {
- mojoExecutions.addAll(mojoExecutionsFromLifecycle);
+ List<String> containedLifeCycles = mojoExecutions.stream()
+ .map(MojoExecution::getLifecyclePhase)
+ .toList();
Review Comment:
⚠️ **Performance — `containedLifeCycles` list rebuilt on every inner-loop
iteration.**
The `containedLifeCycles` list is rebuilt from scratch on every iteration of
the `phaseToMojoMapping.values()` loop. Since `mojoExecutions` only changes
between outer-loop iterations (between tasks), this list should be built
**once** before the inner `for` loop, not inside it.
Additionally, `List.contains()` is O(n). For a lifecycle with many phases
and plugins, this should be a `Set<String>` for O(1) lookups.
```suggestion
Set<String> containedPhases = mojoExecutions.stream()
.map(MojoExecution::getLifecyclePhase)
.collect(java.util.stream.Collectors.toSet());
```
(Moved **before** the `for` loop, and using a `Set` instead of a `List`.)
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/LifecycleExecutorTest.java:
##########
@@ -190,6 +190,46 @@ public void
testCalculationOfBuildPlanTasksOfTheCleanLifecycleAndTheInstallLifec
.toList());
}
+ // We need to take in multiple lifecycles
+ @Test
+ public void
testCalculationOfBuildPlanTasksOfTheVerifyLifecycleAndTheInstallLifecycle()
throws Exception {
+ File pom = getProject("project-with-additional-lifecycle-elements");
+ MavenSession session = createMavenSession(pom);
+ assertEquals(
+ "project-with-additional-lifecycle-elements",
+ session.getCurrentProject().getArtifactId());
+ assertEquals("1.0", session.getCurrentProject().getVersion());
+ List<MojoExecution> executionPlan =
+ getExecutions(calculateExecutionPlan(session, "clean",
"verify", "install"));
+
+ // [01] clean:clean
+ // [02] resources:resources
+ // [03] compiler:compile
+ // [04] it:generate-metadata
+ // [05] resources:testResources
+ // [06] compiler:testCompile
+ // [07] it:generate-test-metadata
+ // [08] surefire:test
+ // [09] jar:jar
+ // [10] install:install
+ //
+ assertListEquals(
+ List.of(
+ "clean:clean",
+ "resources:resources",
+ "compiler:compile",
+ "it:generate-metadata",
+ "resources:testResources",
+ "compiler:testCompile",
+ "it:generate-test-metadata",
+ "surefire:test",
+ "jar:jar",
+ "install:install"),
+ executionPlan.stream()
+ .map(plan ->
plan.getMojoDescriptor().getFullGoalName())
+ .toList());
+ }
+
// We need to take in multiple lifecycles
Review Comment:
⚠️ **Test coverage gap — doesn't test the actual MNG-5885 scenario.**
This test verifies that `clean verify install` deduplicates to the same plan
as `clean install`. That's a valid case, but it doesn't test:
1. **The primary JIRA scenario**: `mvn compile package` — two tasks from the
same lifecycle where the first is a strict subset. Does the plan equal `mvn
package`?
2. **The reverse-order case**: `mvn package compile` — per JIRA, this should
**not** be optimized (compile should still execute after package). Does the
current code handle this?
3. **The interleaved case**: `mvn compile resources:copy-resources package`
— per JIRA, this should run phases up to compile, then the goal, then
process-classes through package.
4. **GoalTask + LifecycleTask interaction**: `mvn compiler:compile package`
— the GoalTask has `null` lifecyclePhase. Does the null get into
`containedLifeCycles` and cause incorrect filtering?
Without these tests, the implementation can't be verified against the actual
requirements.
--
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]
