You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/10/16 08:15:03 UTC
[PR] fix: force reactor plugin projects to reach READY regardless of requested phase [maven]
gnodet opened a new pull request, #13559:
URL: https://github.com/apache/maven/pull/13559
## Summary
- Fix the concurrent builder to un-skip a reactor plugin project's `READY` phase (and its predecessors like `COMPILE`, `RESOURCES`) when another project in the reactor uses that plugin, even if the user requested a phase before `compile` (e.g. `initialize`)
- Re-enable the MNG-1088 integration test with `mvn initialize -b concurrent`, preserving the original test's intent of invoking a phase before "compile"
## Background
[MNG-1088](https://issues.apache.org/jira/browse/MNG-1088) covers the case where a plugin is built and used within the same multiproject build. The IT was disabled for [MNG-7977](https://issues.apache.org/jira/browse/MNG-7977).
PR #13222 re-enables this IT but changes the requested phase from `initialize` to `package`, which breaks the spirit of the test — the original comment says _"It's essential part of the test to invoke a phase before compile"_.
## The problem
In `BuildPlanExecutor.buildInitialPlan()`, reactor plugin detection adds an ordering dependency: `consumer.PLAN → pluginProject.READY`. But when the requested phase is `initialize`, `calculateLifecycleMappings` marks all steps after `initialize` as **skipped** (including `COMPILE`, `RESOURCES`, `READY`). The READY step cascades through as empty `EXECUTED` (no mojos), so the consumer's PLAN step runs — but the plugin JAR was never built, and plugin resolution fails.
## The fix
After establishing the ordering dependency, un-skip the plugin project's READY step and all its same-project predecessors:
```java
readyStep.allPredecessors()
.filter(s -> s.project == pluginProject)
.forEach(s -> s.skip.set(false));
```
Since mojos are added later during the PLAN step (via `addMojo()` which checks `!skip.get()`), clearing the skip flag is sufficient — the plugin project's `COMPILE` and `RESOURCES` mojos will be added and executed normally.
## Test plan
- [ ] `MavenITmng1088ReactorPluginResolutionTest` passes with `mvn initialize -b concurrent`
- [ ] Existing reactor plugin tests still pass
- [ ] CI green
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: force reactor plugin projects to reach READY regardless of requested phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13559:
URL: https://github.com/apache/maven/pull/13559#discussion_r4294281626
##########
its/core-it-suite/src/test/resources/mng-1088/plugin/pom.xml:
##########
@@ -57,17 +48,15 @@ under the License.
</dependencies>
<build>
- <resources>
- <resource>
- <directory>.</directory>
- <includes>
- <include>pom.xml</include>
- <include>src/**</include>
- </includes>
- </resource>
- <resource>
- <directory>src/main/resources</directory>
- </resource>
- </resources>
+ <plugins>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-plugin-plugin</artifactId>
+ <version>4.0.0-beta-1</version>
Review Comment:
⚠️ Hardcoded `maven-plugin-plugin:4.0.0-beta-1` will rot. Other ITs in this suite (e.g. `mng-8525-maven-di-plugin`) extract this to a local property. Add a `<properties>` entry and reference it here:
```suggestion
<version>${mavenPluginPluginVersion}</version>
```
Also add `<mavenPluginPluginVersion>4.0.0-beta-1</mavenPluginPluginVersion>` (or the current GA) to the POM's `<properties>` block.
--
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: force reactor plugin projects to reach READY regardless of requested phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13559:
URL: https://github.com/apache/maven/pull/13559#discussion_r4315754947
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution mojoExecution) {}
pred -> assertTrue(plan.step(pred.project, pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g. "initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
+
+ // The READY step of the plugin project (p2) must NOT be skipped, even though
+ // "initialize" is before "compile" and most of p2's lifecycle steps would normally be skipped.
+ assertFalse(
+ plan.requiredStep(p2, "READY").skip.get(),
Review Comment:
❌ **Wrong step name — test will throw `NoSuchElementException`.** `Lifecycle.Phase.READY` has the value `"ready"` (lowercase). Step names are keyed by `phase.name()` which returns lowercase phase names. `requiredStep(p2, "READY")` will throw `NoSuchElementException`, so the assertion never executes and the test is silently broken.
Fix: use the constant directly.
```suggestion
plan.requiredStep(p2, READY).skip.get(),
```
And add `import static org.apache.maven.api.Lifecycle.Phase.READY;` at the top of the test file (it's already imported in `BuildPlanExecutor`).
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution mojoExecution) {}
pred -> assertTrue(plan.step(pred.project, pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g. "initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
Review Comment:
❌ **Test calls the wrong entry point — the skip-clearing fix is never exercised.** The reactor plugin skip-clearing logic (`readyStep.allPredecessors().filter(...).forEach(s -> s.skip.set(false))`) lives in `buildInitialPlan` (lines 278-294 of `BuildPlanExecutor`), not in `calculateLifecycleMappings`. This test calls `calculateLifecycleMappings` directly, which only builds the phase graph and marks steps beyond the end phase as skipped — it never runs the reactor plugin handling code.
Even with the step name fixed, `plan.requiredStep(p2, READY).skip.get()` would return `true` (the step IS skipped by `calculateLifecycleMappings` since `initialize` is before `compile`), and `assertFalse` would fail — not because the fix is broken, but because the test never called the code that clears the skip flag.
The test needs to also invoke the reactor plugin handling from `buildInitialPlan`. The simplest fix is to extract that loop into a package-private helper on `BuildContext` and call it from the test after `calculateLifecycleMappings`.
--
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: force reactor plugin projects to reach READY regardless of requested phase [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13559:
URL: https://github.com/apache/maven/pull/13559#discussion_r4315710256
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution mojoExecution) {}
pred -> assertTrue(plan.step(pred.project, pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g. "initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
+
+ // The READY step of the plugin project (p2) must NOT be skipped, even though
+ // "initialize" is before "compile" and most of p2's lifecycle steps would normally be skipped.
+ assertFalse(
+ plan.requiredStep(p2, "READY").skip.get(),
Review Comment:
❌ **Wrong step name: `"READY"` (uppercase) does not exist** — the lifecycle phase is named `"ready"` (lowercase, from `Lifecycle.Phase.READY = "ready"`). `plan.requiredStep(p2, "READY")` will throw `NoSuchElementException` at runtime, so this test never reaches its assertions.
Use the constant or lowercase string:
```suggestion
plan.requiredStep(p2, Lifecycle.Phase.READY).skip.get(),
```
(Add `import static org.apache.maven.api.Lifecycle.Phase.READY;` or use the literal `"ready"`.
Same fix needed on line 509: `plan.requiredStep(p2, Lifecycle.Phase.READY)`.
##########
impl/maven-core/src/test/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanCreatorTest.java:
##########
@@ -466,4 +466,50 @@ protected void mojo(Consumer<String> writer, MojoExecution mojoExecution) {}
pred -> assertTrue(plan.step(pred.project, pred.name).isPresent(), "Phase not present: " + pred));
});
}
+
+ /**
+ * Verifies that when the requested phase is before "compile" (e.g. "initialize"),
+ * the new skip-clearing logic forces the reactor plugin project and all its
+ * predecessors to reach READY so that the plugin is available when needed.
+ */
+ @Test
+ void testReactorPluginPreCompilePhase() {
+ MavenProject p1 = new MavenProject();
+ p1.setGroupId("g");
+ p1.setArtifactId("p1");
+ p1.setVersion("1.0");
+ p1.setCollectedProjects(List.of());
+ Plugin plugin = new Plugin();
+ plugin.setGroupId("g");
+ plugin.setArtifactId("p2");
+ plugin.setVersion("1.0");
+ p1.getBuild().addPlugin(plugin);
+
+ MavenProject p2 = new MavenProject();
+ p2.setGroupId("g");
+ p2.setArtifactId("p2");
+ p2.setVersion("1.0");
+ p2.setCollectedProjects(List.of());
+
+ Map<MavenProject, List<MavenProject>> projects = new HashMap<>();
+ projects.put(p1, Collections.emptyList());
+ projects.put(p2, Collections.singletonList(p1));
+
+ // Use a phase before "compile" — this is the path that triggers the skip-clearing logic
+ BuildPlan plan = calculateLifecycleMappings(projects, "initialize");
Review Comment:
❌ **Test calls the wrong method — the skip-clearing fix is unreachable from `calculateLifecycleMappings`.** The fix that clears `skip` on reactor plugin predecessors lives in `BuildPlanExecutor.buildInitialPlan()` (lines 278–290 of the executor). The test helper at line 227–233 constructs a `BuildContext` and calls `context.calculateLifecycleMappings(projects, phase)` directly, which **never executes the reactor-plugin detection block**. Even after fixing the `"READY"` / `"ready"` string, the assertion will fail because `calculateLifecycleMappings` marks all phases after `initialize` as skipped and the fix never runs to un-skip them.
The test must exercise `buildInitialPlan`. Two options:
1. **Preferred**: Refactor `testReactorPluginPreCompilePhase` as a `BuildContext.calculateLifecycleMappings` test that directly validates the **skip behaviour without the fix** (i.e. assert that READY is skipped — baseline), then add a separate test that exercises `buildInitialPlan` via a mocked `MavenSession` to verify the skip-clearing.
2. **Simpler**: Add a test that instantiates a `BuildContext`, builds the plan via `calculateLifecycleMappings`, then manually simulates what `buildInitialPlan` does — apply the reactor-plugin detection logic and assert the result. But this duplicates production logic in test code.
Until this is fixed, `testReactorPluginPreCompilePhase` does not protect the fix from regression.
--
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