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]

(Building with JDK8 requires our newly adopted Error Prone setup to stick with an old version of Error Prone.)

[edit: And eamonnmcmanus mentions #5653 as a problem with using older-than-necessary versions of the JDK in general.]

[stephenspol]

Issue #6167 is related to Error Prone setup.

[cpovirk]

I am not quite ready to declare absolute victory after #6334, but that helps.

I note that we don't run Error Prone (and thus Java8ApiChecker) under Java versions below 11 (#6230, #6243), but that's OK:

• When we build with JDK8, we know that we're compatible with JDK9.
• We can't build with JDK9 or JDK10 at all.

Other things that could help:

• some future-proofing of Java8ApiChecker (internal bug 270398025)
• toolchain-based testing, under which we perform all builds with a new JDK but run tests with older JDKs (Testlib: Add test that putIfAbsent replaces a null value  #6217 (comment), https://jakewharton.com/build-on-latest-java-test-through-lowest-java/)

[cpovirk]

I mentioned this in #5457 (comment) earlier today, but: Error Prone just set up testing based on Maven toolchains. As discussed above, that would be useful for us here.

[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).)