gnodet-bot commented on code in PR #13544:
URL: https://github.com/apache/maven/pull/13544#discussion_r4282888027
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlan.java:
##########
@@ -96,22 +96,86 @@ public void then(BuildPlan step) {
}
private Map<String, BuildStep> merge(Map<String, BuildStep> org,
Map<String, BuildStep> add) {
- // all new phases should be added after the existing ones
- List<BuildStep> lasts =
- org.values().stream().filter(b ->
b.successors.isEmpty()).toList();
- List<BuildStep> firsts =
- add.values().stream().filter(b ->
b.predecessors.isEmpty()).toList();
- firsts.stream()
- .filter(addNode -> !org.containsKey(addNode.name))
- .forEach(addNode -> lasts.forEach(orgNode ->
addNode.executeAfter(orgNode)));
- add.forEach((name, node) -> org.merge(name, node, this::merge));
+ // When add contains infrastructure steps ($plan$, $setup$,
$teardown$), this merge
+ // is from computeForkPlan (execution plan merged into a fork). In
that case, use the
+ // original terminal-step chaining to avoid creating cycles with the
frontier approach.
+ boolean isSegmentMerge = !add.containsKey(BuildStep.PLAN);
Review Comment:
💡 **Nit:** The comment mentions `$plan$, $setup$, $teardown$` but only
`$plan$` is checked. They are always added together so this is functionally
correct, but the comment overstates the check.
```suggestion
// When add contains the infrastructure $plan$ step, this merge is
from
// computeForkPlan (execution plan merged into a fork). In that
case, use the
// original terminal-step chaining to avoid creating cycles with the
frontier approach.
boolean isSegmentMerge = !add.containsKey(BuildStep.PLAN);
```
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlan.java:
##########
@@ -96,22 +96,86 @@ public void then(BuildPlan step) {
}
private Map<String, BuildStep> merge(Map<String, BuildStep> org,
Map<String, BuildStep> add) {
- // all new phases should be added after the existing ones
- List<BuildStep> lasts =
- org.values().stream().filter(b ->
b.successors.isEmpty()).toList();
- List<BuildStep> firsts =
- add.values().stream().filter(b ->
b.predecessors.isEmpty()).toList();
- firsts.stream()
- .filter(addNode -> !org.containsKey(addNode.name))
- .forEach(addNode -> lasts.forEach(orgNode ->
addNode.executeAfter(orgNode)));
- add.forEach((name, node) -> org.merge(name, node, this::merge));
+ // When add contains infrastructure steps ($plan$, $setup$,
$teardown$), this merge
+ // is from computeForkPlan (execution plan merged into a fork). In
that case, use the
+ // original terminal-step chaining to avoid creating cycles with the
frontier approach.
+ boolean isSegmentMerge = !add.containsKey(BuildStep.PLAN);
+
+ if (isSegmentMerge) {
+ // Track which steps are currently skipped before the merge
+ Set<String> skippedInOrg = org.entrySet().stream()
+ .filter(e -> e.getValue().skip.get())
+ .map(Map.Entry::getKey)
+ .collect(Collectors.toSet());
+
+ // The "active frontier" consists of non-skipped steps that have
at least one
+ // skipped same-project successor. This is the boundary between
the active and
+ // skipped regions — the right insertion point for new
non-lifecycle steps
+ // (e.g., aggregator goals). Using the frontier avoids creating
transitive
+ // dependencies on all lifecycle phases through wrapper steps like
after:ALL.
+ List<BuildStep> activeFrontier = org.values().stream()
+ .filter(b -> !b.skip.get())
+ .filter(b -> b.successors.stream().anyMatch(s -> s.project
== b.project && s.skip.get()))
+ .toList();
+ if (activeFrontier.isEmpty()) {
+ activeFrontier = org.values().stream()
+ .filter(b -> b.successors.isEmpty())
+ .toList();
+ }
Review Comment:
💡 **Nit:** The primary frontier computation filters `!b.skip.get()`, but
this fallback doesn't. In the degenerate case where all steps are skipped, it
would pick skipped terminal steps as insertion points. Consider filtering for
consistency:
```suggestion
if (activeFrontier.isEmpty()) {
activeFrontier = org.values().stream()
.filter(b -> !b.skip.get())
.filter(b -> b.successors.isEmpty())
.toList();
}
```
--
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]
