Skip to content

Commit

Permalink
Hard-code Java versions for plugins other than maven-surefire-plugin.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Jul 24, 2024
1 parent 5041fbe commit 57139b0
Show file tree
Hide file tree
Showing 11 changed files with 285 additions and 182 deletions.
7 changes: 7 additions & 0 deletions android/guava-testlib/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
Expand Down
7 changes: 7 additions & 0 deletions android/guava-tests/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
Expand Down
8 changes: 8 additions & 0 deletions android/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ public void testClassesHaveOverrides() throws Exception {
* well be a JDK bug.
*/
|| info.getName().contains("TypeTokenTest")
/*
* "IllegalAccess tried to access class
* com.google.common.collect.testing.AbstractIteratorTester from class
* com.google.common.collect.MultimapsTest"
*
* ...when we build with JDK 22 and run under JDK 8.
*/
|| info.getName().contains("MultimapsTest")
/*
* Luckily, we don't care about analyzing tests at all. We'd skip them all if we could do so
* trivially, but it's enough to skip these ones.
Expand Down
7 changes: 7 additions & 0 deletions android/guava/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,13 @@
</resource>
</resources>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<configuration>
Expand Down
201 changes: 110 additions & 91 deletions android/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,19 @@
<description>Parent for guava artifacts</description>
<url>https://github.com/google/guava</url>
<properties>
<!--
We could override this to change which version we run tests under.
However, I think that our -Djava.security.manager=allow profile is based on the *Maven* JDK.
If we want to use overrides here, we should change that profile to also be based on surefire.toolchain.version.
-->
<surefire.toolchain.version>${java.specification.version}</surefire.toolchain.version>
<!-- Override this with -Dtest.include="**/SomeTest.java" on the CLI -->
<test.include>%regex[.*.class]</test.include>
<truth.version>1.4.4</truth.version>
<jsr305.version>3.0.2</jsr305.version>
<checker.version>3.43.0</checker.version>
<errorprone.version>2.28.0</errorprone.version>
<j2objc.version>3.0.0</j2objc.version>
<javac.version>9+181-r4173-1</javac.version>
<!-- Empty for all JDKs but 9-12 -->
<maven-javadoc-plugin.additionalJOptions></maven-javadoc-plugin.additionalJOptions>
<project.build.outputTimestamp>2024-01-02T00:00:00Z</project.build.outputTimestamp>
Expand Down Expand Up @@ -122,7 +127,7 @@
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
<version>3.13.0</version>
<configuration>
<source>1.8</source>
<target>1.8</target>
Expand All @@ -139,7 +144,32 @@
<arg>doesnotexist</arg>
<!-- https://errorprone.info/docs/installation#maven -->
<arg>-XDcompilePolicy=simple</arg>
<!-- -Xplugin:ErrorProne is set conditionally by a profile. -->

<!-- https://errorprone.info/docs/installation#maven -->
<!-- TODO(cpovirk): Enable NullArgumentForNonNullParameter for
prod code. It's disabled automatically for "test code"
(which is good: our tests have intentional violations), but
Error Prone doesn't know it's building test code unless we
pass -XepCompilingTestOnlyCode, and that argument needs to
be passed as part of the same <arg> as -Xplugin:ErrorProne,
and I gave up trying to figure out how to do that for test
compilation only. -->
<arg>-Xplugin:ErrorProne -Xep:NullArgumentForNonNullParameter:OFF -Xep:Java8ApiChecker:ERROR</arg>
<!-- https://github.com/google/error-prone/blob/f8e33bc460be82ab22256a7ef8b979d7a2cacaba/docs/installation.md#jdk-16 -->
<!-- TODO(cpovirk): Use .mvn/jvm.config instead (per
https://errorprone.info/docs/installation#maven) if it can
be made not to interfere with JDK8 or if we stop building
with JDK8. -->
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
</compilerArgs>
<annotationProcessorPaths>
<path>
Expand All @@ -157,6 +187,70 @@
<fork>true</fork>
</configuration>
</plugin>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
<version>4.5.0</version>
<executions>
<!--
We can apparently have only one <jdk> per execution: Others are silently ignored :(
To properly test this, you need to remove existing toolchains:
rm -rf ~/.m2/jdks/ ~/.m2/toolchains.xml
(But don't run that if you have put something into ~/.m2/toolchains.xml yourself.)
-->
<execution>
<id>download-11</id>
<goals>
<goal>toolchain</goal>
</goals>
<configuration>
<toolchains>
<jdk>
<version>11</version>
<vendor>zulu</vendor>
</jdk>
</toolchains>
</configuration>
</execution>
<execution>
<id>download-22-and-surefire-version</id>
<goals>
<goal>toolchain</goal>
</goals>
<configuration>
<toolchains>
<jdk>
<version>22</version>
<vendor>zulu</vendor>
</jdk>
<testJdk>
<version>${surefire.toolchain.version}</version>
<vendor>zulu</vendor>
</testJdk>
</toolchains>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
<version>3.2.0</version>
<executions>
<execution>
<goals>
<goal>toolchain</goal>
</goals>
</execution>
</executions>
<configuration>
<toolchains>
<jdk>
<version>22</version>
<vendor>zulu</vendor>
</jdk>
</toolchains>
</configuration>
</plugin>
<plugin>
<artifactId>maven-jar-plugin</artifactId>
<version>3.2.0</version>
Expand All @@ -176,7 +270,7 @@
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-io</artifactId>
<!-- DO NOT UPGRADE this past 3.4.1 until https://github.com/codehaus-plexus/plexus-io/issues/109 is fixed. -->
<!-- DO NOT UPGRADE this past 3.4.1 until https://github.com/codehaus-plexus/plexus-io/issues/109 is fixed (probably in 3.5.1). -->
<version>3.4.1</version>
</dependency>
</dependencies>
Expand Down Expand Up @@ -219,8 +313,13 @@
</plugin>
<plugin>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.5.0</version>
<version>3.8.0</version>
<configuration>
<!-- TODO: b/286965322 - Use a newer version (probably the default toolchain we set up?) if it doesn't break Javadoc (including causing trouble for our later run of JDiff, which we do outside Maven, during snapshots/releases). -->
<jdkToolchain>
<version>11</version>
<vendor>zulu</vendor>
</jdkToolchain>
<quiet>true</quiet>
<notimestamp>true</notimestamp>
<encoding>UTF-8</encoding>
Expand All @@ -231,7 +330,7 @@
<additionalOption>-Xdoclint:-html</additionalOption>
</additionalOptions>
<linksource>true</linksource>
<source>${java.specification.version}</source>
<source>8</source>
<additionalJOption>${maven-javadoc-plugin.additionalJOptions}</additionalJOption>
</configuration>
<executions>
Expand All @@ -251,8 +350,12 @@
</plugin>
<plugin>
<artifactId>maven-surefire-plugin</artifactId>
<version>2.7.2</version>
<version>3.3.1</version>
<configuration>
<jdkToolchain>
<version>${surefire.toolchain.version}</version>
<vendor>zulu</vendor>
</jdkToolchain>
<includes>
<include>${test.include}</include>
</includes>
Expand Down Expand Up @@ -394,90 +497,6 @@
</test.add.opens>
</properties>
</profile>
<profile>
<id>javac9-for-jdk8</id>
<activation>
<jdk>1.8</jdk>
</activation>
<build>
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<!-- Under JDK8, we continue to use errorprone's javac9 (even
though we don't run Error Prone itself, which no longer
supports JDK8!).
Why? At some point, presumably after
https://github.com/google/guava/commit/e06a8cec65815599e510d7f9c1ea9d2a8eaa438a,
builds with JDK8 began failing animal-sniffer with the error:
Failed to check signatures: Bad class file .../CollectionFuture$ListFuture.class
One way of dealing with that would be to disable
animal-sniffer. And that would be fine for our -jre builds:
If we're building with JDK8, then clearly we're sticking to
JDK8 APIs. However, I assume (but did not confirm) that we'd
have the same issue with our -android builds, which need
animal-sniffer so that they can check that we're sticking to
JDK6-like APIs.
So instead, we use javac9, which doesn't lead to this error.
-->
<compilerArgs combine.children="append">
<arg>-J-Xbootclasspath/p:${settings.localRepository}/com/google/errorprone/javac/${javac.version}/javac-${javac.version}.jar</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>
</profile>
<profile>
<id>run-error-prone</id>
<activation>
<!--
Error Prone requires 11+: https://errorprone.info/docs/installation
We skip 12-15 because of https://github.com/google/error-prone/issues/3540.
-->
<jdk>[11,12),[16,)</jdk>
</activation>
<build>
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<compilerArgs combine.children="append">
<!-- https://errorprone.info/docs/installation#maven -->
<!-- TODO(cpovirk): Enable NullArgumentForNonNullParameter for
prod code. It's disabled automatically for "test code"
(which is good: our tests have intentional violations), but
Error Prone doesn't know it's building test code unless we
pass -XepCompilingTestOnlyCode, and that argument needs to
be passed as part of the same <arg> as -Xplugin:ErrorProne,
and I gave up trying to figure out how to do that for test
compilation only. -->
<arg>-Xplugin:ErrorProne -Xep:NullArgumentForNonNullParameter:OFF -Xep:Java8ApiChecker:ERROR</arg>
<!-- https://github.com/google/error-prone/blob/f8e33bc460be82ab22256a7ef8b979d7a2cacaba/docs/installation.md#jdk-16 -->
<!-- TODO(cpovirk): Use .mvn/jvm.config instead (per
https://errorprone.info/docs/installation#maven) if it can
be made not to interfere with JDK8 or if we stop building
with JDK8. -->
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED</arg>
<arg>-J--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED</arg>
<arg>-J--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>
</profile>
<profile>
<id>javac-for-jvm18plus</id>
<activation>
Expand Down
7 changes: 7 additions & 0 deletions guava-gwt/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,13 @@
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<configuration>
Expand Down
7 changes: 7 additions & 0 deletions guava-testlib/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
Expand Down
7 changes: 7 additions & 0 deletions guava-tests/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,13 @@
</dependencies>
<build>
<plugins>
<plugin>
<groupId>org.mvnsearch</groupId>
<artifactId>toolchains-maven-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-toolchains-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
Expand Down
8 changes: 8 additions & 0 deletions guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,14 @@ public void testClassesHaveOverrides() throws Exception {
* well be a JDK bug.
*/
|| info.getName().contains("TypeTokenTest")
/*
* "IllegalAccess tried to access class
* com.google.common.collect.testing.AbstractIteratorTester from class
* com.google.common.collect.MultimapsTest"
*
* ...when we build with JDK 22 and run under JDK 8.
*/
|| info.getName().contains("MultimapsTest")
/*
* Luckily, we don't care about analyzing tests at all. We'd skip them all if we could do so
* trivially, but it's enough to skip these ones.
Expand Down
Loading

0 comments on commit 57139b0

Please sign in to comment.