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

[MPLUGIN-441] Support for the new maven4 api #117

Merged
merged 7 commits into from
Nov 15, 2022
Merged

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Jul 8, 2022

JIRA issue: https://issues.apache.org/jira/browse/MPLUGIN-441

This PR includes the following:

  • raise the minimum JDK to 1.8
  • provide additional support for the new maven4 api using maven 4.0.0-alpha-2

The plugin still supports maven 3 plugins so is compatible with all the existing plugins.
The new support is provided by the annotation scanner which now supports the new package org.apache.maven.api.plugin in addition to the old one. The main difference is that if the new package is detected, a flag will be set on the mojo descriptor so that maven can know that the mojo is a v4 mojo and behave accordingly.

@gnodet gnodet marked this pull request as draft July 8, 2022 11:01
@gnodet gnodet force-pushed the m-api branch 2 times, most recently from 05804d9 to bc87d15 Compare July 22, 2022 09:18
@gnodet gnodet changed the title Support mojos using the new API Switch to maven 4 and the new api Aug 24, 2022
@gnodet gnodet changed the title Switch to maven 4 and the new api Switch to maven 4 and the new api Aug 24, 2022
@gnodet gnodet marked this pull request as ready for review September 1, 2022 07:25
@gnodet gnodet changed the title Switch to maven 4 and the new api [4.x] Switch to maven 4 and the new api Oct 12, 2022
@gnodet gnodet changed the title [4.x] Switch to maven 4 and the new api [4.x] Support for the new maven4 api Oct 13, 2022
@gnodet gnodet force-pushed the m-api branch 3 times, most recently from 931f8e6 to 4a5998a Compare October 27, 2022 12:09
@gnodet gnodet requested review from cstamas and kwin October 30, 2022 19:45
@gnodet gnodet changed the title [4.x] Support for the new maven4 api [MPLUGIN-441] Support for the new maven4 api Nov 10, 2022
@gnodet gnodet force-pushed the m-api branch 2 times, most recently from 939348f to 07a5f84 Compare November 10, 2022 13:47
@gnodet
Copy link
Contributor Author

gnodet commented Nov 10, 2022

@kwin @slawekjaranowski I've rebased, cleaned up and added an IT. The base maven version has been reverted to 3.2.5 so that the plugin remains fully compatible with 3.x.

pom.xml Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
maven-plugin-plugin/pom.xml Outdated Show resolved Hide resolved
maven-plugin-plugin/src/it/v4api/pom.xml Outdated Show resolved Hide resolved
maven-plugin-plugin/src/it/v4api/pom.xml Outdated Show resolved Hide resolved
maven-plugin-plugin/src/it/v4api/pom.xml Outdated Show resolved Hide resolved
Comment on lines +67 to +72
mavenComponents.put( "org.apache.maven.api.Session", "${session}" );
mavenComponents.put( "org.apache.maven.api.Project", "${project}" );
mavenComponents.put( "org.apache.maven.api.MojoExecution", "${mojoExecution}" );
// TODO: apiv4: add PluginDescriptor to the api ?
//mavenComponents.put( "org.apache.maven.api.descriptor.PluginDescriptor", "${plugin}" );
mavenComponents.put( "org.apache.maven.api.settings.Settings", "${settings}" );
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about it ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was wondering about that as well. When I added the junit test, I copy/pasted an existing one and made sure it was working correcectly.
However, this is tied to maven itself and the way those parameters/components are injected.
From an API pov, using @Parameter( property = "${session}" ) Session session is not really intuitive, whereas @Component Session session looks simplier.

Copy link
Member

@slawekjaranowski slawekjaranowski Nov 10, 2022

Choose a reason for hiding this comment

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

As I remember @Component Session session is translated to @Parameter( property = "${session}" ) - to check if still needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and the map is here to do that. So if I remove the population of the mavenComponents with objects from the v4 api, this won't work anymore. The IT actually tests that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I don't think the use of @Parameter or @Component should be dictated by the implementation. For example, the maven session is available as both a parameter injected by the plugin manager and a sisu bean inside the @SessionScope. Also @Component relates to the sisu components, but this does not have to be made explicit.
I would personally favor the @Component approach for everything that does not come from the configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Me too.

@gnodet
Copy link
Contributor Author

gnodet commented Nov 15, 2022

@slawekjaranowski are you ok with the current PR ?

@slawekjaranowski
Copy link
Member

one for build log ...

The following dependencies are in wrong scope:
 * org.apache.maven:plexus-utils:jar:4.0.0-alpha-2:compile

@slawekjaranowski
Copy link
Member

Trying:

mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:first

Apache Maven 4.0.0-alpha-2 (c07700ffc97d3967a8de77d5a1065f5e6115c00e)

[ERROR] Failed to parse plugin descriptor for org.apache.maven.its:v4api:1.0-SNAPSHOT (/Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar): JAR entry META-INF/maven/lifecycle.xml not found in /Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar
mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:first

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

[ERROR] Failed to parse plugin descriptor for org.apache.maven.its:v4api:1.0-SNAPSHOT (/Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar): JAR entry META-INF/maven/lifecycle.xml not found in /Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar -> [Help 1]

@gnodet
Copy link
Contributor Author

gnodet commented Nov 15, 2022

one for build log ...

The following dependencies are in wrong scope:
 * org.apache.maven:plexus-utils:jar:4.0.0-alpha-2:compile

plexus-utils is not provided anymore. I think the plugin is fooled by the org.apache.maven groupid, so I think this is correct. See https://issues.apache.org/jira/browse/MNG-6965

@slawekjaranowski
Copy link
Member

mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:help
Apache Maven 4.0.0-alpha-2 (c07700ffc97d3967a8de77d5a1065f5e6115c00e)
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------------------------------------< org.apache.maven:standalone-pom >--------------------------------------------
[INFO] Building Maven Stub Project (No POM) 1
[INFO] ---------------------------------------------------------[ pom ]----------------------------------------------------------
[INFO] 
[INFO] --- v4api:1.0-SNAPSHOT:help (default-cli) @ standalone-pom ---
....

hangs forever, stack:

"main" #1 prio=5 os_prio=31 tid=0x00007fa13980c000 nid=0x2103 runnable [0x00007000033e2000]
   java.lang.Thread.State: RUNNABLE
	at java.util.Hashtable.put(Hashtable.java:459)
	- locked <0x0000000715e3fbd8> (a java.util.Properties)
	at java.util.Hashtable.putAll(Hashtable.java:524)
	- locked <0x0000000715e3fbd8> (a java.util.Properties)
	at org.apache.maven.plugin.PluginParameterExpressionEvaluatorV4.<init>(PluginParameterExpressionEvaluatorV4.java:106)
	at org.apache.maven.plugin.internal.DefaultMavenPluginManager.getConfiguredMojo(DefaultMavenPluginManager.java:617)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)

@gnodet
Copy link
Contributor Author

gnodet commented Nov 15, 2022

Trying:

mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:first

Apache Maven 4.0.0-alpha-2 (c07700ffc97d3967a8de77d5a1065f5e6115c00e)

[ERROR] Failed to parse plugin descriptor for org.apache.maven.its:v4api:1.0-SNAPSHOT (/Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar): JAR entry META-INF/maven/lifecycle.xml not found in /Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar
mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:first

Apache Maven 3.8.6 (84538c9988a25aec085021c365c560670ad80f63)

[ERROR] Failed to parse plugin descriptor for org.apache.maven.its:v4api:1.0-SNAPSHOT (/Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar): JAR entry META-INF/maven/lifecycle.xml not found in /Users/.../.m2/repository/org/apache/maven/its/v4api/1.0-SNAPSHOT/v4api-1.0-SNAPSHOT.jar -> [Help 1]

Do we need the plugin to actually run ?
I can either remove the @Execute or add a lifecycle.xml
After that, another error happens because the plugin is being injected with a custom @Component, but there's no such public component in the v4 api. I can also remove the custom component injection. But the IT is testing the plugin.xml generation and not the plugin itself.
Another option is to document that this plugin does not actually work.

@gnodet
Copy link
Contributor Author

gnodet commented Nov 15, 2022

one for build log ...

The following dependencies are in wrong scope:
 * org.apache.maven:plexus-utils:jar:4.0.0-alpha-2:compile

plexus-utils is not provided anymore. I think the plugin is fooled by the org.apache.maven groupid, so I think this is correct. See https://issues.apache.org/jira/browse/MNG-6965

I've removed the warning.

@gnodet
Copy link
Contributor Author

gnodet commented Nov 15, 2022

mvn org.apache.maven.its:v4api:1.0-SNAPSHOT:help
Apache Maven 4.0.0-alpha-2 (c07700ffc97d3967a8de77d5a1065f5e6115c00e)
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------------------------------------< org.apache.maven:standalone-pom >--------------------------------------------
[INFO] Building Maven Stub Project (No POM) 1
[INFO] ---------------------------------------------------------[ pom ]----------------------------------------------------------
[INFO] 
[INFO] --- v4api:1.0-SNAPSHOT:help (default-cli) @ standalone-pom ---
....

hangs forever, stack:

"main" #1 prio=5 os_prio=31 tid=0x00007fa13980c000 nid=0x2103 runnable [0x00007000033e2000]
   java.lang.Thread.State: RUNNABLE
	at java.util.Hashtable.put(Hashtable.java:459)
	- locked <0x0000000715e3fbd8> (a java.util.Properties)
	at java.util.Hashtable.putAll(Hashtable.java:524)
	- locked <0x0000000715e3fbd8> (a java.util.Properties)
	at org.apache.maven.plugin.PluginParameterExpressionEvaluatorV4.<init>(PluginParameterExpressionEvaluatorV4.java:106)
	at org.apache.maven.plugin.internal.DefaultMavenPluginManager.getConfiguredMojo(DefaultMavenPluginManager.java:617)
	at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo(DefaultBuildPluginManager.java:137)

This looks like a bug in https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/internal/impl/PropertiesAsMap.java

I've raised https://issues.apache.org/jira/browse/MNG-7597 with maven#872.

@slawekjaranowski
Copy link
Member

This looks like a bug in https://github.com/apache/maven/blob/master/maven-core/src/main/java/org/apache/maven/internal/impl/PropertiesAsMap.java

Yes - iterator next method always return the same element 😄

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

As plugin under test is only used for generating descriptor - it is ok for me.

@gnodet gnodet added this to the 3.8.0 milestone Nov 15, 2022
@gnodet gnodet merged commit 50482ba into apache:master Nov 15, 2022
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