Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MNG-8123] Fix Lifecycle in v3 #1524

Merged
merged 3 commits into from
May 22, 2024
Merged

[MNG-8123] Fix Lifecycle in v3 #1524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32270c8
[MNG-8123] Fix Lifecycle in v3
gnodet May 17, 2024
5621b1d
Fix DefaultLifecycleRegistry so that lookups are lazy
gnodet May 21, 2024
96041bb
Fix the lookup method
gnodet May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions api/maven-api-core/src/main/java/org/apache/maven/api/services/LifecycleRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,14 @@
*/
package org.apache.maven.api.services;

import java.util.List;
import java.util.stream.Stream;
import java.util.stream.StreamSupport;

import org.apache.maven.api.Lifecycle;

public interface LifecycleRegistry extends ExtensibleEnumRegistry<Lifecycle>, Iterable<Lifecycle> {

default Stream<Lifecycle> stream() {
return StreamSupport.stream(spliterator(), false);
}

List<String> computePhases(Lifecycle lifecycle);
}
4 changes: 0 additions & 4 deletions maven-api-impl/src/test/java/org/apache/maven/internal/impl/standalone/ApiRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,10 +311,6 @@ public Optional<Type> lookup(String id) {
@Provides
static LifecycleRegistry newLifecycleRegistry() {
return new LifecycleRegistry() {
@Override
public List<String> computePhases(Lifecycle lifecycle) {
return List.of();
}

@Override
public Iterator<Lifecycle> iterator() {
Expand Down
9 changes: 0 additions & 9 deletions maven-compat/src/test/java/org/apache/maven/project/EmptyLifecycleBindingsInjector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,6 @@ public class EmptyLifecycleBindingsInjector extends DefaultLifecycleBindingsInje
private static PackagingRegistry packagingRegistry;

private static final LifecycleRegistry emptyLifecycleRegistry = new LifecycleRegistry() {
@Override
public List<String> computePhases(Lifecycle lifecycle) {
return List.of();
}

@Override
public Iterator<Lifecycle> iterator() {
Expand Down Expand Up @@ -136,11 +132,6 @@ public Optional<Lifecycle> lookup(String id) {
return getDelegate().lookup(id);
}

@Override
public List<String> computePhases(Lifecycle lifecycle) {
return getDelegate().computePhases(lifecycle);
}

@Override
public Iterator<Lifecycle> iterator() {
return getDelegate().iterator();
Expand Down
150 changes: 120 additions & 30 deletions maven-core/src/main/java/org/apache/maven/internal/impl/DefaultLifecycleRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,32 @@

import javax.inject.Inject;
import javax.inject.Named;
import javax.inject.Provider;
import javax.inject.Singleton;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.maven.api.Lifecycle;
import org.apache.maven.api.model.Plugin;
import org.apache.maven.api.services.LifecycleRegistry;
import org.apache.maven.api.services.LookupException;
import org.apache.maven.api.spi.ExtensibleEnumProvider;
import org.apache.maven.api.spi.LifecycleProvider;
import org.apache.maven.lifecycle.mapping.LifecyclePhase;
import org.codehaus.plexus.PlexusContainer;
import org.codehaus.plexus.component.repository.exception.ComponentLookupException;

import static java.util.Arrays.asList;
import static java.util.Collections.singleton;
Expand All @@ -47,23 +57,19 @@
*/
@Named
@Singleton
public class DefaultLifecycleRegistry
extends ExtensibleEnumRegistries.DefaultExtensibleEnumRegistry<Lifecycle, LifecycleProvider>
implements LifecycleRegistry {
public class DefaultLifecycleRegistry implements LifecycleRegistry {

private final List<LifecycleProvider> providers;

public DefaultLifecycleRegistry() {
super(Collections.emptyList());
this(Collections.emptyList());
}

@Inject
public DefaultLifecycleRegistry(
List<LifecycleProvider> providers, Map<String, org.apache.maven.lifecycle.Lifecycle> lifecycles) {
super(
concat(providers, new LifecycleWrapperProvider(lifecycles)),
new CleanLifecycle(),
new DefaultLifecycle(),
new SiteLifecycle(),
new WrapperLifecycle());
public DefaultLifecycleRegistry(List<LifecycleProvider> providers) {
List<LifecycleProvider> p = new ArrayList<>(providers);
p.add(() -> List.of(new CleanLifecycle(), new DefaultLifecycle(), new SiteLifecycle(), new WrapperLifecycle()));
this.providers = p;
// validate lifecycle
for (Lifecycle lifecycle : this) {
Set<String> set = new HashSet<>();
Expand All @@ -78,37 +84,44 @@ public DefaultLifecycleRegistry(

@Override
public Iterator<Lifecycle> iterator() {
return values.values().iterator();
return stream().toList().iterator();
}

@Override
public Stream<Lifecycle> stream() {
return values.values().stream();
}

static <T> List<T> concat(List<T> l, T t) {
List<T> nl = new ArrayList<>(l.size() + 1);
nl.addAll(l);
nl.add(t);
return nl;
return providers.stream().map(ExtensibleEnumProvider::provides).flatMap(Collection::stream);
}

@Override
public List<String> computePhases(Lifecycle lifecycle) {
return lifecycle.phases().stream().map(Lifecycle.Phase::name).toList();
public Optional<Lifecycle> lookup(String id) {
return stream().filter(lf -> Objects.equals(id, lf.id())).findAny();
}

static class LifecycleWrapperProvider implements LifecycleProvider {
private final Map<String, org.apache.maven.lifecycle.Lifecycle> lifecycles;
@Named
@Singleton
public static class LifecycleWrapperProvider implements LifecycleProvider {
private final PlexusContainer container;

@Inject
LifecycleWrapperProvider(Map<String, org.apache.maven.lifecycle.Lifecycle> lifecycles) {
this.lifecycles = lifecycles;
public LifecycleWrapperProvider(PlexusContainer container) {
this.container = container;
}

@Override
public Collection<Lifecycle> provides() {
return lifecycles.values().stream().map(this::wrap).collect(Collectors.toList());
try {
Map<String, org.apache.maven.lifecycle.Lifecycle> all =
container.lookupMap(org.apache.maven.lifecycle.Lifecycle.class);
return all.keySet().stream()
.filter(id -> !Lifecycle.CLEAN.equals(id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this filter? How are these 4 lifecycles published? Or in other words, why distinguish these 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a kind of chicken/egg situation. Lifecycle can be published by maven 3 extensions, but lifecycles must also be provided through the Maven 3 API. We must avoid rewrapping in a kind of loop the default lifecycles exposed using the maven 4 API because it leads to a lookup error (we end-up with a bean creation cycle).

I can see if there's a better way, such as adding a new sub-interface WrappedLifecycle to better detect where the lifecycle come from (Maven 3 or 4 api and only wrap if for the other side).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I can't use the instance to detect, as getting the instance will cause the lookup error: the map is lazily managed by sisu, and the instance is created when requested only, but that's what causes the lookup error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this somewhat similar to Type / ArtifactHandler story? Is same problem present there as well?

&& !Lifecycle.DEFAULT.equals(id)
&& !Lifecycle.SITE.equals(id)
&& !Lifecycle.WRAPPER.equals(id))
.map(id -> wrap(all.get(id)))
.collect(Collectors.toList());
} catch (ComponentLookupException e) {
throw new LookupException(e);
}
}

private Lifecycle wrap(org.apache.maven.lifecycle.Lifecycle lifecycle) {
Expand All @@ -120,13 +133,90 @@ public String id() {

@Override
public Collection<Phase> phases() {
// TODO: implement
throw new UnsupportedOperationException();
return lifecycle.getPhases().stream()
.map(name -> (Phase) new Phase() {
@Override
public String name() {
return name;
}

@Override
public List<Plugin> plugins() {
Map<String, LifecyclePhase> lfPhases = lifecycle.getDefaultLifecyclePhases();
LifecyclePhase phase = lfPhases != null ? lfPhases.get(name) : null;
if (phase != null) {
Map<String, Plugin> plugins = new LinkedHashMap<>();
DefaultPackagingRegistry.parseLifecyclePhaseDefinitions(plugins, name, phase);
return plugins.values().stream().toList();
}
return List.of();
}
})
.toList();
}
};
}
}

static class WrappedLifecycle extends org.apache.maven.lifecycle.Lifecycle {
WrappedLifecycle(Lifecycle lifecycle) {
super(lifecycle);
}
}

abstract static class BaseLifecycleProvider implements Provider<org.apache.maven.lifecycle.Lifecycle> {
@Inject
private PlexusContainer lookup;

private final String name;

BaseLifecycleProvider(String name) {
this.name = name;
}

@Override
public org.apache.maven.lifecycle.Lifecycle get() {
try {
LifecycleRegistry registry = lookup.lookup(LifecycleRegistry.class);
return new WrappedLifecycle(registry.require(name));
} catch (ComponentLookupException e) {
throw new LookupException(e);
}
}
}

@Singleton
@Named(Lifecycle.CLEAN)
static class CleanLifecycleProvider extends BaseLifecycleProvider {
CleanLifecycleProvider() {
super(Lifecycle.CLEAN);
}
}

@Singleton
@Named(Lifecycle.DEFAULT)
static class DefaultLifecycleProvider extends BaseLifecycleProvider {
DefaultLifecycleProvider() {
super(Lifecycle.DEFAULT);
}
}

@Singleton
@Named(Lifecycle.SITE)
static class SiteLifecycleProvider extends BaseLifecycleProvider {
SiteLifecycleProvider() {
super(Lifecycle.SITE);
}
}

@Singleton
@Named(Lifecycle.WRAPPER)
static class WrapperLifecycleProvider extends BaseLifecycleProvider {
WrapperLifecycleProvider() {
super(Lifecycle.WRAPPER);
}
}

static class CleanLifecycle implements Lifecycle {

private static final String MAVEN_CLEAN_PLUGIN_VERSION = "3.2.0";
Expand Down
2 changes: 1 addition & 1 deletion maven-core/src/main/java/org/apache/maven/internal/impl/DefaultPackagingRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ private Map<String, PluginContainer> getPlugins(LifecycleMapping lifecycleMappin
return lfs;
}

private void parseLifecyclePhaseDefinitions(Map<String, Plugin> plugins, String phase, LifecyclePhase goals) {
static void parseLifecyclePhaseDefinitions(Map<String, Plugin> plugins, String phase, LifecyclePhase goals) {
InputSource inputSource =
new InputSource(DefaultLifecyclePluginAnalyzer.DEFAULTLIFECYCLEBINDINGS_MODELID, null);
InputLocation location = new InputLocation(-1, -1, inputSource, 0);
Expand Down
8 changes: 3 additions & 5 deletions maven-core/src/main/java/org/apache/maven/lifecycle/DefaultLifecycles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,9 @@ private Map<String, Lifecycle> lookupLifecycles() {

// Lifecycles cannot be cached as extensions might add custom lifecycles later in the execution.
try {
Map<String, Lifecycle> lifecycles = new HashMap<>(lookup.lookupMap(Lifecycle.class));
for (org.apache.maven.api.Lifecycle lf : registry) {
lifecycles.put(lf.id(), new Lifecycle(registry, lf));
}
return lifecycles;
return registry != null
? registry.stream().collect(Collectors.toMap(lf -> lf.id(), lf -> new Lifecycle(lf)))
: Map.of();
} catch (LookupException e) {
throw new IllegalStateException("Unable to lookup lifecycles from the plexus container", e);
}
Expand Down
7 changes: 4 additions & 3 deletions maven-core/src/main/java/org/apache/maven/lifecycle/Lifecycle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,12 @@ public Lifecycle(String id, List<String> phases, Map<String, LifecyclePhase> def
this.defaultPhases = defaultPhases;
}

public Lifecycle(
org.apache.maven.api.services.LifecycleRegistry registry, org.apache.maven.api.Lifecycle lifecycle) {
public Lifecycle(org.apache.maven.api.Lifecycle lifecycle) {
this.lifecycle = lifecycle;
this.id = lifecycle.id();
this.phases = registry.computePhases(lifecycle);
this.phases = lifecycle.phases().stream()
.map(org.apache.maven.api.Lifecycle.Phase::name)
.toList();
this.defaultPhases = getDefaultPhases(lifecycle);
}

Expand Down
2 changes: 1 addition & 1 deletion maven-core/src/main/java/org/apache/maven/lifecycle/internal/ExecutionPlanItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,6 @@ public Plugin getPlugin() {

@Override
public String toString() {
return "ExecutionPlanItem{" + ", mojoExecution=" + mojoExecution + '}' + super.toString();
return "ExecutionPlanItem{" + mojoExecution.toString() + "}@" + Integer.toHexString(hashCode());
}
}
3 changes: 2 additions & 1 deletion maven-core/src/test/java/org/apache/maven/lifecycle/DefaultLifecyclesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ void testCustomLifecycle() throws ComponentLookupException {
when(mockedPlexusContainer.lookupMap(Lifecycle.class)).thenReturn(lifeCycles);

DefaultLifecycles dl = new DefaultLifecycles(
new DefaultLifecycleRegistry(Collections.emptyList(), Collections.emptyMap()),
new DefaultLifecycleRegistry(
List.of(new DefaultLifecycleRegistry.LifecycleWrapperProvider(mockedPlexusContainer))),
new DefaultLookup(mockedPlexusContainer));

assertThat(dl.getLifeCycles().get(0).getId(), is("clean"));
Expand Down
2 changes: 0 additions & 2 deletions maven-core/src/test/java/org/apache/maven/lifecycle/MavenExecutionPlanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ void testFindLastInPhase() throws Exception {
MavenExecutionPlan plan = LifecycleExecutionPlanCalculatorStub.getProjectAExecutionPlan();

ExecutionPlanItem expected = plan.findLastInPhase("package");
ExecutionPlanItem beerPhase = plan.findLastInPhase("BEER"); // Beer comes straight after package in stub
assertEquals(expected, beerPhase);
assertNotNull(expected);
}

Expand Down
9 changes: 0 additions & 9 deletions maven-core/src/test/java/org/apache/maven/project/EmptyLifecycleBindingsInjector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ public class EmptyLifecycleBindingsInjector extends DefaultLifecycleBindingsInje
private static PackagingRegistry packagingRegistry;

private static final LifecycleRegistry emptyLifecycleRegistry = new LifecycleRegistry() {
@Override
public List<String> computePhases(Lifecycle lifecycle) {
return List.of();
}

@Override
public Iterator<Lifecycle> iterator() {
Expand Down Expand Up @@ -135,11 +131,6 @@ public Optional<Lifecycle> lookup(String id) {
return getDelegate().lookup(id);
}

@Override
public List<String> computePhases(Lifecycle lifecycle) {
return getDelegate().computePhases(lifecycle);
}

@Override
public Iterator<Lifecycle> iterator() {
return getDelegate().iterator();
Expand Down