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

Update Guava to 31.1. #14942

Closed
wants to merge 9 commits into from
Closed

Update Guava to 31.1. #14942

wants to merge 9 commits into from

Conversation

haxorz
Copy link
Contributor

@haxorz haxorz commented Mar 2, 2022

Main motivation is to be able to use the new
ImmutableMap.Builder#buildKeepingLast.

Jars were downloded from Maven Central:

This is the same as commit 5d55643, but I'm doing it as a GitHub PR
rather than a Gerrit change, so that way we can run it through the CI
pipeline and figure out why it didn't work (5d55643 was reverted in
b9bfde8).

haxorz added 8 commits March 2, 2022 17:26
Main motivation is to be able to use the new
`ImmutableMap.Builder#buildKeepingLast`.

Jars were downloded from Maven Central:
* https://repo1.maven.org/maven2/com/google/guava/guava/31.1-jre/
* https://repo1.maven.org/maven2/com/google/guava/guava-testlib/31.1-jre/

This is the same as commit 5d55643, but I'm doing it as a GitHub PR
rather than a Gerrit change, so that way we can run it through the CI
pipeline and figure out why it didn't work (5d55643 was reverted in
b9bfde8).
Main motivation is to be able to use the new
`ImmutableMap.Builder#buildKeepingLast`.

Jars were downloded from Maven Central:
* https://repo1.maven.org/maven2/com/google/guava/guava/31.1-jre/
* https://repo1.maven.org/maven2/com/google/guava/guava-testlib/31.1-jre/

This is the same as commit 5d55643, but I'm doing it as a GitHub PR
rather than a Gerrit change, so that way we can run it through the CI
pipeline and figure out why it didn't work (5d55643 was reverted in
b9bfde8).
//src/test/java/com/google/devtools/build/android/desugar:desugar_testdata_by_desugaring_try_with_resources
works now.

The issue is `//third_party:guava-jars` now has more than out output.

I can't try this out locally because I don't have an Android SDK installed.
@haxorz
Copy link
Contributor Author

haxorz commented Mar 3, 2022

Hi Alex and Kevin. Looks like some GitHub automation added you because I modified src/test/java/com/google/devtools/build/android/desugar/BUILD.

Just a heads up that this PR isn't complete yet. If my modification to that BUILD file works out, then I need to modify all other consumers of //third_party:guava-jars.

@meteorcloudy meteorcloudy self-requested a review March 3, 2022 09:17
@@ -499,7 +499,10 @@ distrib_java_import(
# For bootstrapping JavaBuilder
distrib_jar_filegroup(
name = "guava-jars",
srcs = ["guava/guava-31.0.1-jre.jar"],
srcs = [
"guava/failureaccess-1.0.1.jar",
Copy link
Member

Choose a reason for hiding this comment

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

You probably need to split this into two filegroups

filegroup(
    name = "guava-jar",
    srcs = ["guava/guava-31.1-jre.jar"],
)

filegroup(
    name = "guava-failureaccess-jar",
    srcs = ["guava/guava-31.1-jre.jar"],
)

distrib_jar_filegroup(
    name = "guava-jars",
    srcs = [
        ":guava-jar",
        ":guava-failureaccess-jar",
    ],
    enable_distributions = ["debian"],
)

Then you can add them separately above:

"--classpath_entry $(location //third_party:guava-jar) " +
"--classpath_entry $(location //third_party:guava-failureaccess-jar) " +

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Experimentally, it seems like it's not necessary to do literally that. 6fbe89e passes CI. Is that commit fine? And do I really need to have used distrib_jar_filegroup for the new //third_party:guava-failureaccess-jar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, if srcs in :guava-jars has more than one element then we run into the issue mentioned in b98be959.

Copy link
Member

Choose a reason for hiding this comment

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

I see, it looks good. Using distrib_jar_filegroup is for the Bazel Debian build to switch to system installed libraries easily (eg. /usr/share/java/guava.jar), it's better to keep this pattern in case we want to package a new Bazel for Debian.

`//src/test/java/com/google/devtools/build/android/desugar:desugar_guava_at_head`
working.

Let's see if CI likes this. If so, I'll explain my understanding of
what's going on here based on the discussion with coworkers today.
@haxorz
Copy link
Contributor Author

haxorz commented Mar 3, 2022

Okay, 6fbe89e passed CI.

@meteorcloudy Is the restriction mentioned in #14357 (comment) still a thing? If so, I will need to do 3 PRs: One to add a new //third_party:guava-failureaccess-jar target to third_party/BUILD, one to preemptively use it in //src/test/java/com/google/devtools/build/android/desugar:desugar_guava_at_head, and a final one to update the jar files used by //third_party:guava-jars and //third_party:guava-testlib. Then I will do 2 more PRs after that (one to update compile.sh and one to delete the old Guava jars). Please confirm this is what I need to do. Then I will add a comment explaining the issue 5d55643 tickled.

@meteorcloudy
Copy link
Member

Yes, #14357 (comment) is still true, what you described sounds good to me!

@haxorz
Copy link
Contributor Author

haxorz commented Mar 4, 2022

[me] Then I will add a comment explaining the issue 5d55643 tickled.

@cushon and @michajlo figured this out. Guava 31.1 newly contains google/guava@46093e2. They verified that code causes the Desugaring step at

to think about class InternalFutureFailureAccess, whereas it previously didn't need that class. So we now need to make sure Desugar has the jar containing that class on its classpath.

@haxorz
Copy link
Contributor Author

haxorz commented Mar 4, 2022

My sequence of changes needs to involve 5 commits. @katre advised me to do 5 separate PRs, so that he can vet and merge each one separately. So I'll close this one and then mail those out.

@haxorz haxorz closed this Mar 4, 2022
haxorz added a commit that referenced this pull request Mar 4, 2022
haxorz added a commit that referenced this pull request Mar 4, 2022
`//src/test/java/com/google/devtools/build/android/desugar:desugar_guava_at_head`.

This is commit 2/5 for
#14942 (comment).
haxorz added a commit that referenced this pull request Mar 4, 2022
`//src/test/java/com/google/devtools/build/android/desugar:desugar_guava_at_head`.

This is commit 2/5 for
#14942 (comment).
haxorz added a commit that referenced this pull request Mar 4, 2022
Main motivation is to be able to use the new
`ImmutableMap.Builder#buildKeepingLast`.

Jars were downloded from Maven Central:
* https://repo1.maven.org/maven2/com/google/guava/guava/31.1-jre/
* https://repo1.maven.org/maven2/com/google/guava/guava-testlib/31.1-jre/

This is commit 3/5 for
#14942 (comment).
haxorz added a commit that referenced this pull request Mar 4, 2022
Main motivation is to be able to use the new
`ImmutableMap.Builder#buildKeepingLast`.

Jars were downloded from Maven Central:
* https://repo1.maven.org/maven2/com/google/guava/guava/31.1-jre/
* https://repo1.maven.org/maven2/com/google/guava/guava-testlib/31.1-jre/

This is commit 3/5 for
#14942 (comment).
bazel-io pushed a commit that referenced this pull request Mar 4, 2022
bazel-io pushed a commit that referenced this pull request Mar 4, 2022
…st/java/com/google/devtools/build/android/desugar:desugar_guava_at_head`.

This is commit 2/5 for
#14942 (comment).

Closes #14961.

PiperOrigin-RevId: 432507064
bazel-io pushed a commit that referenced this pull request Mar 4, 2022
Main motivation is to be able to use the new
`ImmutableMap.Builder#buildKeepingLast`.

Jars were downloded from Maven Central:
* https://repo1.maven.org/maven2/com/google/guava/guava/31.1-jre/
* https://repo1.maven.org/maven2/com/google/guava/guava-testlib/31.1-jre/

This is commit 3/5 for
#14942 (comment).

Closes #14962.
bazel-io pushed a commit that referenced this pull request Mar 4, 2022
This is commit 4/5 for
#14942 (comment).

Closes #14963.

PiperOrigin-RevId: 432530027
@haxorz haxorz deleted the guava-31.1 branch March 7, 2022 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants