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

Improvement: Bump errorprone to 2.11.0 #2069

Merged
merged 8 commits into from
Mar 28, 2022
Merged

Conversation

robert3005
Copy link
Contributor

Before this PR

After this PR

==COMMIT_MSG==
Upgrade errorprone to 2.11.0
==COMMIT_MSG==

Since we bundle mockito errorprone rules and there was a slight api break in errorprone we need mockito to release their updated rules first mockito/mockito#2553

Possible downsides?

@policy-bot policy-bot bot requested a review from ferozco February 3, 2022 18:57
check("StrictUnusedVariable", CheckSeverity.OFF)
}
}

// Incorrectly identifies tests as junit4 usage
tasks.checkJUnitDependencies.enabled = false
tasks.javadoc.enabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea how to pass --add-exports flags to javadoc in gradle (options are definitely not being respected) and at this point I have given up and disabled javadoc for errorprone.

@@ -42,6 +42,7 @@ allprojects {
tasks.withType(JavaCompile) {
options.compilerArgs += ['-Werror', '-Xlint:deprecation']
options.errorprone {
enabled = false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this can be reenabled once we release this pr and pick it up - otherwise we fail because of the break

'--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED',
'--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED',
'--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED',
'--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED'
Copy link
Contributor

@ferozco ferozco Feb 3, 2022

Choose a reason for hiding this comment

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

should we just set this in gradle.properties? I think it will fix the java doc problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not, I spent couple hours trying to make it work and nothing made sense. Since with toolchains with fork the jvm for compilation add this to gradle.properties doesn't fix compilation

@@ -152,6 +152,10 @@ class BaselineErrorProneRefasterIntegrationTest extends AbstractPluginTest {
when:
buildFile << standardBuildFile
buildFile << '''
tasks.withType(JavaCompile) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is unnecessary once mockito releases with bumped errorprone

@@ -68,6 +68,9 @@ class BaselineExactDependenciesTest extends AbstractPluginTest {
mavenCentral()
mavenLocal() // for baseline-error-prone
}
tasks.withType(JavaCompile) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is unnecessary once mockito releases with bumped errorprone

@avisbal
Copy link

avisbal commented Feb 4, 2022

I'm seeing the following error in some excavators, would you mind updating AvoidNewHashMapInt as well? I'm guessing there was a break for MethodMatchers$ConstructorClassMatcher.withParameters(java.lang.String[]).

Caused by: java.lang.NoSuchMethodError: 'com.google.errorprone.matchers.method.MethodMatchers$ParameterMatcher com.google.errorprone.matchers.method.MethodMatchers$ConstructorClassMatcher.withParameters(java.lang.String[])'
	at com.palantir.baseline.errorprone.AvoidNewHashMapInt.<clinit>(AvoidNewHashMapInt.java:48)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at java.base/java.util.ServiceLoader$ProviderImpl.newInstance(ServiceLoader.java:780)
	... 49 more

@robert3005
Copy link
Contributor Author

robert3005 commented Feb 4, 2022

I already have - the problem is that you have the rule compiled against 2.10.0 and you try to run it against 2.11.0. What you're seeing is a linking problem - if you'd recompile the method against new errorprone it will be fine since signature is api but not abi compatible

```
./gradlew classes testClasses -PerrorProneApply=BugPatternNaming
./gradlew format
```
@bulldozer-bot bulldozer-bot bot merged commit 20c2a69 into develop Mar 28, 2022
@bulldozer-bot bulldozer-bot bot deleted the rk/bump-errorprone branch March 28, 2022 18:34
@svc-autorelease
Copy link
Collaborator

Released 4.82.0

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

Successfully merging this pull request may close these issues.

7 participants