Copilot commented on code in PR #13798:
URL: https://github.com/apache/maven/pull/13798#discussion_r4453278939
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +716,104 @@ private void plan() {
}
}
+ void linkAggregatorSteps() {
+ // Planning can be deferred for reactor plugins. Revisit
completion links after
+ // each planning pass so newly planned consumers and aggregators
are included.
+ for (BuildStep aggregator : plan.allSteps()
+ .filter(step -> step.name.startsWith("$aggregator:"))
+ .toList()) {
+ BuildStep phaseEnd = plan.requiredStep(aggregator.project,
AFTER + aggregator.phase.name());
+ List<MavenProject> children =
aggregator.project.getCollectedProjects();
+ for (BuildStep successor : List.copyOf(phaseEnd.successors)) {
+ // Collected projects may need the parent's ordinary phase
outputs in
+ // order to build the very outputs that its aggregator
consumes.
+ if (successor != aggregator && (children == null ||
!children.contains(successor.project))) {
+ successor.executeAfter(aggregator);
+ }
+ }
+ }
+ }
+
+ private boolean inheritedFromReactorAncestor(
+ MavenProject project, Plugin plugin, PluginExecution
execution, String goal, String phase) {
+ if (declaredInProject(project, plugin, execution, goal)) {
+ return false;
+ }
+ for (MavenProject parent = project.getParent(); parent != null;
parent = parent.getParent()) {
+ for (Plugin parentPlugin : parent.getBuild().getPlugins()) {
+ if (!parentPlugin.getKey().equals(plugin.getKey())) {
+ continue;
+ }
+ if (!parentPlugin.isInherited()) {
+ return false;
+ }
+ for (PluginExecution parentExecution :
parentPlugin.getExecutions()) {
+ if
(!parentExecution.getId().equals(execution.getId())) {
+ continue;
+ }
+ if (!parentExecution.isInherited()) {
+ return false;
+ }
+ if (parentExecution.getGoals().contains(goal)
+ && Objects.equals(parentExecution.getPhase(),
execution.getPhase())
+ && plan.getAllProjects().containsKey(parent)
+ && plan.step(parent, phase)
+ .filter(step -> !step.skip.get())
+ .isPresent()) {
+ return true;
+ }
Review Comment:
A reactor POM parent need not be the project's module aggregator: the root
can list the parent POM and its inheriting child as sibling modules. This
suppresses the child's copy even though the parent's aggregation step has no
dependency on that child's phase completion. Resolve the matching reactor
project and suppress only when it actually collects the child.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -642,14 +645,44 @@ private void plan() {
plan.step(project, resolvedPhase).ifPresent(n
-> {
MojoExecution mojoExecution = new
MojoExecution(mojoDescriptor, execution.getId());
mojoExecution.setLifecyclePhase(phase);
- n.addMojo(mojoExecution,
execution.getPriority());
+ BuildStep target = n;
+ if (mojoDescriptor.isAggregator()
+ &&
!resolvedPhase.startsWith(BEFORE)
+ &&
!resolvedPhase.startsWith(AFTER)) {
Review Comment:
This condition also excludes standard lifecycle phases that are translated
into hook steps: `prepare-package` resolves to `before:package`, and
`process-classes` resolves to `after:compile` in `DefaultLifecycleRegistry`. An
aggregator bound to either phase still goes into the original per-project step,
bypassing both child-completion ordering and inherited-execution suppression.
Handle these legacy phase aliases when creating the aggregation barrier as
well, so ordinary lifecycle-bound aggregators do not retain the original
ordering bug.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -642,14 +645,44 @@ private void plan() {
plan.step(project, resolvedPhase).ifPresent(n
-> {
MojoExecution mojoExecution = new
MojoExecution(mojoDescriptor, execution.getId());
mojoExecution.setLifecyclePhase(phase);
- n.addMojo(mojoExecution,
execution.getPriority());
+ BuildStep target = n;
+ if (mojoDescriptor.isAggregator()
+ &&
!resolvedPhase.startsWith(BEFORE)
+ &&
!resolvedPhase.startsWith(AFTER)) {
+ // Only suppress inherited copies when
their declaring ancestor is
+ // part of the reactor; partial builds
still run the selected goal.
+ if (inheritedFromReactorAncestor(
+ project, plugin, execution,
goal, resolvedPhase)) {
+ return;
+ }
+ // An aggregator needs a normal step:
after:* steps also run for
+ // failure cleanup, even if a child
failed.
+ if (!n.skip.get()) {
+ String name = "$aggregator:" +
resolvedPhase;
+ target = plan.step(project,
name).orElseGet(() -> {
+ BuildStep aggregator = new
BuildStep(name, project, n.phase);
+ BuildStep phaseEnd =
plan.requiredStep(project, AFTER + resolvedPhase);
+
aggregator.executeAfter(phaseEnd);
+ if
(project.getCollectedProjects() != null) {
+ for (MavenProject child :
project.getCollectedProjects()) {
+ plan.step(child, AFTER
+ resolvedPhase)
+
.ifPresent(aggregator::executeAfter);
Review Comment:
With `--fail-at-end`, a `verify` aggregator can still run after a child's
`test` goal fails. `processStep()` marks the failed phase's completion as
`SKIPPED`, but enclosing `after:*` steps can become `EXECUTED` again,
satisfying this dependency. Gate aggregation on successful child work while
preserving cleanup, and add a nested-phase failure test.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -642,14 +645,44 @@ private void plan() {
plan.step(project, resolvedPhase).ifPresent(n
-> {
MojoExecution mojoExecution = new
MojoExecution(mojoDescriptor, execution.getId());
mojoExecution.setLifecyclePhase(phase);
- n.addMojo(mojoExecution,
execution.getPriority());
+ BuildStep target = n;
+ if (mojoDescriptor.isAggregator()
+ &&
!resolvedPhase.startsWith(BEFORE)
+ &&
!resolvedPhase.startsWith(AFTER)) {
+ // Only suppress inherited copies when
their declaring ancestor is
+ // part of the reactor; partial builds
still run the selected goal.
+ if (inheritedFromReactorAncestor(
+ project, plugin, execution,
goal, resolvedPhase)) {
+ return;
+ }
+ // An aggregator needs a normal step:
after:* steps also run for
+ // failure cleanup, even if a child
failed.
+ if (!n.skip.get()) {
+ String name = "$aggregator:" +
resolvedPhase;
+ target = plan.step(project,
name).orElseGet(() -> {
+ BuildStep aggregator = new
BuildStep(name, project, n.phase);
+ BuildStep phaseEnd =
plan.requiredStep(project, AFTER + resolvedPhase);
+
aggregator.executeAfter(phaseEnd);
Review Comment:
Moving every aggregator behind the entire phase reverses the order of goals
bound to that phase. For example, if an aggregator attaches an artifact and a
later package-bound goal consumes or signs it, the consumer now runs first,
even for a single-project build and regardless of execution priority. Preserve
the configured same-phase goal ordering while inserting the child-phase barrier
before the aggregator.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +716,104 @@ private void plan() {
}
}
+ void linkAggregatorSteps() {
+ // Planning can be deferred for reactor plugins. Revisit
completion links after
+ // each planning pass so newly planned consumers and aggregators
are included.
+ for (BuildStep aggregator : plan.allSteps()
+ .filter(step -> step.name.startsWith("$aggregator:"))
+ .toList()) {
+ BuildStep phaseEnd = plan.requiredStep(aggregator.project,
AFTER + aggregator.phase.name());
+ List<MavenProject> children =
aggregator.project.getCollectedProjects();
+ for (BuildStep successor : List.copyOf(phaseEnd.successors)) {
+ // Collected projects may need the parent's ordinary phase
outputs in
+ // order to build the very outputs that its aggregator
consumes.
+ if (successor != aggregator && (children == null ||
!children.contains(successor.project))) {
+ successor.executeAfter(aggregator);
Review Comment:
`linkAggregatorSteps()` only visits direct successors, so consumers behind
an elided dependency edge can run before the aggregate is attached. It also
loses the barrier when a direct edge would cycle—for example, a parent's
`package` can overtake its `compile` aggregator when child compilation needs
the parent's `ready` output. Recover the logical dependencies and attach the
barrier to non-cyclic downstream steps, with tests for both cases.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +716,104 @@ private void plan() {
}
}
+ void linkAggregatorSteps() {
+ // Planning can be deferred for reactor plugins. Revisit
completion links after
+ // each planning pass so newly planned consumers and aggregators
are included.
+ for (BuildStep aggregator : plan.allSteps()
+ .filter(step -> step.name.startsWith("$aggregator:"))
+ .toList()) {
+ BuildStep phaseEnd = plan.requiredStep(aggregator.project,
AFTER + aggregator.phase.name());
+ List<MavenProject> children =
aggregator.project.getCollectedProjects();
+ for (BuildStep successor : List.copyOf(phaseEnd.successors)) {
+ // Collected projects may need the parent's ordinary phase
outputs in
+ // order to build the very outputs that its aggregator
consumes.
+ if (successor != aggregator && (children == null ||
!children.contains(successor.project))) {
+ successor.executeAfter(aggregator);
+ }
+ }
+ }
+ }
+
+ private boolean inheritedFromReactorAncestor(
+ MavenProject project, Plugin plugin, PluginExecution
execution, String goal, String phase) {
+ if (declaredInProject(project, plugin, execution, goal)) {
+ return false;
+ }
+ for (MavenProject parent = project.getParent(); parent != null;
parent = parent.getParent()) {
+ for (Plugin parentPlugin : parent.getBuild().getPlugins()) {
+ if (!parentPlugin.getKey().equals(plugin.getKey())) {
+ continue;
+ }
+ if (!parentPlugin.isInherited()) {
+ return false;
+ }
Review Comment:
An execution-level `<inherited>true</inherited>` overrides a plugin-level
`<inherited>false</inherited>` in `MavenModelMerger.mergePlugin_Executions`.
Such an aggregator execution really is inherited by the child, but this early
return treats the plugin flag as an unconditional barrier and lets the
inherited copy run again in every child. Honor an explicit inheritance override
on the matching execution before applying the plugin-level default.
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java:
##########
@@ -681,6 +716,104 @@ private void plan() {
}
}
+ void linkAggregatorSteps() {
+ // Planning can be deferred for reactor plugins. Revisit
completion links after
+ // each planning pass so newly planned consumers and aggregators
are included.
+ for (BuildStep aggregator : plan.allSteps()
+ .filter(step -> step.name.startsWith("$aggregator:"))
+ .toList()) {
+ BuildStep phaseEnd = plan.requiredStep(aggregator.project,
AFTER + aggregator.phase.name());
+ List<MavenProject> children =
aggregator.project.getCollectedProjects();
+ for (BuildStep successor : List.copyOf(phaseEnd.successors)) {
+ // Collected projects may need the parent's ordinary phase
outputs in
+ // order to build the very outputs that its aggregator
consumes.
+ if (successor != aggregator && (children == null ||
!children.contains(successor.project))) {
+ successor.executeAfter(aggregator);
+ }
+ }
+ }
+ }
+
+ private boolean inheritedFromReactorAncestor(
+ MavenProject project, Plugin plugin, PluginExecution
execution, String goal, String phase) {
+ if (declaredInProject(project, plugin, execution, goal)) {
+ return false;
+ }
+ for (MavenProject parent = project.getParent(); parent != null;
parent = parent.getParent()) {
+ for (Plugin parentPlugin : parent.getBuild().getPlugins()) {
+ if (!parentPlugin.getKey().equals(plugin.getKey())) {
+ continue;
+ }
+ if (!parentPlugin.isInherited()) {
+ return false;
+ }
+ for (PluginExecution parentExecution :
parentPlugin.getExecutions()) {
+ if
(!parentExecution.getId().equals(execution.getId())) {
+ continue;
+ }
+ if (!parentExecution.isInherited()) {
+ return false;
+ }
+ if (parentExecution.getGoals().contains(goal)
+ && Objects.equals(parentExecution.getPhase(),
execution.getPhase())
Review Comment:
When a child changes only the plugin version, the same goal can have a
different aggregator flag or default phase. This check compares only the plugin
key and raw phase, so the parent can run a non-aggregating goal, or no goal at
that phase, while the child's only aggregator is dropped. Require matching
plugin versions, or verify the ancestor's resolved mojo and phase, before
suppressing the child.
--
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]
