-
Notifications
You must be signed in to change notification settings - Fork 185
Introduce graphRoots for dependencyFilter based mojos #1571
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
base: master
Are you sure you want to change the base?
Conversation
.../java/org/apache/maven/plugins/dependency/fromDependencies/AbstractDependencyFilterMojo.java
Show resolved
Hide resolved
.../java/org/apache/maven/plugins/dependency/fromDependencies/AbstractDependencyFilterMojo.java
Show resolved
Hide resolved
.../java/org/apache/maven/plugins/dependency/fromDependencies/AbstractDependencyFilterMojo.java
Show resolved
Hide resolved
| List<RemoteRepository> remoteRepositories = | ||
| RepositoryUtils.toRepos(session.getProjectBuildingRequest().getRemoteRepositories()); | ||
|
|
||
| Collection<org.eclipse.aether.artifact.Artifact> depArtifacts = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: we do full (transitive) dependency resolution only when the user specified one or more graphRoots, but not when the project itself is the graphRoot. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project dependencies have already been resolved, as the goal of the final Mojo contains requiresDependencyResolution = ResolutionScope.TEST. So it is actually re-resolving the dependencies. The MavenProject doesn't provide the graph, hence the need ask for the graph user maven artifact resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it. Maybe worth copying that text to an inline comment, in order to prevent future maintainers from asking themselves why the code is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| package org.apache.maven.plugins.dependency.fromDependencies; | ||
|
|
||
| /** | ||
| * All values are static, no expression, so matcher can use equals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the matcher does not use equals()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question. My first approach was to make these values Nullable: if not specified it is a match. However while working with it (and as confirmed in the integration-test) it is probably better to be explicit. Just always specify groupId and artifactId, and type if it is not a jar, just as explicit as the dependency. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would certainly prefer to make it explicit, and not give null the magical meaning of "could be anything" or "don't care".
Just always specify groupId and artifactId, and type if it is not a jar, just as explicit as the dependency.
You mean: actual matching is as explicit as the graphRoot declaration in the plugin configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small rewrite, you know must define the graphRoot's groupId and artifactId matching the targeted dependency. I've removed the type, because only the main artifact can have dependencies (classified like javadoc and sources don't have dependencies) and the pom can only point to 1 main artifact via packaging. I don't expect a request for support of type, but if so it should be solved separately. It is probably tricky if 2 main artifacts share the same pom.
|
|
||
| @Override | ||
| public boolean matches(Dependency dependency) { | ||
| return matches(graphRoot.getGroupId(), dependency.getGroupId()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could've used Objects.equals() here - it's available since Java 1.7, according to its Javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous comment. If graphroot is explicit, this matches can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've decided to keep this class, as we're trying to match 2 different kind of classes. null-ignoring matches method has been replaced.
|
|
||
| import org.apache.maven.model.Dependency; | ||
|
|
||
| class OrDependencyMatcher implements DependencyMatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would've added unit tests for this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| import org.apache.maven.model.Dependency; | ||
|
|
||
| class GraphRootMatcher implements DependencyMatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I would've added unit tests for this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…cies/AbstractDependencyFilterMojo.java Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Maarten Mulders <[email protected]>
Co-authored-by: Maarten Mulders <[email protected]>
| * @since 3.10.0 | ||
| */ | ||
| @Parameter | ||
| private List<GraphRoot> graphRoots; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As GraphRoot is an complex object we can provide an example in documentation to be clear how to use in configuration
| root, session.getRepositorySession().getArtifactTypeRegistry()); | ||
|
|
||
| List<RemoteRepository> remoteRepositories = | ||
| RepositoryUtils.toRepos(session.getProjectBuildingRequest().getRemoteRepositories()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is MavenProject#getRemoteProjectRepositories
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.