Implement workaround for MCOMPILER-503 #149
Conversation
Stephan202
force-pushed
the
rossendrijver/annotation_processor_path_work_around
branch
from
4f1d397 to
716b003
Compare
pom.xml
Outdated
| <!-- This is a dependency of some Error Prone | ||
| plugins, but for licensing reasons it is not | ||
| packaged with the artifact. --> | ||
| <path> | ||
| <groupId>com.google.code.findbugs</groupId> | ||
| <artifactId>jFormatString</artifactId> | ||
| <version>${version.findbugs-format-string}</version> | ||
| </path> |
There was a problem hiding this comment.
Seems we don't need this anymore; let's drop it :)
pom.xml
Outdated
| <path> | ||
| <groupId>${groupId.error-prone}</groupId> | ||
| <artifactId>error_prone_annotations</artifactId> | ||
| <version>${version.error-prone}</version> | ||
| </path> |
pom.xml
Outdated
| <!-- XXX: MCOMPILER-XXX: The annotation processor's | ||
| dependency resolution results in a highly unintuitive | ||
| classpath. We can work around this by listing all | ||
| dependencies here, if possible, to be able to control | ||
| the resolution order. --> |
There was a problem hiding this comment.
| <!-- XXX: MCOMPILER-XXX: The annotation processor's | |
| dependency resolution results in a highly unintuitive | |
| classpath. We can work around this by listing all | |
| dependencies here, if possible, to be able to control | |
| the resolution order. --> | |
| <!-- XXX: MCOMPILER-YYY: The plugin contructs a highly | |
| unintuitive annotation processor classpath, in which | |
| some indirect dependencies take precedence over | |
| explicitly defined dependencies. This can largely be | |
| mitigated through careful ordering of the dependencies, | |
| but this prevents (where relevant) declaring these | |
| dependencies as part of their corresponding Maven | |
| profile. --> |
There was a problem hiding this comment.
Thanks for looking into this and opening a ticket upstream (soon) 🙏 I got one question before ✔️
Suggested commit message:
Order `annotationProcessorPaths` for `maven-compiler-plugin` (#149)
To circumvent an issue in which indirect dependencies take precedence over
explicitly defined dependency on the annotation processor's classpath.
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-errorprone</artifactId> | ||
| <version>${version.mockito}</version> | ||
| </path> |
There was a problem hiding this comment.
So to my understanding, we just need to have error_prone_core entry before auto-service to workaround the compiler issues. Does this also hold for the other artifacts included here? Asking this because now I think some build checks (from guava, nopen, nullaway) will still happen even though verification.skip is provided. Otherwise, reading the comment, I believe these could just stay in said profile.
I assume error-prone is still not enabled when providing verification.skip as configured in the error-prone profile, so that is nice.
There was a problem hiding this comment.
guava-beta-checker, nopen-checker, nullaway, errorprone-slf4j and mockito-errorprone are all Error Prone checks, so without the error-prone profile their declaration is a no-op (and thus they're not active with -Dverification.skip).
It's true that likely it's safe to keep them in their original location, but this way we have a clearer control over the annotation classpath. The logic is now: declare centrally unless that's really not possible (which applies to the two other annotationProcessorPaths declarations).
We plan to simply revert this PR once the issue is fixed in maven-compiler-plugin.
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-errorprone</artifactId> | ||
| <version>${version.mockito}</version> | ||
| </path> |
There was a problem hiding this comment.
guava-beta-checker, nopen-checker, nullaway, errorprone-slf4j and mockito-errorprone are all Error Prone checks, so without the error-prone profile their declaration is a no-op (and thus they're not active with -Dverification.skip).
It's true that likely it's safe to keep them in their original location, but this way we have a clearer control over the annotation classpath. The logic is now: declare centrally unless that's really not possible (which applies to the two other annotationProcessorPaths declarations).
We plan to simply revert this PR once the issue is fixed in maven-compiler-plugin.
rickie
force-pushed
the
rossendrijver/annotation_processor_path_work_around
branch
from
716b003 to
0f28fc0
Compare
|
So the ticket is there now: https://issues.apache.org/jira/browse/MCOMPILER-503. |
rickie
changed the title
annotationProcessorPathsannotationProcessorPaths
There was a problem hiding this comment.
Alternative suggested commit message:
Implement workaround for MCOMPILER-503 (#149)
By moving around some annotation processor classpath entries we ensure that the
`maven-compiler-plugin` uses the appropriate version of Error Prone.
See https://issues.apache.org/jira/browse/MCOMPILER-503
There was a problem hiding this comment.
Alternative suggested commit message LGTM. Thanks for the explanation @Stephan202 and @rickie for creating the issue upstream! 🚀
rickie
changed the title
annotationProcessorPaths
Currently used
MCOMIPLER-XXXas we first need to create the ticket, that is a WIP.