Build with --release 8, or require builds to use JDK 8

[cpovirk]

I discovered in protocolbuffers/protobuf#7827 (comment) that Animal Sniffer does not fail the build for dangerous covariant return types. This means that, if we build a Guava release with JDK 11 (which we probably will, if we haven't already!), it will contain references that don't work under JDK 8. We do in fact have such references.

Luckily, I do not see any additional such problems in the Android branch. (Presumably our internal Android tests have detected any possible problems so far.)

We should set this up before our next release or else be VERY VERY sure to use JDK 8 to build it.

[cpovirk]

Later, assuming that the implementation of GoogleCloudPlatform/cloud-opensource-java#1605 goes in, we should consider switching to the Linkage Checker Enforcer Rule.

[cpovirk]

Ah, but the Linkage Checker will check only against the JDK that you're using to build. So we're back to needing to require builds to use JDK 8. And all the Linkage Checker would be doing then is looking at dependencies (of which we have effectively none).

[cpovirk]

--release 8 is indeed intended to catch such problems, including the specific ones we'd see with ByteBuffer: jetty/jetty.project#3244 (comment)

[cpovirk]

As I just noted on the protobuf thread, it sounds like the best option is to require builds to be done with JDK 8. (It might also be OK to build with a newer JDK but with an older bootclasspath, but I'm not sure how well supported that is, since bootclasspath is on the way out. I gather that we're internally doing something along those lines with Bazel, but I don't know how easy it is to do the equivalent externally or with Maven.)

We could consider requiring JDK 8 only for release builds. That would make life easier for an average developer who wants to contribute to Guava (and might not have JDK 8 lying around). On the other hand, it might make it more confusing to reproduce actual problems. Maybe we'd want to support some kind of -DletMeUseMyOwnJdkIPromiseIAmNotDoingARelease override flag.

Hmm, and we have Travis building with various versions.... I wonder how long it will be until openjdk8 is no longer available there.

[cpovirk]

(Certainly we at least want it to be possible to build and test with another JDK, since we want to verify that Guava works with new Java versions.)

[cpovirk]

Hmm, internally we're not going to be able to keep building with JDK 8 after our switchover. (And as established above, we can't just use --release.) So we should probably look at (a) wrapping all our calls to ByteBuffer methods that are changing and (b) writing a quick Error Prone check to ensure that we continue to do so.

Such a check, of course, would catch only the methods that we specifically program it to look for.

If we get things right internally, that should also make it possible to build with JDK 11 externally.

And building with JDK 11 is good for at least one more reason: Just as we currently use Unsafe conditionally, we'll want to use new APIs like VarHandle and @ReservedStackAccess conditionally. For that, we'll need to build with newer JDKs. It would be a shame not to have a way to make those improvements available publicly. [edit: Oh, but I guess that is what multi-release jars are for. We'd have to do some work to set them up, but it should be doable.]

We'll see if it's still straightforward to refer to Unsafe when building for JDK 11. Maybe we'll end up with a gross build setup that unpacks an old rt.jar and tries to put sun.misc.Unsafe on the classpath. Or maybe we'll end up running multiple separate javac invocations. Or maybe this will end up being easier under Bazel, since presumably external Bazel will support whatever we end up doing with internal Bazel. But if we're lucky, it will remain straightforward. [edit: Bazel still supports building with JDK8, but we might prefer not to rely on whatever build tools we use to support older JDKs for long.]

[cpovirk]

Hmm, and this is not the same thing, but when testing my fix for ByteBuffer when building with JDK 11, I noticed an error when running mvn clean install:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.0:jar (attach-docs) on project guava: MavenReportException: Error while generating Javadoc: Unable to find javadoc command: The environment variable JAVA_HOME is not correctly set. -> [Help 1]

It works fine if I set JAVA_HOME, but it's unfortunate that that's necessary. I will see if 3.2.0 works better.

[cpovirk]

(3.2.0 has the same problem.)

[cpovirk]

Ah, since we run our build and tests under JDK 11 with Travis, I guess we have evidence that that works, at least currently :)

[cpovirk]

406a4ea did enough that I'm no longer alarmed about this. Still, we'll want to do more before too long.

[cpovirk]

Some work on ClosingFuture to address https://travis-ci.org/github/google/guava/builds/726475379 reminds me that javac 8 probably has at least more bugs than javac 11 does.

[cpovirk]

We already have cases of the conditional use I described above. Specifically, we use APIs from higher than Java 6 in guava-android -- again, only conditionally, but --release 6 would still object:

guava/android/guava/src/com/google/common/util/concurrent/FuturesGetChecked.java

Line 117 in c8d0536

@IgnoreJRERequirement // getChecked falls back to another implementation if necessary

[cpovirk]

Maybe we'll end up with a gross build setup that unpacks an old rt.jar and tries to put sun.misc.Unsafe on the classpath.

• This appears to work.
• It's apparently much like what we do internally for Android builds.

[cpovirk]

We can also consider using Unsafe through a MethodHandle instead of referring to it directly in code.

That said, that is an option only for the JRE, not for Android. That said, we less urgently need --release 8 for Android because Android builds work around the Buffer problem (as noted in passing here). So maybe we could just keep using Unsafe directly in the Android branch. But wait, no, because that branch should still be usable from the JRE. (And anyway, it would be nice to avoid having diffs between how we use Unsafe in each environment, even if hidden in a package-private UnsafeAccess class that we write, tweak for each flavor of Guava, and clone to every package that needs it.)

[cpovirk]

We should set this up before our next release or else be VERY VERY sure to use JDK 8 to build it.

There are many downsides to the "be very very sure" approach, but one that's only recently occurred to me is this: Other people may build their own copies of Guava for internal use (to fulfill security requirements, to apply patches, to insert debugging printlns...). It would be nice if they didn't encounter any unpleasant surprises when they switch from a prebuilt Guava to a version that they build locally—and potentially build with a different version of javac.

(not that we are currently relying on our builds to be built with JDK 8, but the JDK developers have the right to make another ByteBuffer-like change in any future release)

[tbroyer]

Coming here from google/auto#1596

Fwiw, that "animal sniffer doesn't detect everything" issue has been known for almost a decade (and the underlying issue for much more: there's a reason why javac warns when using -source without -bootclasspath since jdk 7 😉): see https://www.cloudbees.com/blog/beware-sirens-target-call and https://www.draconianoverlord.com/2014/04/01/jdk-1.8/1.7-compatibility-gotcha.html/ (and that was the reason I was delighted by the --release addition to the JDK: now I could safely cross-compile! …of course because I don't personally need any --add-opens/--add-exports, or Unsafe 😅)

[cpovirk]

The bizarre thing about the ByteBuffer Animal Sniffer problem in particular is that the Animal Sniffer developers added code whose explicit and only purpose was to not error out when it detects this dangerous thing that it was at that point already erroring out for: If the developers had just left things alone 10 years ago, Animal Sniffer would have continued to detect the ByteBuffer problem. Aside from that, we've found Animal Sniffer to be a decent defense against -source without -bootclasspath. (If there are additional problems with it, then I would be, as you can probably guess, all ears :) I do know of at least a few.... [edit: I dug up the issue we have about this, and I see the Iterator.remove issue you linked to, which I'd forgotten about :)])

[cpovirk]

Sorry, I was a bit distracted by my Animal Sniffer grouchiness yesterday, and I didn't really speak much to your point, nor think things through a whole lot in general.

If I had any point yesterday, it would be: The combination of Animal Sniffer and an additional build with Java 8 has worked pretty well for us, except for the ByteBuffer case I was ranting about. But that's a rather large exception. And, as you say, even if Animal Sniffer were to fix the ByteBuffer case, Animal Sniffer remains insufficient on its own. (We hit the Animal Sniffer issue from the CloudBees post in Flogger, IIRC.)

For our projects that don't even use Animal Sniffer yet, I definitely would want to pursue the toolchain-based approach (rather than Animal Sniffer) if we were to decide to pursue anything at all. Our current approach in Guava (and Truth and maybe others) is Doing It Wrong (though we're likely to be OK in practice, given that we've thrown yet more testing thrown into the mix).

Your comment also reminds me that, while I found that we could add Unsafe to the classpath during a --release 8 build, I wouldn't want to assume that that's a permanent solution: Might the module-related enforcement of --release 9 and higher interfere with sticking classes into sun.misc? Maybe, maybe not. But I'd want to test before putting that approach in place. (Not that I expect to play with --release for Guava in the near future.)

[cpovirk]

We're also hearing that Android desugaring is more reliable when libraries are compiled against android.jar or at least against a relatively similar JDK. As we build against newer and newer versions of the JDK API (by using newer and newer JDKs without using --release), we are increasingly likely to see problems there—like an Android problem similar to the problem reported by Animal Sniffer in #6830.

[cpovirk]

(Ideally we'd have a test that runs D8/R8 over guava-android, too. We run such tests in our internal build, but as I noted in 464bedf, we have reason to believe that we could see different errors internally and externally, even before accounting for the differences in the internal and external copies of Guava. I wonder if that would get easier with Bazel (#2850).)

[cpovirk]

#7333 has provided yet another line of defense here: It arranges for us to always build with the same JDK version (currently 22) but to then run tests with JDK 8 and others. It is still possible for an incompatibility to sneak through if we're lacking in test coverage, but again, this is all yet another line of defense (in addition to, e.g., Animal Sniffer, which was already quite good at catching API problems, the main problems [edit: though not necessarily the only ones]), so we should have gone from "in quite good shape" to "in very good shape."

[edit: I confirmed that, if I insert calls to JDK 9+ ByteBuffer/CharBuffer methods, and if I disable Java8ApiChecker, and if I disable Animal Sniffer, then a guava-tests run with JAVA_HOME=$HOME/openjdk-8u342 (similar to what's included in our CI) gets far enough to run our tests—which then catch the problem, as desired: java.lang.NoSuchMethodError: java.nio.CharBuffer.clear()Ljava/nio/CharBuffer;, etc.]

[cpovirk]

Another thing that we've considered in the past but that I don't see discussed here is the approach that we use inside Google: We could use the javac --system flag to make JDK 22 (or whichever version) build against the libraries from JDK 8. I had previously written this off because it requires a JDK 8 to be present. But now that we download necessary JDKs automatically, we could revisit.

The tricky part might be getting the path to that JDK. It would be nice if there were some way to get the path to a given JDK toolchain as a system property, but I haven't found one so far. We presumably could use exec-maven-plugin to run a tiny Java application that merely outputs the location of the JDK to a properties file, but we'd have to get that to run under JDK 8, and exec-maven-plugin looks to probably use the default toolchain, which currently we set to 22. We could maybe set the default to 8 and then override back to 22 in the plugins that actually support overrides, though. (Or we could try writing the properties file from an annotation processor, since maven-compiler-plugin does support per-plugin toolchain selection.) If we were to get that working, then we could presumably read the JDK location from the file by using properties-maven-plugin. Of course, all this would require messing with phases and whatnot, including making sure that we compile the tiny Java application but don't include it in Guava. Maybe the easiest thing would be to write our own Maven plugin, but that would be kind of sad.

I doubt that it's worth it unless something changes. But something very well could change, perhaps with SequencedCollection. Maybe there will be a better way to pull this off by then. (Or maybe there's a better way now and I just haven't found it. Gemini is convinced that Maven sets a toolchains.jdk.home property automatically, but I can't find documentation of that, and even if it's true, I think we may still need an approach that involves 2 plugins.)

[jbduncan]

I wonder how much work we could avoid, if any, by migrating to Bazel, which I believe has support for Java toolchains. 🤔

[cpovirk]

Yeah, I wonder that from time to time. I know that, earlier on, there were relatively frequent changes in Bazel, which we weren't great about keeping up with in our few projects that do use Bazel. I imagine that that's stabilized a bit by now, and it's not like Maven has been maintenance-free, either :)

I do think we're in a fairly decent place with Maven at the moment, though I would still like to do things like:

• Upgrade the version of Javadoc that we use to generate our docs (probably easy?).
• Actually set --system for our builds (probably hard unless cushon@'s feature request is implemented).
• Switch to japicmp (possibly not that hard if I can sort out versioning questions, but requires edits to release scripts that may be annoying to test).

I don't have a great feel for whether those would be easier on net with Bazel than with Maven, but obviously we'd rather not figure them all out with Maven and then re-figure them all out with Bazel....

[cushon]

I've been asking myself similar questions about Error Prone :) It would make things like MR-JARs and --system a lot easier. I am less familiar with the current state of things like IDE support and the options for things like publishing to sonatype, but I know solutions exist.