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.

Sign up for GitHub

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

Implement workaround for MCOMPILER-503 #149

Merged
merged 2 commits into from
Aug 2, 2022
Merged

Implement workaround for MCOMPILER-503 #149

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f28fc0
Introduce workaround for MCOMPILER-XXX by updating the `annotationPro…
rickie Jul 6, 2022
269c702
Fix a boo-boo and update ticket reference
rickie Aug 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 52 additions & 59 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,12 +758,64 @@
<artifactId>maven-compiler-plugin</artifactId>
<version>3.10.1</version>
<configuration>
<!-- XXX: MCOMPILER-503: 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. -->
<annotationProcessorPaths>
<path>
<groupId>${groupId.error-prone}</groupId>
<artifactId>error_prone_core</artifactId>
<version>${version.error-prone}</version>
</path>
<path>
<groupId>com.google.auto.service</groupId>
<artifactId>auto-service</artifactId>
<version>${version.auto-service}</version>
</path>
<path>
<groupId>com.google.guava</groupId>
<artifactId>guava-beta-checker</artifactId>
<version>${version.guava-beta-checker}</version>
</path>
<path>
<groupId>com.jakewharton.nopen</groupId>
<artifactId>nopen-checker</artifactId>
<version>${version.nopen-checker}</version>
</path>
<!-- XXX: Before enabling these plugins we'll
need to resolve some violations. Some of the
checks will need to be disabled.
<path>
<groupId>com.palantir.assertj-automation</groupId>
<artifactId>assertj-error-prone</artifactId>
<version>${version.palantir-assertj-automation}</version>
</path>
<path>
<groupId>com.palantir.baseline</groupId>
<artifactId>baseline-error-prone</artifactId>
<version>${version.palantir-baseline}</version>
</path>
-->
<path>
<groupId>com.uber.nullaway</groupId>
<artifactId>nullaway</artifactId>
<version>${version.nullaway}</version>
</path>
<path>
<groupId>jp.skypencil.errorprone.slf4j</groupId>
<artifactId>errorprone-slf4j</artifactId>
<version>${version.error-prone-slf4j}</version>
</path>
<path>
<groupId>org.mockito</groupId>
<artifactId>mockito-errorprone</artifactId>
<version>${version.mockito}</version>
</path>
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

</annotationProcessorPaths>
<compilerArgs>
<arg>-Xmaxerrs</arg>
Expand Down Expand Up @@ -1419,65 +1471,6 @@
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<annotationProcessorPaths combine.children="append">
<path>
<groupId>${groupId.error-prone}</groupId>
<artifactId>error_prone_annotations</artifactId>
<version>${version.error-prone}</version>
</path>
<path>
<groupId>${groupId.error-prone}</groupId>
<artifactId>error_prone_core</artifactId>
<version>${version.error-prone}</version>
</path>
<!-- 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>
<path>
<groupId>com.google.guava</groupId>
<artifactId>guava-beta-checker</artifactId>
<version>${version.guava-beta-checker}</version>
</path>
<path>
<groupId>com.jakewharton.nopen</groupId>
<artifactId>nopen-checker</artifactId>
<version>${version.nopen-checker}</version>
</path>
<!-- XXX: Before enabling these plugins we'll
need to resolve some violations. Some of the
checks will need to be disabled.
<path>
<groupId>com.palantir.assertj-automation</groupId>
<artifactId>assertj-error-prone</artifactId>
<version>${version.palantir-assertj-automation}</version>
</path>
<path>
<groupId>com.palantir.baseline</groupId>
<artifactId>baseline-error-prone</artifactId>
<version>${version.palantir-baseline}</version>
</path>
-->
<path>
<groupId>com.uber.nullaway</groupId>
<artifactId>nullaway</artifactId>
<version>${version.nullaway}</version>
</path>
<path>
<groupId>jp.skypencil.errorprone.slf4j</groupId>
<artifactId>errorprone-slf4j</artifactId>
<version>${version.error-prone-slf4j}</version>
</path>
<path>
<groupId>org.mockito</groupId>
<artifactId>mockito-errorprone</artifactId>
<version>${version.mockito}</version>
</path>
</annotationProcessorPaths>
<compilerArgs combine.children="append">
<!-- Enable and configure Error Prone. -->
<arg>
Expand Down