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

Require Java 11+ to *build* Guava but continue to support Java 8 at runtime #6549

Open
cpovirk opened this issue Jun 12, 2023 · 20 comments
Open
Labels
P3 package=general type=other Miscellaneous activities not covered by other type= labels

Comments

@cpovirk
Copy link
Member

cpovirk commented Jun 12, 2023

I'd mentioned this in #3990 (comment).

It would be nice to be able to refer in source code to newer types (e.g., CRC32C, VarHandle, and @ReservedStackAccess)—with, of course, appropriate runtime guards. Of course, that would be incompatible with one of the possible approaches to #3990, which is to use --release.

(Using Java 11 might also still dodge bugs in older versions of javac.)

We'd probably accept a PR for this unless some of our users want to tell us that they build Guava from source themselves with javac 8 (or 9 or 10), in which case the decision would get more complex.

@cpovirk cpovirk added type=other Miscellaneous activities not covered by other type= labels package=general P3 labels Jun 12, 2023
@cpovirk
Copy link
Member Author

cpovirk commented Jun 12, 2023

Of course, that would be incompatible with one of the possible approaches to #3990, which is to use --release.

Well, not completely: We could use --release 8 but then perform a second compilation with (e.g.) --release 11 to put a second set of classes into a multi-release jar. Even though multi-release jars don't do one of the things we might like, I think they could work well here. We could even imagine conditionally using APIs like CRC32C internally by providing an exception-throwing implementation for Java 8 in addition to a "real" implementation for newer versions and then making sure to perform appropriate checks before using the implementation class.

@ben-manes
Copy link
Contributor

@ReservedStackAccess is only available to privileged code during bootstrap and be ignored otherwise, unless -XX:-RestrictReservedStack is specified. Similarly for @Contended and its flag -XX:-RestrictContended. That unfortunately makes it useless for library code.

It may be reasonable to use Atomic*FieldUpdaters instead of Unsafe. When introduced in Java 5 they were known to have performance issues. That may be mostly resolved according to Faster Atomic*FieldUpdaters for Everyone. You might consider running a fresh set of benchmarks on those usages to see if the impact is acceptable.

I would be inclined to instead make jdk11 the minimum version. Those who are performance sensitive should not be on jdk8 and those users are likely to only need critical bug fixes to be backported. Android annoyingly only supports a subset of jdk11, but likely enough for your needs. You'll certainly have people complain but external are suffering regardless and internal are posing a security risk, so you might not have as much actionable push back if pursued.

I recall some tools were not multi-release jar friendly (e.g. if naively creating a fatjar) and since they are not widely used, I'd consider that to likely be a higher risk of unexpectedly breaking someone in a surprising way than adopting jdk11.

@cpovirk
Copy link
Member Author

cpovirk commented Jun 13, 2023

Thanks, I'm not sure whether I ever knew that about the annotations. I'll have to dig up some past threads and leave a link from them to here.

I'll also read your link about the field updaters. That's good news for someday getting off Unsafe.

As for requiring a new Java version, that is above my pay grade :) Thankfully, even our final straggler internally was moved off Java 8 a while back, but we'll likely need to maintain compatibility as long as enough open-source Google and non-Google libraries do.

Sadly, if we try to make only bug fixes to a Java 8 backport, I'd expect the same kind of version conflicts that users are used to from the days when we often removed APIs: If one of your deps updates to Guava 32.4.0, then it might use APIs that aren't available to you.

It's good to know that we should look into the fat-jar situation if we try mrjars. I think we had some trouble with that kind of thing internally, if I'm remembering right. Hopefully it's been sorted out both internally and externally, but it's worth remembering that there are risks to everything (just in case the recent CVE-Windows experience didn't remind me enough :)). And at the moment, the benefits of using Java 9+ APIs are looking small (especially after what you said about the annotations), so we will have less motivation to go with a multi-release jar (though it's still a possibility).

--

One other note that I should have put in my original post:

Also related to toolchains: #5457 That's where I'd previously mentioned google/error-prone#3895 (comment), which is the key to testing Java 8 compatibility even when building with a newer version.

@cpovirk
Copy link
Member Author

cpovirk commented Jun 13, 2023

My guess at the full set of related commits in Error Prone is:

copybara-service bot pushed a commit that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539722059
copybara-service bot pushed a commit that referenced this issue Jun 13, 2023
This is the first use of a Java 9 API in Guava, but we use the API only when it's available, so we maintain compatibility with Java 8. Use of Java 9 APIs is relevant to #6549 and #3990 (and also mojohaus/animal-sniffer#67).

I didn't make the same change for `guava-android`, which [will add `java.util.zip.CRC32C` in API Level 34](https://developer.android.com/reference/java/util/zip/CRC32C). I don't know if Android is providing similar performance improvements, so it might not even matter. But even if I wanted to do it, I can't with my current approach, which relies on `MethodHandle`—unless I want to make even the usage of `MethodHandle` conditional on a reflective check :)

RELNOTES=`hash`: Enhanced `crc32c()` to use Java's hardware-accelerated implementation where available.
PiperOrigin-RevId: 539983316
@cpovirk
Copy link
Member Author

cpovirk commented Jul 5, 2023

We might also eventually see new versions of Maven plugins drop support for Java 8, as noted in uber/NullAway#778.

In fact, even today, we have special build setup for Error Prone, which requires Java 11+ [edit: for now].

@cpovirk
Copy link
Member Author

cpovirk commented Jul 24, 2023

If any of our annotation dependencies start being built for Java 9+, then we would start to need to build with a newer javac in all cases. I speculate that the annotations targeting Java 9+ might still be OK to references from a Guava that targets Java 8, but we'd want to verify that—especially since Java 8 has a bug in its handling of type-use annotations that it can't resolve.

@cpovirk
Copy link
Member Author

cpovirk commented Sep 15, 2023

An interesting question from an internal discussion:

I wonder if multi-release annotation processor jars work.

I would like to think so, but that sounds like a good thing for us to check if we ever look seriously into publishing a multi-release jar.

@cpovirk
Copy link
Member Author

cpovirk commented Sep 15, 2023

(As I just noted in #3990 (comment), even a build-time requirement could be inconvenient for users if they build Guava themselves: In this case, it could be inconvenient if they have continued to build with an older JDK version. So, for example, if an organization both builds with JDK 8 and runs with JDK 8, then they'd no longer be able to build Guava if its build required JDK 11, even if newer compilers could produce a version of Guava that runs on JDK 8.)

@cpovirk
Copy link
Member Author

cpovirk commented Sep 26, 2023

I am in the process of trying to conditionally use a Java 9+ API again as part of #6634 (comment) :\

@cpovirk
Copy link
Member Author

cpovirk commented Oct 13, 2023

On the toolchain front: I heard a while back that JDiff breaks at JDK ~13, so we wouldn't necessarily want to build with newer versions, at least for our Javadoc generation. Or we could finally migrate to japicmp, which seems to be the replacement that everyone recommends.

[edit: But I didn't see any obvious problems when #7087 changed Javadoc+JDiff to use Java 21. I suppose it's possible that JDiff failed silently. Or maybe what I heard is that JDiff breaks on JDK 13 bytecode or something?]

[edit: Or did it fail after all?]

@cpovirk
Copy link
Member Author

cpovirk commented Oct 13, 2023

This would also be convenient (but not strictly necessary) if we want to start generating a module-info.class (#2970): That requires JDK9+ at build time (perhaps configured like this, incidentally). If we don't have JDK9+, then our options are to submit a precompiled module-info.class to the repo or to skip the module-info compilation when run under JDK8 (and then set up our release configuration to require JDK9+ (which in practice means JDK11+, which is fine, since that's what we'd be using, anyway)).

copybara-service bot pushed a commit that referenced this issue Oct 16, 2023
… to 1.0.2 to prepare for a release (which we'll then need to update Guava to use).

I chose an `Automatic-Module-Name` over an actual `module-info`, even for this dependency-free artifact, because [I can do that without requiring JDK 9+ for builds](#6549 (comment)). Granted, there would be relatively little harm in requiring JDK 9+ for `failureaccess` builds, since `failureaccess` isn't part of our normal build process. (Guava's build pulls an already released version of `failureaccess`.) Still, it's possible that someone is building both Guava _and_ `failureaccess` with JDK 8, so it may be nice not to break that workflow. Plus, I'm not sure that a proper module definition buys us much (relative to `Automatic-Module-Name`) when we have no deps? Still, I am a bit tempted, if only to try to shake out remaining issues that `module-info` might cause our users.

This CL is progress toward fixing #6776 (or "toward working around a Maven bug," if you prefer).

It's also a tiny bit of progress toward modularizing Guava (#2970), since `failureaccess` is one of its existing unmodularized dependencies.

RELNOTES=Added an `Automatic-Module-Name` to `failureaccess`, [Guava's one strong runtime dependency](https://github.com/google/guava/wiki/UseGuavaInYourBuild#what-about-guavas-own-dependencies).
PiperOrigin-RevId: 573236700
copybara-service bot pushed a commit that referenced this issue Oct 16, 2023
… to 1.0.2 to prepare for a release (which we'll then need to update Guava to use).

I chose an `Automatic-Module-Name` over an actual `module-info`, even for this dependency-free artifact, because [I can do that without requiring JDK 9+ for builds](#6549 (comment)). Granted, there would be relatively little harm in requiring JDK 9+ for `failureaccess` builds, since `failureaccess` isn't part of our normal build process. (Guava's build pulls an already released version of `failureaccess`.) Still, it's possible that someone is building both Guava _and_ `failureaccess` with JDK 8, so it may be nice not to break that workflow. Plus, I'm not sure that a proper module definition buys us much (relative to `Automatic-Module-Name`) when we have no deps? Still, I am a bit tempted, if only to try to shake out remaining issues that `module-info` might cause our users.

This CL is progress toward fixing #6776 (or "toward working around a Maven bug," if you prefer).

It's also a tiny bit of progress toward modularizing Guava (#2970), since `failureaccess` is one of its existing unmodularized dependencies.

RELNOTES=Added an `Automatic-Module-Name` to `failureaccess`, [Guava's one strong runtime dependency](https://github.com/google/guava/wiki/UseGuavaInYourBuild#what-about-guavas-own-dependencies).
PiperOrigin-RevId: 573930127
@cpovirk
Copy link
Member Author

cpovirk commented Oct 17, 2023

I just noticed that we could simplify our build configuration slightly if we could require 17+:

guava/pom.xml

Lines 339 to 345 in 280b5d2

Passes JDK 11-12-specific `no-module-directories` flag to Javadoc tool,
which is required to make symbol search work correctly in the generated
pages.
This flag does not exist on 9-10 and 13+ (https://bugs.openjdk.java.net/browse/JDK-8215582).
Consider removing it once our release and test scripts are migrated to a recent JDK (17+).

That alone is clearly not a reason to do so, but it's a reminder that sometimes upgrades solve problems. And while upgrading to JDK 13 or so would break JDiff, it would also let us use the nicer format for code snippets in our Javadoc. I think that, in particular, we could start lines with "@Override," etc. without having that interpreted as a Javadoc tag.

@cpovirk
Copy link
Member Author

cpovirk commented Nov 10, 2023

More toolchain thoughts:

  • I wonder if running our tests n times in one job will end up slower than building+running our tests n times in m jobs. We'd have to test to see.
  • Building under JDK21+ actually doesn't work right now (Set up CI for newer JDK versions #6830): Not only is there an Animal Sniffer error, but that error indicates that the built binary legitimately wouldn't work under older versions. While we could avoid releasing with JDK21+, we'd also need to avoid having our CI build with JDK21+ if it's going to then run our tests with that binary against older versions.

@cpovirk
Copy link
Member Author

cpovirk commented Mar 6, 2024

We're hitting a javac8 issue again in #7080:

[INFO] --- compiler:3.8.1:testCompile (default-testCompile) @ guava-tests ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 643 source files to /home/runner/work/guava/guava/guava-tests/target/test-classes
[INFO] -------------------------------------------------------------
Warning:  COMPILATION WARNING : 
[INFO] -------------------------------------------------------------
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[34,23] ProviderList is internal proprietary API and may be removed in a future release
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[35,23] Providers is internal proprietary API and may be removed in a future release
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[84,4] ProviderList is internal proprietary API and may be removed in a future release
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[84,29] Providers is internal proprietary API and may be removed in a future release
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[85,30] ProviderList is internal proprietary API and may be removed in a future release
Warning:  /home/runner/work/guava/guava/guava-tests/test/com/google/common/hash/MacHashFunctionTest.java:[85,4] Providers is internal proprietary API and may be removed in a future release
[INFO] 6 warnings 
[INFO] -------------------------------------------------------------
[INFO] -------------------------------------------------------------
Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  An exception has occurred in the compiler (1.8.0_402). Please file a bug against the Java compiler via the Java bug reporting page (http://bugreport.java.com/) after checking the Bug Database (http://bugs.java.com/) for duplicates. Include your program and the following diagnostic in your report. Thank you.
java.lang.AssertionError: annotation tree hasn't been attributed yet: @Nullable()
	at com.sun.tools.javac.util.Assert.error(Assert.java:133)
	at com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:118)
	at com.sun.tools.javac.comp.Check.validateTypeAnnotation(Check.java:2748)
	at com.sun.tools.javac.comp.Attr$TypeAnnotationsValidator.visitAnnotation(Attr.java:4484)
...

(https://bugs.openjdk.org/browse/JDK-8144101)

Also, I think I saw that new versions of Maven will soon require Java 17. That may require us to finally do the toolchain setup if we want to test under Java 8.

@cpovirk
Copy link
Member Author

cpovirk commented Mar 8, 2024

  • Our use of javac8 is going to complicate module-info support at least a little (by forcing us to make it conditional). [edit: Oops, I'd already covered this above.]
  • I may have mentioned this already (too lazy to check), but javac does phase out support for older versions over time, as noted in feat: add jpms definition for annotations error-prone#4311. Eventually, Java 8 may become hard to target under new versions.
    • To be clear: If we need to continue to support Java 8, then the solution to that problem is going to be to someday not build with the newest javac versions—or at least to build with them only for testing (with the target version set higher) and then be careful not to use the result for a release. As always, what we really should be doing is building with some newish version of javac and then testing under other versions by using toolchains. All that may differ here is that we might not want to use the absolute newest javac if that version lacks support for -target 8.

@cpovirk
Copy link
Member Author

cpovirk commented Mar 13, 2024

Builds with Java 11 are fine, but if we go to even newer versions (under the theory that we should build with the newest version available), it appears that we'll have problems with Javadoc. Maybe we can continue generating Javadoc snapshots with java 11, even as we perform the main build with a newer version (especially since we target Java 8 and restrict ourselves to Java 8 APIs)? (Or maybe we can, you know, make it work with new versions :) But I fear that the problem is JDiff, so it might not be easy to fix.) We might see problems with that approach if we try to use Java 12+ APIs internally, but I don't know that we'll do that anytime soon. And even if we do, we should be providing Java-8-compatible sources alongside the Java 12+ sources, so I'd hope that we can "just" point Javadoc to those sources.

@ben-manes
Copy link
Contributor

ben-manes commented Mar 13, 2024

Not sure if it helps, but I was pinged a few years ago by @lvc showing off his tool (example). Looks like an inactive project and jdiff is too, but maybe could be revived easier?

Oh, and mentioned in #2963

@cpovirk
Copy link
Member Author

cpovirk commented Mar 13, 2024

Oh, good point, thanks.

I occasionally hear of a new such tool, and I dump it into a list I have in an internal issue about JDiff. It looks like the tool that I've seen the most energy around is japicmp (used by AdoptJDK and recommended by Kotlin).

@cpovirk
Copy link
Member Author

cpovirk commented May 24, 2024

We hit one interesting quirk when building with JDK 21+ and running tests under JDK 8 this week: jspecify/jspecify#517.

That happened specifically with JUnit 5, but it could happen in other code that performs reflection. I haven't tested Guava with the 21-8 mix to see what happens here, so I'm just mentioning the problem now as something that we'd want to remember in case we encounter it when we try toolchains for Guava or our other projects.

(We still won't be able to release under JDK 21+ or even 13+, thanks to JDiff.)

@cpovirk
Copy link
Member Author

cpovirk commented Jun 10, 2024

Another note on toolchains: https://openjdk.org/jeps/471 is coming, so eventually we'll find it difficult to compile against the newest JDK and continue to refer to Unsafe.

copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592.

(See also [these notes](#5457 (comment)).)

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I assume that we'll now get the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. That seems fine. We could consider setting it to 8 to match our normal build (which I thought I had remembered was the `maven-javadoc-plugin` default, but I don't think I'm seeing that, at least not under our current versions), but I don't see much downside to 11—or even to newer versions that we might someday use for Maven Javadoc generation, given that we keep the code compiling under new versions already.

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to remove the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I assume that we'll now get the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. That seems fine. We could consider setting it to 8 to match our normal build (which I thought I had remembered was the `maven-javadoc-plugin` default, but I don't think I'm seeing that, at least not under our current versions), but I don't see much downside to 11—or even to newer versions that we might someday use for Maven Javadoc generation, given that we keep the code compiling under new versions already.

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to change the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I first tried going with the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. But that led to a problem in `org.codehaus.plexus.languages.java.jpms.CmdModuleNameExtractor`, which apparently tries to look up the module name for the `-source 11` run but uses the Maven run's JDK instead of the Javadoc toolchain or Maven toolchain. So now I've set it to 8 to match what we use for `maven-compiler-plugin`. (I _thought_ I had remembered that `maven-javadoc-plugin` defaulted to matching `maven-compiler-plugin`, even though that's weird. Maybe the two actually just read from the same Maven property or something, or maybe the behavior changed.)

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)
- I forgot the other thing while I was typing :(

(See also [these notes](#5457 (comment)).)

So

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation (also, for any other [affected plugins](https://maven.apache.org/guides/mini/guide-using-toolchains.html#prerequisites)) to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet. There are probably other simplifications that we could perform, as well, such as `maven-javadoc-plugin.additionalJOptions`.
   - Originally, I'd set up this CL to explicitly set only the toolchain of `maven-compiler-plugin` to 22. I had it using 11 for any other plugins (just Animal Sniffer, maybe?), I think from when I was trying to get toolchains to take effect at all. I've since changed this CL to set the _default_ toolchain to 22 while still including overrides for `maven-javadoc-plugin` and `maven-surefire-plugin`.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to change the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I first tried going with the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. But that led to a problem in `org.codehaus.plexus.languages.java.jpms.CmdModuleNameExtractor`, which apparently tries to look up the module name for the `-source 11` run but uses the Maven run's JDK instead of the Javadoc toolchain or Maven toolchain. So now I've set it to 8 to match what we use for `maven-compiler-plugin`. (I _thought_ I had remembered that `maven-javadoc-plugin` defaulted to matching `maven-compiler-plugin`, even though that's weird. Maybe the two actually just read from the same Maven property or something, or maybe the behavior changed.)

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)
- I forgot the other thing while I was typing :(

(See also [these notes](#5457 (comment)).)

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655592201
copybara-service bot pushed a commit that referenced this issue Jul 24, 2024
In particular:

- Use JDK 22 for compilation (also, for any other [affected plugins](https://maven.apache.org/guides/mini/guide-using-toolchains.html#prerequisites)) to [avoid a JDK 11 bug](#7331).
   - Another way to avoid that bug would be to use JDK 8, which [would also provide a `--release`-like compatibility guarantee](#3990). However, that could complicate [using newer APIs conditionally](#6549). And of course we'd expect JDK 8 to be buggier than JDK 22. (In fact, we still have a workaround or two for JDK 8 bugs (with a brand new one coming in cl/655556207), and we could now remove those—assuming that none of our users use JDK 8 to build Guava outside of our Maven build.) JDK 22 also supports new versions of Error Prone, while JDK 8 does not.
   - This change also allows us to simplify our Error Prone configuration, which until now needed different profiles in order to support both JDK 8 and JDK 9+. We could now upgrade Error Prone, but I haven't done so yet. There are probably other simplifications that we could perform, as well, such as `maven-javadoc-plugin.additionalJOptions`.
   - Originally, I'd set up this CL to explicitly set only the toolchain of `maven-compiler-plugin` to 22. I had it using 11 for any other plugins (just Animal Sniffer, maybe?), I think from when I was trying to get toolchains to take effect at all. I've since changed this CL to set the _default_ toolchain to 22 while still including overrides for `maven-javadoc-plugin` and `maven-surefire-plugin`.
- Continue to use JDK 11 for Javadoc, as [we're doing now](https://github.com/google/guava/blob/5041fbe61965a73ea269c7c24ea746d89bd1b1ba/.github/workflows/ci.yml#L89-L99) because of [problems with at least JDK 21](#7109).
   - What matters might actually be the version used [by _JDiff_](#6549 (comment)), which comes from the version in the linked `ci.yml` file. But since we're using JDK 11 currently for docs in general, I'm sticking with that for now. Still, we should consider [upgrading the version used for Javadoc generation](#6790 (comment)). But this CL is already complicated enough....
   - When we hard-code JDK 11, we need to change the `<source>${java.specification.version}</source>` line: That would otherwise set (for example) `-source 17` when running Maven under JDK 17, and JDK 11 wouldn't recognize it. As I recall, the `java.specification.version` usage was from the days in which we tried to inherit Javadoc from the JDK. Inheritance had [stopped working](#6790), and we ripped it out in cl/614693592. I first tried going with the default from the JDK whose Javadoc binary we're using, which (again) will now be 11. But that led to a problem in `org.codehaus.plexus.languages.java.jpms.CmdModuleNameExtractor`, which apparently tries to look up the module name for the `-source 11` run but uses the Maven run's JDK instead of the Javadoc toolchain or Maven toolchain. So now I've set it to 8 to match what we use for `maven-compiler-plugin`. (I _thought_ I had remembered that `maven-javadoc-plugin` defaulted to matching `maven-compiler-plugin`, even though that's weird. Maybe the two actually just read from the same Maven property or something, or maybe the behavior changed.)

Some other thing I'm wondering:

- I wonder if we should activate(?) some of the plugins, including the new toolchain plugins, in the `<plugins>` (not just `<pluginManagement>`) section of the parent `pom.xml`. Might that save us from having to do so in each separate `pom.xml`? (We might actually mostly get away with activating(?) them only in the main `guava` build: That _downloads and registers_ the toolchains, and then at least the other projects' _per-plugin_ toolchain configuration probably finds them? But for the more general configuration to work, I think we at least need to activate(?) `maven-toolchains-plugin` in each? I haven't experimented a ton with this.)
- I forgot the other thing while I was typing :(

(See also [these notes](#5457 (comment)).)

Fixes #7331

RELNOTES=n/a
PiperOrigin-RevId: 655647768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 package=general type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

No branches or pull requests

2 participants