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 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
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Jul 24, 2024
1 parent 5041fbe commit 2509793
Show file tree
Hide file tree
Showing 11 changed files with 283 additions and 188 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
10 changes: 7 additions & 3 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 Expand Up @@ -98,9 +105,6 @@
</instructions>
</configuration>
</plugin>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
<plugin>
<artifactId>maven-source-plugin</artifactId>
</plugin>
Expand Down
200 changes: 109 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,6 @@
<additionalOption>-Xdoclint:-html</additionalOption>
</additionalOptions>
<linksource>true</linksource>
<source>${java.specification.version}</source>
<additionalJOption>${maven-javadoc-plugin.additionalJOptions}</additionalJOption>
</configuration>
<executions>
Expand All @@ -251,8 +349,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 +496,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 2509793

Please sign in to comment.