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-8134] Add a @Resolution annotation to mojos to inject project dependencies collection / resolution result #1559

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Jun 5, 2024

JIRA issue: https://issues.apache.org/jira/browse/MNG-8134

The apache/maven-plugin-tools#295 PR updates m-plugin-p to generate the correct descriptor and modifies the v4 IT to make sure this new annotation works correctly.

In short, a mojo could be defined as:

@Mojo(name = "my-mojo")
public class MyMojo implements org.apache.maven.api.plugin.Mojo {

    @Parameter
    boolean skip;

    @Inject
    Session session;

    @Resolution(pathScope = "main-compile")
    Map<PathType, List<Path>> paths;

    public void execute() {
        // ... do stuff ...
    }
}

and the mojo would be injected with the resolved dependencies for the main-compile path scope.

@gnodet gnodet added this to the 4.0.0-beta-4 milestone Jun 5, 2024
@cstamas
Copy link
Member

cstamas commented Jun 5, 2024

Can a mojo have 2 or more fields annotated like this?

@gnodet
Copy link
Contributor Author

gnodet commented Jun 5, 2024

Can a mojo have 2 or more fields annotated like this?

Yes, the mojo descriptor contains a List<Dependencies>, each Dependencies object containing the field name and the pathScope value (null meaning a collection rather than a resolution).

@cstamas
Copy link
Member

cstamas commented Jun 5, 2024

This then opens interesting possibilities for Mojos...

@gnodet gnodet force-pushed the MNG-8134 branch 4 times, most recently from 5377f97 to d6857d7 Compare June 7, 2024 07:31
@gnodet gnodet marked this pull request as ready for review June 10, 2024 11:04
@gnodet
Copy link
Contributor Author

gnodet commented Jun 10, 2024

The v4api IT in apache/maven-plugin-tools#295 adds a field with the @Resolution annotation.

@rmannibucau
Copy link
Contributor

I'd shamefully copy a new DependencySet wrapper idea to avoid to have to inject a map or a list which are rarely what is needed so a think wrapper enabling to lookup what is needed (with filter + default collectors support).

That said I don't know if we need an injection or just this wrapper being accessible from the project - I have a few cases where the scope is configured in the mojo so the injection can need an expression to work (+-0 to make it working or just rely on v4 project).

@gnodet gnodet changed the title [MNG-8134] Add a @Dependencies annotation to mojos to inject project dependencies collection / resolution result [MNG-8134] Add a @Resolution annotation to mojos to inject project dependencies collection / resolution result Jun 10, 2024
@gnodet
Copy link
Contributor Author

gnodet commented Jun 10, 2024

I'd shamefully copy a new DependencySet wrapper idea to avoid to have to inject a map or a list which are rarely what is needed so a think wrapper enabling to lookup what is needed (with filter + default collectors support).

That said I don't know if we need an injection or just this wrapper being accessible from the project - I have a few cases where the scope is configured in the mojo so the injection can need an expression to work (+-0 to make it working or just rely on v4 project).

If you want the wrapper, you can inject the DependencyResolverResult class from the API which provides everything. The other ones are just to simplify for simple use cases. In the example, I used a map, which is one of the fields available from the DependencyResolverResult. The other fields can be injected too, so you can have Node, or List<Node.

For the expression, I thought about it, but I'm not sure that's easily feasible, nor worth it. The API is quite easily usable, it would just save two lines:

        PathScope ps = session.getService(PathScopeRegistry.class).require(pathScope);
        DependencyResolverResult res = session.getService(DependencyResolver.class).resolve(session, project, ps);

and the benefit of having it statically defined (so that we can compute exactly what is needed in advance) would be lost.
Another possibility would be for the mojo to define multiple resolutions:

    @Resolution(pathScope = "main-compile")
    Map<PathType, List<Path>> compilePaths;

    @Resolution(pathScope = "main-test")
    Map<PathType, List<Path>> testPaths;

@gnodet gnodet force-pushed the MNG-8134 branch 2 times, most recently from f43d279 to 4f4e42d Compare June 10, 2024 15:09
@gnodet gnodet force-pushed the MNG-8134 branch 2 times, most recently from 9bb6f67 to ea215b7 Compare June 10, 2024 16:37
…pendencies collection / resolution result
@rmannibucau
Copy link
Contributor

the benefit of having it statically defined (so that we can compute exactly what is needed in advance)

I'm not sure I get it, this should be 1-1 with the programmatic flavor IMHO so either it is cached in the impl and it is always computed in advanced somehow - or there is no pitfall computing it lazily more exactly - or it will be wrong if computed in advanced and a mojo modifies the current scope for next mojo 🤔

I assume - but not sure at all - you speak about adding more concurrency or something like that but I'm not sure we can do it more than the resolution requested by the mojo itself - which needs to move to a list of custom string I guess now.

@gnodet
Copy link
Contributor Author

gnodet commented Jun 10, 2024

the benefit of having it statically defined (so that we can compute exactly what is needed in advance)

I'm not sure I get it, this should be 1-1 with the programmatic flavor IMHO so either it is cached in the impl and it is always computed in advanced somehow - or there is no pitfall computing it lazily more exactly - or it will be wrong if computed in advanced and a mojo modifies the current scope for next mojo 🤔

I assume - but not sure at all - you speak about adding more concurrency or something like that but I'm not sure we can do it more than the resolution requested by the mojo itself - which needs to move to a list of custom string I guess now.

The fact that we statically know that a given mojo execution will require the project dependencies will be helpful if we can implement the fully concurrent lifecycle. My goal would be to have a builder which goes down to the phase or mojo level : all mojos executions should be able to run concurrently and ordered according to the actual dependencies specified between projects.

My assumption is that "Project A depends on Project B" has no direct impact on the build order. It only has an effect if a mojo execution actually requires the dependencies of project A. This is currently not really needed, because we build a given project completely before building another one.
This means we need more fine-grained dependencies: for example, if Project A depends on Project B, at compile time, we don't really need the fully built Project B, we need compiled and post-processed classes. We don't need the compiled test classes, nor the results of the unit tests.
This information can be either stored in phase semantics: all mojos executions in the compile phase need to be executed after the compile time dependencies of the project are themselves in the compile phase. This can also be provided on a per-mojo basis: the @Resolution annotation of this PR does provide some information that a given mojo execution does require its dependencies (though it currently does not specify in which phase, that may need to be added later). With both source of informations, we can build a full graph of mojo executions and run it in a much more efficient way that with our current model.

@rmannibucau
Copy link
Contributor

@gnodet ok, if we mix it with the binding/phase - execution has it - later it will work, 👍, thanks.

@gnodet
Copy link
Contributor Author

gnodet commented Jun 11, 2024

I assume - but not sure at all - you speak about adding more concurrency or something like that but I'm not sure we can do it more than the resolution requested by the mojo itself - which needs to move to a list of custom string I guess now.

When injecting into a field, we can use a single pathScope, however, we have the dependencyResolutionPathScopes field of the @Mojo annotation which can be used for informative purpose (as I explained above), and that one is defined in the javadoc as a comma separated list of pathScopes.

@gnodet gnodet merged commit b852f74 into apache:master Jun 11, 2024
13 checks passed
@gnodet gnodet deleted the MNG-8134 branch June 11, 2024 07:46
@gnodet gnodet modified the milestones: 4.0.0-beta-4, 4.0.0 Jul 6, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants