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

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

Merged
merged 1 commit into from Apr 24, 2019

Conversation

@srdo
Copy link
Contributor

@srdo srdo commented Apr 20, 2019

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 srdo force-pushed the issue-326 branch 3 times, most recently from c985d30 to 363900f Apr 21, 2019
@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 21, 2019

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

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 22, 2019

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

+1

Copy link
Contributor

@ppalaga ppalaga left a comment

There are some questions and suggestions inline.

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

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

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

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

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

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

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

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?
Copy link
Contributor

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Removed the comment

@Override
public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
{
this.aetherRepoSession = aetherRepoSession;
Copy link
Contributor

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

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

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ppalaga ppalaga Apr 23, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Done

* 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.
*/
Copy link
Contributor

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 22, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Using MavenSession


public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
{
this.aetherRepoSession = aetherRepoSession;
Copy link
Contributor

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

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

public void setAetherRepoSession( RepositorySystemSession aetherRepoSession )
{
this.aetherRepoSession = aetherRepoSession;
Copy link
Contributor

@ppalaga ppalaga Apr 22, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@srdo srdo Apr 23, 2019

Choose a reason for hiding this comment

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

Replaced with MavenSession injection

@srdo
Copy link
Contributor Author

@srdo srdo commented Apr 23, 2019

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
Copy link
Contributor Author

@srdo srdo commented Apr 23, 2019

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 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
@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 23, 2019

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
Copy link
Contributor Author

@srdo srdo commented Apr 23, 2019

Yes, done.

@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 23, 2019

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

@ppalaga ppalaga merged commit c0ff2d7 into mojohaus:master Apr 24, 2019
2 checks passed
@ppalaga
Copy link
Contributor

@ppalaga ppalaga commented Apr 24, 2019

Great work, thanks a lot, @srdo !

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

Successfully merging this pull request may close these issues.

None yet

2 participants