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.

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

[MNG-8052] New Lifecycle API #1411

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[MNG-8052] New Lifecycle API #1411

wants to merge 4 commits into from

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Feb 13, 2024

@gnodet gnodet force-pushed the lifecycle branch 2 times, most recently from e352d2e to bae14ce Compare February 13, 2024 14:20
@gnodet gnodet changed the title Lifecycle API / SPI (wip) [MNG-8052] New Lifecycle for Maven 4 Feb 17, 2024
@gnodet gnodet force-pushed the lifecycle branch 2 times, most recently from bc224e1 to b1895a6 Compare February 19, 2024 14:11
@gnodet gnodet force-pushed the lifecycle branch 4 times, most recently from 22f2b4b to 36110b1 Compare February 27, 2024 20:51
@gnodet gnodet changed the title [MNG-8052] New Lifecycle for Maven 4 Lifecycle API for Make n4 Feb 27, 2024
@gnodet gnodet changed the title Lifecycle API for Make n4 Lifecycle API for Maven 4 Feb 27, 2024
@gnodet gnodet force-pushed the lifecycle branch 3 times, most recently from 049146f to 956ee16 Compare February 29, 2024 07:00
@gnodet gnodet changed the title Lifecycle API for Maven 4 Lifecycle API and dynamic phases Mar 13, 2024
@gnodet gnodet changed the title Lifecycle API and dynamic phases Lifecycle API Mar 21, 2024
@gnodet gnodet force-pushed the lifecycle branch 3 times, most recently from 9013a20 to 8bf3a5f Compare March 22, 2024 15:10
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Enum values should be upper case by convention

@gnodet gnodet changed the title Lifecycle API [MNG-8052] New Lifecycle API Apr 4, 2024
@gnodet gnodet marked this pull request as ready for review April 4, 2024 09:55
String POST = "post:";
String RUN = "run:";

String READY = "ready";
Copy link
Member

Choose a reason for hiding this comment

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

What is this stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really used anymore in this PR, so I'll remove those new concepts and I'll defer their addition to #1429.

For the record, it's used for the new lifecycle definition supporting dynamic phases and a fully concurrent build, see https://github.com/apache/maven/pull/1429/files#diff-dbccda410d9433bca88c9821a2ebd7ecb3b759738a2599a85748766c34658547R224-R252
In this PR, the phases are more fine-grained with inter-phases dependencies and more fine-grained inter-project dependencies. The lifecycle is not a simple list anymore, but looks more like a tree.
So ready represents the state of a project which is ready to be used as a dependency. This usually means that resources have been processed and classes compiled. The main point is that the compile phase should not have to be executed after the resources phase, as they both execute on different input and output and can be executed in parallel. So compile phase depends on sources phase, and ready depends on compile and resources, then package depends on ready.
The benefits (in addition to supporting dynamic phases with pre/post steps) is that we can have a package phase which does not depend on running tests, while verify would package and run unit/integration tests. This also cleanly solves problems coming from running mvn compile where some post processing can be mapped to that ready phase. So the definition of the tree-lifecycle above is not well defined yet, especially, the problem is how to bring those new features and keep a good level of compatibility.

I'd rather have a clean set of inter-phase and inter-project dependencies (for example, install and deploy do depend on package, but should not depend on running unit-tests and integration-tests).

Anyway, this can be deferred and discussed in the context of #1429, so I'll trim the Lifecycle interface to what is currently in Maven 3 (keeping in mind it can be extended in the future).


@Override
public Iterator<Lifecycle> iterator() {
return values.values().iterator();
Copy link
Member

Choose a reason for hiding this comment

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

Is this remove-safe?

Set<String> s2 =
computed.stream().filter(s -> !given.contains(s)).collect(Collectors.toSet());
throw new IllegalArgumentException(
"List of phases differ in size: expected " + computed.size() + " but received " + given.size()
Copy link
Member

Choose a reason for hiding this comment

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

, but

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 IllegalArgumentException(
Copy link
Member

Choose a reason for hiding this comment

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

Why is it IAE instead of ISE?

@@ -18,8 +18,8 @@
*/
package org.apache.maven.lifecycle;

import java.util.List;
import java.util.Map;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we avoid this?

@gnodet
Copy link
Contributor Author

gnodet commented Apr 11, 2024

Mmh, it does not make sense to remove the new stuff from this PR, as the JIRA is about the new lifecycle.
I'll remove the changes for #1457 so that it provide a simple API, and keep that one for the new lifecycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants