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

Bump error prone version to 2.4.0 #11426

Conversation

davido
Copy link
Contributor

@davido davido commented May 17, 2020

No description provided.

@davido
Copy link
Contributor Author

davido commented May 18, 2020

@jin @meteorcloudy Question about Guava upgrade to sync with the version used in Error Prone.

It appears that the version is used in android_common/com.android.tools_sdk-common_25.0.0 is totally outdated. Moreover, version 25.0.0 seems to be from April 2016. The current version is provided by Google and is from May 2020: 27.1.0-alpha09:

https://maven.google.com/com/android/tools/sdklib/27.1.0-alpha09/sdklib-27.1.0-alpha09.jar

Are there any plans to bump the sdklib ?

All Andoird tests are failing now:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //src/test/java/com/google/devtools/build/android:AndroidDataWriterTest
-----------------------------------------------------------------------------
JUnit4 Test Runner
.......E..
Time: 0.527
There was 1 failure:
1) writeResourceXmlWithQualfiers(com.google.devtools.build.android.AndroidDataWriterTest)
java.lang.NoSuchFieldError: JAVA_LETTER
        at com.android.ide.common.resources.configuration.LocaleQualifier.isValidAlpha2Code(LocaleQualifier.java:105)
        at com.android.ide.common.resources.configuration.LocaleQualifier.getQualifier(LocaleQualifier.java:171)
        at com.android.ide.common.resources.configuration.LocaleQualifier.checkAndSet(LocaleQualifier.java:423)
        at com.android.ide.common.resources.configuration.FolderConfiguration.getConfigFromQualifiers(FolderConfiguration.java:233)
        at com.android.ide.common.resources.configuration.FolderConfiguration.getConfigFromQualifiers(FolderConfiguration.java:137)
        at com.google.devtools.build.android.FullyQualifiedName$Qualifiers.getQualifiers(FullyQualifiedName.java:549)
        at com.google.devtools.build.android.FullyQualifiedName$Qualifiers.lambda$parseFrom$0(FullyQualifiedName.java:525)
        at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1705)
        at com.google.devtools.build.android.FullyQualifiedName$Qualifiers.parseFrom(FullyQualifiedName.java:524)
        at com.google.devtools.build.android.ParsedAndroidData$ResourceFileVisitor.preVisitDirectory(ParsedAndroidData.java:305)
        at com.google.devtools.build.android.ParsedAndroidData$ResourceFileVisitor.preVisitDirectory(ParsedAndroidData.java:273)
        at java.base/java.nio.file.Files.walkFileTree(Files.java:2731)
        at com.google.devtools.build.android.ParsedAndroidData$ParsedAndroidDataBuildingPathWalker.walkResources(ParsedAndroidData.java:232)
        at com.google.devtools.build.android.UnvalidatedAndroidDirectories.walk(UnvalidatedAndroidDirectories.java:77)
        at com.google.devtools.build.android.ParsedAndroidData.from(ParsedAndroidData.java:376)
        at com.google.devtools.build.android.AndroidDataBuilder.buildParsed(AndroidDataBuilder.java:157)
        at com.google.devtools.build.android.AndroidDataWriterTest.writeResourceXmlWithQualfiers(AndroidDataWriterTest.java:200)

FAILURES!!!
Tests run: 9,  Failures: 1

@aiuto aiuto added team-Android Issues for Android team team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website labels May 19, 2020
@aiuto aiuto requested review from philwo and jin May 19, 2020 02:22
Copy link
Contributor

@aiuto aiuto left a comment

Choose a reason for hiding this comment

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

Is there any way we can do this to a version that does not have SNAPSHOT in the name. A fixed version like is 2.3.4 preferable.

@jin
Copy link
Member

jin commented May 19, 2020

Please see #10394 (comment)

The fastest way to update android_common is actually to patch it. Fully upgrading the JAR will take weeks/months to finish due to internal blockers.

cc @ahumesky

@davido
Copy link
Contributor Author

davido commented May 19, 2020

Please see #10394 (comment)

The fastest way to update android_common is actually to patch it. Fully upgrading the JAR will take weeks/months to finish due to internal blockers.

I am not a big friend of patching third party dependencies...

Weeks or months to upgrade major external dependency is fine for such complex project as Bazel. Check Jetty upgrade in GWT: it took me years and dozen of transitive dependencies must be updated: [1].

Can you comment what has to be done to bump android_common/com.android.tools_sdk-common_25.0.0 to a newer release? Which one exactly?

[1] https://gwt-review.googlesource.com/c/gwt/+/7857

@davido
Copy link
Contributor Author

davido commented May 19, 2020

Is there any way we can do this to a version that does not have SNAPSHOT in the name. A fixed version like is 2.3.4 preferable.

This is still waiting for Error Prone project to review my PR to fix this issue, where I demoted two checks to warning severity and SNPASHOT release that I am upgrading here, is included that PR.

In that way the upgrade to newer Error Prone release is not breaking any more, and we don't have to protect it with --incompatible_use_java_tools_release_X. See discussion here.

Upgrade of Error Prone to a non SNAPSHOT release is in another PR that is pending for review. Though it will break all Bazel users who rely on cross compilation. I fixed two breakages in rules_closure and rules_kotlin, but there would be definitely more breakages in the wild.

@cushon
Copy link
Contributor

cushon commented May 21, 2020

Is there any way we can do this to a version that does not have SNAPSHOT in the name.

The open issues have been fixed / have pending fixes, I will cut a non-SNAPSHOT Error Prone release once Bazel's CI is clean with Error Prone at head.

@davido
Copy link
Contributor Author

davido commented May 21, 2020

@cushon Since google/error-prone@b81bf44, EP is using shaded checkerframework distribution that also includes dependency on:

  • checker_framework_annotations
  • checker_framework_javacutil

However, Bazel is still using dedicated artifacts for the transitive dependencies of the above libraries. Moreover, dataflow-shaded-3.1.2-sources.jar doesn't include the source for the above transitive dependencies, so that they would have to be pulled in addition to the dataflow-shaded sources. That would complicate things with EP integration in Bazel.

Moreover, yet another new dependency was added to EP 2.3.5: threeten-extra-1.5.0.jar, after caffeine-cache that was already added in 2.3.4.

´

@davido davido force-pushed the bump_error_prone_version_to_2.3.5-snapshot branch from 2959c1f to 2f2f25b Compare May 27, 2020 19:56
@davido davido force-pushed the bump_error_prone_version_to_2.3.5-snapshot branch 2 times, most recently from a20a9e6 to 870a19b Compare May 27, 2020 21:19
@davido
Copy link
Contributor Author

davido commented May 27, 2020

@cushon

The open issues have been fixed / have pending fixes, I will cut a non-SNAPSHOT Error Prone release once Bazel's CI is clean with Error Prone at head.

It's now the case (after downgrading Guava again not to conflict with Android SDK): Bazel CI is clean with Error Prone at head. After demoting of problematic bug pattern to severity level warning, there is no known backwards compatibility issues.

When 2.4.0 is released, I will update that PR and replace 2.3.5-SNAPSHOT jars with 2.4.0 jars.

@davido
Copy link
Contributor Author

davido commented May 28, 2020

@aiuto Can you please update the reviewer list? team-Android can be removed now, because after Guava library downgrade there is no conflicts any more as can be seen that the CI is green, Yay!

As pointed out by @philwo in Release 3.2 issue:

Bazel 3.3.0rc1 should be cut beginning of next week[...]

@lberki Who supposed to review/merge this PR and as the follow-up conduct new java_tools release to be included in the upcoming Bazel release (similarly to what I did in my custom release for Linux here: [1])?

As you might know, the upcoming Error Prone 2.4.0: [2] fixed many bugs, added new checks, demoted backwards incompatible checks to warnings severity level only (thanks @cushon!) and added support for current JDK 14.

[1] https://github.com/davido/java_tools/releases/tag/v14.0
[2] google/error-prone#1639

@aiuto
Copy link
Contributor

aiuto commented May 28, 2020

@philwo is still the right lead reviewer. His team owns the build process and third party dependencies.

As far as updating java_tools: to be honest, it's not clear who owns that problem. I think @philwo and @lberki have to work that out between them.

@aiuto aiuto added area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling team-Rules-Java Issues for Java rules and removed team-Android Issues for Android team team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website labels May 28, 2020
@philwo
Copy link
Member

philwo commented May 29, 2020

If tests pass and @cushon says this is fine to merge, it's LGTM from my side.

Do I understand the last comment that we have to wait for Error Prone 2.4.0 and then change the version in this PR before I should merge it?

@davido
Copy link
Contributor Author

davido commented May 29, 2020

Do I understand the last comment that we have to wait for Error Prone 2.4.0 and then change the version in this PR before I should merge it?

Yes, exactly. We are still waiting here until Error Prone 2.4.0 release is conducted. Last comment in the linked issue is:

As a quick update, I found a few crashes while validating the release, and
I'm preparing fixes for those. I'll cut the 2.4.0 release as soon as those fixes land.

Once, EP 2.4.0 is released, I will replace the JARs in this PR and will mark this PR as "ready for review" (it's currently draft).

However, even if this PR is merged, we still need to conduct new java_tools release with bumped EP version included! So that this still raises the question: who can help conduct new java_tools release? Last time @iirina and @meisterT released new java_tools versions. I can look into uploading a PR to kick the CI pipeline, but not sure, whether or not I have the ACL to do that?

@philwo
Copy link
Member

philwo commented May 29, 2020

So that this still raises the question: who can help conduct new java_tools release?

I'll figure out how to do it, when we're at that step in the process. :)

@davido
Copy link
Contributor Author

davido commented May 29, 2020

So that this still raises the question: who can help conduct new java_tools release?

I'll figure out how to do it, when we're at that step in the process. :)

The future could be faster than you think, if @cushon will release EP 2.4.0 in the next days, and I will update this PR couple of hours later, and you would merge the new version of this PR ASAP, then we would be at that step in the process, so may be you can launch the second stage of the rocket already now? ;-)

@cushon
Copy link
Contributor

cushon commented May 29, 2020

@davido Error Prone 2.4.0 is ready to go

@davido davido changed the title Bump error prone version to 2.3.5 snapshot Bump error prone version to 2.4.0 Jun 2, 2020
@davido davido marked this pull request as ready for review June 2, 2020 05:45
@davido
Copy link
Contributor Author

davido commented Jun 2, 2020

@philwo @cushon I have upgraded EP to release 2.4.0. PTAL.

@davido
Copy link
Contributor Author

davido commented Jun 2, 2020

One test seems to be failing on Mac Os X. All is fine here on Linux:

$ bazel test //src/test/java/com/google/devtools/build/lib/skyframe:SkyframeTests
DEBUG: /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/external/bazel_toolchains/rules/rbe_repo/version_check.bzl:68:14: 
Current running Bazel is ahead of bazel-toolchains repo. Please update your pin to bazel-toolchains repo in your WORKSPACE file.
DEBUG: /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:103:14: rbe_ubuntu1804_java11 not using checked in configs as detect_java_home was set to True 
DEBUG: /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/external/bazel_toolchains/rules/rbe_repo/version_check.bzl:68:14: 
Current running Bazel is ahead of bazel-toolchains repo. Please update your pin to bazel-toolchains repo in your WORKSPACE file.
DEBUG: /home/davido/.cache/bazel/_bazel_davido/4596c3304267e4c44bca87d41e038f29/external/bazel_toolchains/rules/rbe_repo/checked_in.bzl:103:14: rbe_ubuntu1604_java8 not using checked in configs as detect_java_home was set to True 
INFO: Analyzed target //src/test/java/com/google/devtools/build/lib/skyframe:SkyframeTests (37 packages loaded, 1254 targets configured).
INFO: Found 1 test target...
Target //src/test/java/com/google/devtools/build/lib/skyframe:SkyframeTests up-to-date:
  bazel-bin/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeTests.jar
  bazel-bin/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeTests
INFO: Elapsed time: 122.546s, Critical Path: 71.08s
INFO: 127 processes: 81 linux-sandbox, 46 worker.
INFO: Build completed successfully, 131 total actions
//src/test/java/com/google/devtools/build/lib/skyframe:SkyframeTests     PASSED in 46.6s
  Stats over 20 runs: max = 46.6s, min = 19.8s, avg = 34.9s, dev = 7.3s

Executed 1 out of 1 test: 1 test passes.
INFO: Build completed successfully, 131 total actions

davido added 3 commits June 2, 2020 08:14
Fixes bazelbuild#11272 (first attempt that was reverted: bazelbuild#9286).
Fixes: bazelbuild#11272.

This change updates error prone to 2.3.5 SNAPSHOT and includes demotion
of breaking changes to warning severity: ExtendsAutoValue and
RefersToDaggerCodegen. Given that Bazel EP integration does not activate
warning checks, this upgrade is backwards compatible and must not be
protected with new --incompatible_use_error_prone_2.3.5_version option.

Additional library is needed: threeten-extra-1.5.0.
@davido davido force-pushed the bump_error_prone_version_to_2.3.5-snapshot branch from 351c143 to d6a3566 Compare June 2, 2020 06:15
@davido
Copy link
Contributor Author

davido commented Jun 2, 2020

I have rebased the whole series on top of master, let's see if it helps with failing //src/test/java/com/google/devtools/build/lib/skyframe:SkyframeTests test.

@meteorcloudy @laurentlb Can you shed some light why skyframe:SkyframeTests was failing on Mac Os X?

@philwo
Copy link
Member

philwo commented Jun 2, 2020

@davido That test is flaky and it’s deactivated at head. By rebasing your PR should now also have picked up the commit that disables the test. 👍

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@cushon
Copy link
Contributor

cushon commented Jun 22, 2020

Is this blocked on anything, or can we merge it?

@davido
Copy link
Contributor Author

davido commented Jun 22, 2020

It would be great if this could be merged. The follow-up work, e.g. adding native support for JDK 14 toolchain depends on new Error Prone version.

@lberki
Copy link
Contributor

lberki commented Jun 24, 2020

@philwo , are there any blockers left?

@philwo
Copy link
Member

philwo commented Jun 24, 2020

Sorry, the only remaining blocker was finding time on my side :( I'll get to this tomorrow, promise!

@davido
Copy link
Contributor Author

davido commented Jun 24, 2020

I'll get to this tomorrow, promise!

@philwo Thanks. It would be great, to ship new verson of java_tools with new and shiny EP 2.4.0 included in the next bazel release: 3.4 or even 4.0.

That means, however, that only merging this PR doesn't help much. What must happen as follow-up is to conduct new java_tools release and include it in upcoming Bazel release.

@philwo
Copy link
Member

philwo commented Jun 24, 2020

@davido Yes, no problem. 👍 I'll do this and ensure that it's all done in time for the next release cut :)

@philwo
Copy link
Member

philwo commented Jun 29, 2020

I have verified all sha256sums of the JARs added in this PR against freshly downloaded copies from Maven:

$ sha256sum -c <<EOF
b1107a4097be4515a923a55bc63da04e411e6964aa045bece1bc7acb84872ed7 *third_party/caffeine/caffeine-2.8.0.jar
cc3f80b7723022dc5976724e57baa623d2fcaf55ee9c46c83e1198d30ef2714e *third_party/checker_framework_annotations/checker-qual-3.2.0-sources.jar
ad1b38e543ed167695ce0b1b30da473993e53e7a52b8d4e38cbfe6b6c74fbd77 *third_party/checker_framework_annotations/checker-qual-3.2.0.jar
99ebf17ed3b3805d7d6ca05dbb1d5a099bdb934a1d4a2751b1cdd0c724c20abb *third_party/checker_framework_dataflow/dataflow-shaded-3.1.2-sources.jar
3c6a0cb60a8567880a81978908f092d38769098a8c4fa52329e0e16a48e41f28 *third_party/checker_framework_dataflow/dataflow-shaded-3.1.2.jar
91b75c7947301ff1976747c3203e34bc4cc0387d98a907ee9b38c46ea513f225 *third_party/checker_framework_javacutil/javacutil-3.2.0-sources.jar
da46ef0b69d8f6448d19fc4e9fb4a1a263200d1dba03922ae8b90dcc4098e410 *third_party/checker_framework_javacutil/javacutil-3.2.0.jar
e2421bdf5be3a0c3ab95db74e0615c5460e0798121f66e1e6474d753d41e1b2b *third_party/error_prone/error_prone_annotation-2.4.0.jar
5f2a0648230a662e8be049df308d583d7369f13af683e44ddf5829b6d741a228 *third_party/error_prone/error_prone_annotations-2.4.0.jar
6b8ef18bc92d7da055242ab702f2def5a35d33159c24d2bfdfcaa721cff66d4d *third_party/error_prone/error_prone_check_api-2.4.0.jar
65064ec40ed91a388d5b3e0e1bc6c2f17a778d0d456cd4129495fe4e0bf34978 *third_party/error_prone/error_prone_core-2.4.0.jar
936c3ff826cb6b5c30f22173c04ed34f61eef3adfc550178701e7ff34d64c8f5 *third_party/error_prone/error_prone_type_annotations-2.4.0.jar
e7def554536188fbaf8aac1a0a2f956b039cbbb5696edc3b8336c442c56ae445 *third_party/error_prone/threeten-extra-1.5.0.jar
EOF
third_party/caffeine/caffeine-2.8.0.jar: OK
third_party/checker_framework_annotations/checker-qual-3.2.0-sources.jar: OK
third_party/checker_framework_annotations/checker-qual-3.2.0.jar: OK
third_party/checker_framework_dataflow/dataflow-shaded-3.1.2-sources.jar: OK
third_party/checker_framework_dataflow/dataflow-shaded-3.1.2.jar: OK
third_party/checker_framework_javacutil/javacutil-3.2.0-sources.jar: OK
third_party/checker_framework_javacutil/javacutil-3.2.0.jar: OK
third_party/error_prone/error_prone_annotation-2.4.0.jar: OK
third_party/error_prone/error_prone_annotations-2.4.0.jar: OK
third_party/error_prone/error_prone_check_api-2.4.0.jar: OK
third_party/error_prone/error_prone_core-2.4.0.jar: OK
third_party/error_prone/error_prone_type_annotations-2.4.0.jar: OK
third_party/error_prone/threeten-extra-1.5.0.jar: OK

Merging this now.

@philwo
Copy link
Member

philwo commented Jun 29, 2020

Merged in 239b2aa.

@philwo philwo closed this Jun 29, 2020
@davido
Copy link
Contributor Author

davido commented Jun 29, 2020

Thanks @philwo!

Can we start the next step and kick off java_tools pipeline from Bazel@HEAD?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants