[MNG-8244] Fix wrong phase order

apache · Dec 13, 2024 · 7be65f2 · 7be65f2
1 parent 874a0d5
commit 7be65f2
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 87 deletions.
diff --git a/api/maven-api-core/src/main/java/org/apache/maven/api/Lifecycle.java b/api/maven-api-core/src/main/java/org/apache/maven/api/Lifecycle.java
@@ -20,7 +20,6 @@
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Optional;
 import java.util.stream.Stream;
 
 import org.apache.maven.api.annotations.Experimental;
@@ -70,6 +69,15 @@ public interface Lifecycle extends ExtensibleEnum {
      */
     Collection<Phase> phases();
 
+    /**
+     * Collection of main phases for this lifecycle used with the Maven 3 builders.
+     * Those builders does not operate on a graph, but on the list and expect a slightly
+     * different ordering (mainly unit test being executed before packaging).
+     */
+    default Collection<Phase> v3phases() {
+        return phases();
+    }
+
     /**
      * Stream of phases containing all child phases recursively.
      */
@@ -82,14 +90,6 @@ default Stream<Phase> allPhases() {
      */
     Collection<Alias> aliases();
 
-    /**
-     * Pre-ordered list of phases.
-     * If not provided, a default order will be computed.
-     */
-    default Optional<List<String>> orderedPhases() {
-        return Optional.empty();
-    }
-
     /**
      * A phase in the lifecycle.
      *

diff --git a/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java b/impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java
@@ -24,7 +24,6 @@
 import javax.inject.Singleton;
 
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
@@ -50,6 +49,8 @@
 import org.codehaus.plexus.PlexusContainer;
 import org.codehaus.plexus.component.repository.exception.ComponentLookupException;
 
+import static org.apache.maven.api.Lifecycle.AFTER;
+import static org.apache.maven.api.Lifecycle.BEFORE;
 import static org.apache.maven.api.Lifecycle.Phase.ALL;
 import static org.apache.maven.api.Lifecycle.Phase.BUILD;
 import static org.apache.maven.api.Lifecycle.Phase.COMPILE;
@@ -130,34 +131,20 @@ public Optional<Lifecycle> lookup(String id) {
 
     public List<String> computePhases(Lifecycle lifecycle) {
         Graph graph = new Graph();
-        lifecycle.phases().forEach(phase -> addPhase(graph, null, null, phase));
+        lifecycle.v3phases().forEach(phase -> addPhase(graph, null, null, phase));
         List<String> allPhases = graph.visitAll();
         Collections.reverse(allPhases);
         List<String> computed =
                 allPhases.stream().filter(s -> !s.startsWith("$")).collect(Collectors.toList());
-        List<String> given = lifecycle.orderedPhases().orElse(null);
-        if (given != null) {
-            if (given.size() != computed.size()) {
-                Set<String> s1 =
-                        given.stream().filter(s -> !computed.contains(s)).collect(Collectors.toSet());
-                Set<String> s2 =
-                        computed.stream().filter(s -> !given.contains(s)).collect(Collectors.toSet());
-                throw new IllegalStateException(
-                        "List of phases differ in size: expected " + computed.size() + ", but received " + given.size()
-                                + (s1.isEmpty() ? "" : ", missing " + s1)
-                                + (s2.isEmpty() ? "" : ", unexpected " + s2));
-            }
-            return given;
-        }
         return computed;
     }
 
     private static void addPhase(
             Graph graph, Graph.Vertex before, Graph.Vertex after, org.apache.maven.api.Lifecycle.Phase phase) {
-        Graph.Vertex ep0 = graph.addVertex("$" + phase.name());
+        Graph.Vertex ep0 = graph.addVertex(BEFORE + phase.name());
         Graph.Vertex ep1 = graph.addVertex("$$" + phase.name());
         Graph.Vertex ep2 = graph.addVertex(phase.name());
-        Graph.Vertex ep3 = graph.addVertex("$$$" + phase.name());
+        Graph.Vertex ep3 = graph.addVertex(AFTER + phase.name());
         graph.addEdge(ep0, ep1);
         graph.addEdge(ep1, ep2);
         graph.addEdge(ep2, ep3);
@@ -172,11 +159,23 @@ private static void addPhase(
                 if (link.kind() == Lifecycle.Link.Kind.AFTER) {
                     graph.addEdge(graph.addVertex(link.pointer().phase()), ep0);
                 } else {
-                    graph.addEdge(ep3, graph.addVertex("$" + link.pointer().phase()));
+                    graph.addEdge(ep3, graph.addVertex(BEFORE + link.pointer().phase()));
                 }
             }
         });
-        phase.phases().forEach(child -> addPhase(graph, ep1, ep2, child));
+        // We add ordering between internal phases.
+        // This would be wrong at execution time, but we are here computing a list and not a graph,
+        // so in order to obtain the expected order, we add these links between phases.
+        Lifecycle.Phase prev = null;
+        for (Lifecycle.Phase child : phase.phases()) {
+            // add phase
+            addPhase(graph, ep1, ep2, child);
+            if (prev != null) {
+                // add link between end of previous phase and beginning of this one
+                graph.addEdge(graph.addVertex(AFTER + prev.name()), graph.addVertex(BEFORE + child.name()));
+            }
+            prev = child;
+        }
     }
 
     @Named
@@ -408,6 +407,29 @@ public Collection<Phase> phases() {
                     phase(DEPLOY, after(PACKAGE))));
         }
 
+        @Override
+        public Collection<Phase> v3phases() {
+            return List.of(phase(
+                    ALL,
+                    phase(VALIDATE),
+                    phase(INITIALIZE),
+                    phase(SOURCES),
+                    phase(RESOURCES),
+                    phase(COMPILE),
+                    phase(READY),
+                    phase(TEST_SOURCES),
+                    phase(TEST_RESOURCES),
+                    phase(TEST_COMPILE),
+                    phase(TEST),
+                    phase(UNIT_TEST),
+                    phase(PACKAGE),
+                    phase(BUILD),
+                    phase(INTEGRATION_TEST),
+                    phase(VERIFY),
+                    phase(INSTALL),
+                    phase(DEPLOY)));
+        }
+
         @Override
         public Collection<Alias> aliases() {
             return List.of(
@@ -425,42 +447,6 @@ public Collection<Alias> aliases() {
                     alias("pre-integration-test", BEFORE + INTEGRATION_TEST),
                     alias("post-integration-test", AFTER + INTEGRATION_TEST));
         }
-
-        @Override
-        public Optional<List<String>> orderedPhases() {
-            return Optional.of(Arrays.asList(
-                    VALIDATE,
-                    INITIALIZE,
-                    // "generate-sources",
-                    SOURCES,
-                    // "process-sources",
-                    // "generate-resources",
-                    RESOURCES,
-                    // "process-resources",
-                    COMPILE,
-                    // "process-classes",
-                    READY,
-                    // "generate-test-sources",
-                    TEST_SOURCES,
-                    // "process-test-sources",
-                    // "generate-test-resources",
-                    TEST_RESOURCES,
-                    // "process-test-resources",
-                    TEST_COMPILE,
-                    // "process-test-classes",
-                    TEST,
-                    UNIT_TEST,
-                    // "prepare-package",
-                    PACKAGE,
-                    BUILD,
-                    // "pre-integration-test",
-                    INTEGRATION_TEST,
-                    // "post-integration-test",
-                    VERIFY,
-                    INSTALL,
-                    DEPLOY,
-                    ALL));
-        }
     }
 
     static class SiteLifecycle implements Lifecycle {

diff --git a/impl/maven-core/src/main/java/org/apache/maven/lifecycle/Lifecycle.java b/impl/maven-core/src/main/java/org/apache/maven/lifecycle/Lifecycle.java
@@ -23,13 +23,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import org.apache.maven.lifecycle.mapping.LifecyclePhase;
 
-import static org.apache.maven.api.Lifecycle.AFTER;
-import static org.apache.maven.api.Lifecycle.BEFORE;
-
 /**
  * Lifecycle definition, with eventual plugin bindings (when they are not packaging-specific).
  */
@@ -46,9 +42,7 @@ public Lifecycle(
             org.apache.maven.api.services.LifecycleRegistry registry, org.apache.maven.api.Lifecycle lifecycle) {
         this.lifecycle = lifecycle;
         this.id = lifecycle.id();
-        this.phases = registry.computePhases(lifecycle).stream()
-                .flatMap(p -> Stream.of(BEFORE + p, p, AFTER + p))
-                .toList();
+        this.phases = registry.computePhases(lifecycle);
         this.defaultPhases = getDefaultPhases(lifecycle);
     }
 

diff --git a/...ain/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java b/...ain/java/org/apache/maven/lifecycle/internal/DefaultLifecycleExecutionPlanCalculator.java
@@ -25,7 +25,6 @@
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -264,13 +263,10 @@ private Map<String, List<MojoExecution>> calculateLifecycleMappings(
         }
 
         LifecycleMappingDelegate delegate;
-        if (Arrays.binarySearch(DefaultLifecycles.STANDARD_LIFECYCLES, lifecycle.getId()) >= 0) {
+        if (List.of(DefaultLifecycles.STANDARD_LIFECYCLES).contains(lifecycle.getId())) {
             delegate = standardDelegate;
         } else {
-            delegate = delegates.get(lifecycle.getId());
-            if (delegate == null) {
-                delegate = standardDelegate;
-            }
+            delegate = delegates.getOrDefault(lifecycle.getId(), standardDelegate);
         }
 
         return delegate.calculateLifecycleMappings(session, project, lifecycle, lifecyclePhase);

diff --git a/...re/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleMappingDelegate.java b/...re/src/main/java/org/apache/maven/lifecycle/internal/DefaultLifecycleMappingDelegate.java
@@ -82,14 +82,23 @@ public Map<String, List<MojoExecution>> calculateLifecycleMappings(
             lifecyclePhase = PhaseId.of(aliases.get(lifecyclePhase)).phase();
         }
 
+        boolean passed = false;
         for (String phase : lifecycle.getPhases()) {
-            Map<PhaseId, List<MojoExecution>> phaseBindings =
-                    new TreeMap<>(Comparator.comparing(PhaseId::toString, new PhaseComparator(lifecycle.getPhases())));
-
-            mappings.put(phase, phaseBindings);
-
+            boolean include = true;
             if (phase.equals(lifecyclePhase)) {
-                break;
+                passed = true;
+            } else if (passed) {
+                if (phase.startsWith(org.apache.maven.api.Lifecycle.AFTER)) {
+                    String realPhase = phase.substring(org.apache.maven.api.Lifecycle.AFTER.length());
+                    include = mappings.containsKey(org.apache.maven.api.Lifecycle.BEFORE + realPhase);
+                } else {
+                    include = false;
+                }
+            }
+            if (include) {
+                Map<PhaseId, List<MojoExecution>> phaseBindings = new TreeMap<>(
+                        Comparator.comparing(PhaseId::toString, new PhaseComparator(lifecycle.getPhases())));
+                mappings.put(phase, phaseBindings);
             }
         }
 

diff --git a/...-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java b/...-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/BuildPlanExecutor.java
@@ -683,9 +683,7 @@ public BuildPlan calculateLifecycleMappings(
                                 + " or a goal in the format <plugin-prefix>:<goal> or"
                                 + " <plugin-group-id>:<plugin-artifact-id>[:<plugin-version>]:<goal>. Available lifecycle phases are: "
                                 + lifecycles.stream()
-                                        .flatMap(l -> l.orderedPhases()
-                                                .map(List::stream)
-                                                .orElseGet(() -> l.allPhases().map(Lifecycle.Phase::name)))
+                                        .flatMap(l -> l.allPhases().map(Lifecycle.Phase::name))
                                         .collect(Collectors.joining(", "))
                                 + ".",
                         lifecyclePhase));
@@ -738,6 +736,10 @@ public BuildPlan calculateLifecycleMappings(
                                 ? lifecyclePhase.substring(AT.length())
                                 : AFTER + lifecyclePhase;
                 Set<BuildStep> toKeep = steps.get(endPhase).allPredecessors().collect(Collectors.toSet());
+                toKeep.addAll(toKeep.stream()
+                        .filter(s -> s.name.startsWith(BEFORE))
+                        .map(s -> steps.get(AFTER + s.name.substring(BEFORE.length())))
+                        .toList());
                 steps.values().stream().filter(n -> !toKeep.contains(n)).forEach(BuildStep::skip);
 
                 plan.addProject(project, steps);