Fix #326 Migrate to using Maven 3 APIs and Aether for dependency resolution #327

srdo · 2019-04-20T21:46:35Z

Based on #324, please look at that one first.

This migrates the plugin to Maven 3 dependencies, and also replaces the old ArtifactResolver with maven-resolver.

The version requirement here is at least 3.5.0, as maven-resolver-provider is not present in prior versions. For now I've set the plugin to need 3.6.1, as that was what I tested with.

srdo · 2019-04-21T07:11:42Z

Set required Maven version to 3.5.4, as that is the version used by CI.

ppalaga · 2019-04-22T15:40:06Z

Set required Maven version to 3.5.4, as that is the version used by CI.

+1

ppalaga

There are some questions and suggestions inline.

Besides, could you please reword to Fix #326 ...?

ppalaga · 2019-04-22T17:16:42Z

src/main/java/org/codehaus/mojo/license/AbstractAddThirdPartyMojo.java

+     * Set repo session in the components that need it.
+     * @param session The repository system session
+     */
+    public void setAetherRepoSession( RepositorySystemSession session )


Why is this setter needed? I do not see it called from anywhere.

Maven calls it when injecting the RepositorySystemSession. The idea was to have this setter initialize the fields in the other classes. I couldn't find a way to get the DI framework to inject it.

Replaced with MavenSession injection

ppalaga · 2019-04-22T17:39:09Z

src/main/java/org/codehaus/mojo/license/AbstractLicensesXmlMojo.java

+     * Set repo session in the components that need it.
+     * @param session The repository system session
+     */
+    public void setAetherRepoSession( RepositorySystemSession session )


Why is this setter needed? I do not see it called from anywhere.

Replaced with MavenSession injection

ppalaga · 2019-04-22T17:41:55Z

src/main/java/org/codehaus/mojo/license/AbstractThirdPartyReportMojo.java

+     * Set repo session in the components that need it.
+     * @param session The repository system session
+     */
+    public void setAetherRepoSession( RepositorySystemSession session )


Why is this setter needed? I do not see it called from anywhere.

Replaced with MavenSession injection

ppalaga · 2019-04-22T18:02:58Z

src/main/java/org/codehaus/mojo/license/api/DefaultThirdPartyTool.java

-            String type, String classifier, ArtifactRepository localRepository, List<ArtifactRepository> repositories )
-                    throws ArtifactResolutionException, IOException, ArtifactNotFoundException
+            String type, String classifier, List<RemoteRepository> remoteRepositories )
+                    throws ArtifactResolutionException
    {
        // TODO: this is a bit crude - proper type, or proper handling as metadata rather than an artifact in 2.1?


Shouldn't this comment be removed? It does not seem to make any sense anymore.

Maybe? I understood the comment to say that handling the third party descriptor as an artifact was crude, but I'm not sure what the intended alternative was. I'll remove the comment.

Removed the comment

ppalaga · 2019-04-22T18:09:34Z

src/main/java/org/codehaus/mojo/license/api/DefaultThirdPartyTool.java

+    @Override
+    public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
+    {
+        this.aetherRepoSession = aetherRepoSession;


Making aetherRepoSession mutable in this way does not feel good. If there is no way to persuade plexus to inject it, I vote for removing the aetherRepoSession field and pass it via method parameter.

I agree, it is not nice. I couldn't find a way to inject it. I'll move it to method parameters.

Replaced with MavenSession injection

ppalaga · 2019-04-22T18:20:17Z

src/main/java/org/codehaus/mojo/license/api/DependenciesTool.java

+                     */
+                    Properties props = new Properties();
+                    props.putAll( System.getProperties() );
+                    ProjectBuildingRequest req = new DefaultProjectBuildingRequest()


Do we need a new instance of ProjectBuildingRequest for each iteration of the enclosing for loop?

The parameters don't change, so probably not. I'll change it to reuse the request.

I think it is better here. Constructing the request shouldn't take any real time, and moving it outside the for loop puts a lot of distance between the declaration and use of the request.

Constructing the request shouldn't take any real time

It very much depends on how many iterations the loop has and also on how many sys props are set.

puts a lot of distance

Sounds like a readability/aesthetics concern? - those are important, but here, I'd favor performance, unless proven that pulling the req out of the loop is technically impossible.

Sure, but the system properties aren't being copied anymore, since the session change.

It's just a readability concern.

I'll move it out of the loop.

ppalaga · 2019-04-22T18:22:13Z

src/main/java/org/codehaus/mojo/license/api/DependenciesTool.java

+                     * Some POMs may refer to e.g. java.home or java.version,
+                     * so we need to ensure common system properties are set when parsing POMs.
+                     * Using the current system properties seems like a decent solution.
+                     */


I wonder if it wasn't safer to derive the ProjectBuildingRequest from the currently running request. Something like https://github.com/blackducksoftware/hub-maven-plugin/blob/master/src/main/java/com/blackducksoftware/integration/maven/MavenDependencyExtractor.java#L73

That looks better, wasn't aware of MavenSession. Will try it out.

Using MavenSession

ppalaga · 2019-04-22T18:22:48Z

src/main/java/org/codehaus/mojo/license/api/DependenciesTool.java

+
+    public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
+    {
+        this.aetherRepoSession = aetherRepoSession;


Making aetherRepoSession mutable in this way does not feel good. If there is no way to persuade plexus to inject it, I vote for removing the aetherRepoSession field and pass it via method parameter.

Replaced with MavenSession injection

ppalaga · 2019-04-22T18:23:33Z

src/main/java/org/codehaus/mojo/license/download/LicensedArtifactResolver.java

@@ -216,4 +231,8 @@ public void loadProjectDependencies( ResolvedProjectDependencies artifacts,
    }
    // CHECKSTYLE_ON: MethodLength

+    public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
+    {
+        this.aetherRepoSession = aetherRepoSession;


Making aetherRepoSession mutable in this way does not feel good. If there is no way to persuade plexus to inject it, I vote for removing the aetherRepoSession field and pass it via method parameter.

Replaced with MavenSession injection

srdo · 2019-04-23T18:36:58Z

Addressed comments, and reworded commit message.

The MavenSession has a getter for the RepositorySystemSession, and can be injected with @Requirement, so I just did that rather than add RepositorySystemSession as a parameter everywhere.

srdo · 2019-04-23T19:15:00Z

The test failure is a timeout.

[WARNING] Unable to retrieve license from URL 'http://www.opensource.org/licenses/bsd-license.php' for dependency 'org.hamcrest:hamcrest-core:1.3': Read timed out

ppalaga · 2019-04-23T20:44:06Z

Thanks, looks very good.
Sorry for the last nit pick: Could you please pull the buildReq out of the loop also in LicensedArtifactResolver.loadProjectDependencies()?

srdo · 2019-04-23T20:48:09Z

Yes, done.

ppalaga · 2019-04-23T21:03:00Z

Ouch, an editorconfig violation. Otherwise looks mergeable. Thanks!

…ncy resolution

ppalaga · 2019-04-24T08:18:57Z

Great work, thanks a lot, @srdo !

srdo force-pushed the issue-326 branch 3 times, most recently from c985d30 to 363900f Compare April 21, 2019 07:11

srdo force-pushed the issue-326 branch from 363900f to e5719d3 Compare April 22, 2019 15:03

ppalaga requested changes Apr 22, 2019

View reviewed changes

srdo force-pushed the issue-326 branch from e5719d3 to 3fc3c25 Compare April 23, 2019 18:35

srdo force-pushed the issue-326 branch from 3fc3c25 to 1c5eeca Compare April 23, 2019 18:41

ppalaga changed the title ~~ISSUE-326: Migrate to using Maven 3 APIs and Aether for dependency resolution~~ Fix #326 Migrate to using Maven 3 APIs and Aether for dependency resolution Apr 23, 2019

srdo force-pushed the issue-326 branch from 1c5eeca to fbeb8cb Compare April 23, 2019 20:36

srdo force-pushed the issue-326 branch from fbeb8cb to 92d160d Compare April 23, 2019 20:48

Fix mojohaus#326 Migrate to using Maven 3 APIs and Aether for depende…

340c0ec

…ncy resolution

srdo force-pushed the issue-326 branch from 92d160d to 340c0ec Compare April 23, 2019 21:18

ppalaga merged commit c0ff2d7 into mojohaus:master Apr 24, 2019

srdo mentioned this pull request Sep 24, 2019

Upgrade to Maven 3 APIs and Aether #326

Closed

This was referenced Mar 8, 2021

Bump license-maven-plugin from 1.14 to 2.0.0 in /webapps langfr/camunda-bpm-platform#73

Closed

chore(deps): bump license-maven-plugin from 1.14 to 2.0.0 in /webapps marcus-nl/camunda-bpm-platform#3

Closed

Fix #326 Migrate to using Maven 3 APIs and Aether for dependency resolution #327

Fix #326 Migrate to using Maven 3 APIs and Aether for dependency resolution #327

Conversation

srdo commented Apr 20, 2019

srdo commented Apr 21, 2019

ppalaga commented Apr 22, 2019

ppalaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srdo commented Apr 23, 2019

srdo commented Apr 23, 2019

ppalaga commented Apr 23, 2019

srdo commented Apr 23, 2019

ppalaga commented Apr 23, 2019

ppalaga commented Apr 24, 2019