-
Notifications
You must be signed in to change notification settings - Fork 168
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
[MCOMPILER-391] Use dep mgmt when resolving annotation processors and their deps #180
Conversation
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Outdated
Show resolved
Hide resolved
4090001
to
0ba9e03
Compare
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
I decided to proceed with the suggested solution. The PR now contains changes for both use cases:
There is also a new flag that enables / disables the usage of dependency management for the second use case. I added some inline comments for things that I am not 100% sure about or things that potentially need more eyes. |
I ran a quick performance test using the Quarkus repo (1k modules, where 500 of them are using annotation processors) and I could not really spot any noticeable difference in the overall build time. I am sure there is more "work" being done (e.g. search those dependency management lists), but it seems it gets lost in the rest of the work being done as part of the build. I ran the following command:
(using the enforcer 3.2.1 since that is much faster than the default 3.0.0-M3)
In all cases I got build times ranging from 01:49min to 01:52min and there was no noticeable difference overall. |
…ir deps * dependency managemet can be used for two slighty difference use cases (when it comes to annotation processors): -- getting the version of the top-level processor path elements -- passing the list of managed dependencies to maven-resolver when resolving the whole processorpath These two can be combined, so there is total of 4 valid combinations (some of them make more sense than the others, but generally there is no reason to not support all of them) * using dependency management when resolving transitive dependencies of annotation processors is something that may or may not be desired. For that reason, there is a new flag that explicitly enables this behavior, while default is to _not_ use dependency management (the current behavior)
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.
Very good work.
Thanks for comments with explanation.
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugin/compiler/AbstractCompilerMojo.java
Show resolved
Hide resolved
could we kindly ask for an increase in priority on this issue. The fact that Maven is unable to correctly manage annotation processor dependencies like Gradle can is a major disadvantage. It is issues like this that imagine lead to any tool chain that relies on annotation processors moving away from Maven (see Android) |
I can update the docs, based on the comments above. Besides that, the PR should be ready from my point of view. Please let me know if there is anything else that would need changing. |
Just a heads up: This implementation does not seem to respect relocations, e.g. I was still using https://mvnrepository.com/artifact/org.hibernate/hibernate-jpamodelgen in my plugin config but the Quarkus BOM contains (only) the new and proper group id PS: I think I never got a relocation hint on the console. |
@famod could you please provide a bit more details, maybe with an example configuration of the plugin? Are you relying on the new dependency management, or specifying the version manually as before? |
@psiroky sure:
Yes, that's what I tried and this doesn't work (old groupId): <configuration>
<annotationProcessorPaths>
<path>
<groupId>org.hibernate</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
</path> but this does work (new, relocated groupId): <configuration>
<annotationProcessorPaths>
<path>
<groupId>org.hibernate.orm</groupId>
<artifactId>hibernate-jpamodelgen</artifactId>
</path> The Quarkus BOM I'm importing has this entry (new groupId only!): |
@famod thanks. In this case, I am not quite sure this is supported from Maven itself. I assume there is no version anywhere in the project depMgmt for Does that make sense? Or am I overlooking something in the setup? Or this perhaps something that is working for you outside of the compiler plugin configuration with standard project dependencies? |
Yeah, I think you have a very valid point: There is no explicit version for that old groupId and so there is no precise way to find the exact pom with the relocation info (=> new groupId). For extra user-friendliness this plugin could check (if no version is found) for similar deps, e.g. same artifactId but different groupId and print put all those candidates, but relocations are not limited to groupIds (AFAIK), so... It probably boils down to the question why there are/were no relocation warnings like you get for "normal" dependencies. For general plugin dependencies that's clearly something that Maven should cover. |
Yeah, there could possibly be some sort of heuristics to try to detect these relocations. Not sure if this would be something that should be done directly in the plugin or rather left to the dependency resolver. I think the missing relocation warning has the same cause - the plugin (or more precisely the dependency resolution mechanism the plugin uses) simply does not know that the artifact was relocated, because without version, it cannot fetch the relocation config. I believe the relocation warning gets printed in case you depend on the old artifact, but the version must be known, explicitly or via depMgmt: e.g:
In that case, the POM is fetched, relocation config found and the warning can be printed. |
I guess I should have been more precise: I had exactly that (only a version property instead of the version literal, but that shouldn't make any difference) and still no warning. Maven 3.9.6, btw. |
Ah, I see. In that case you are right, the relocation message should be there. I'll see if I can find some time to take a look at this. |
dependency managemet can be used for two slighty difference use
cases (when it comes to annotation processors):
-- getting the version of the top-level processor path elements
-- passing the list of managed dependencies to maven-resolver
when resolving the whole processorpath
These two can be combined, so there is total of 4 valid combinations
(some of them make more sense than the others, but generally there is
no reason to not support all of them)
using dependency management when resolving transitive dependencies of
annotation processors is something that may or may not be desired.
For that reason, there is a new flag that explicitly enables this
behavior, while default is to not use dependency management
(the current behavior)
do not be scared by the the number of new lines. 95% of that are ITs, and of that like 50% are license headers 😄
These are the 4 use cases I am considering as part of this PR:
Version of the annotation processor path is explicitly set + useDepMgmt flag = false (default) -- the current behavior, dependency management is not used at all
Version of the annotation processor path is explicitly set + useDepMgmt flag = true (this may make the least sense of the combinations, but I see no reason to not allow it)
Version of the annotation processor dep is taken from dep mgmt + useDepMgmt flag = false (dep mgmt is when resolving the whole processorpath).
Version of the annotation processor dep is taken from dep mgmt + useDepMgmt flag= true (dep mgmt used when resolving the whole processorpath)
Note: the whole logic around annotation processor paths is getting bit out of hand (read more complex) and I think it would make sense to extract this into a separate class. I did want to complicate this PR further though, so that would be something for next PR(s).
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MCOMPILER-XXX] - Fixes bug in ApproximateQuantiles
,where you replace
MCOMPILER-XXX
with the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verify
to make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean 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.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.