Hard-code Java versions for plugins other than maven-surefire-plugin.

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
google · Jul 24, 2024 · e710f66 · e710f66
1 parent 5041fbe
commit e710f66
Show file tree

Hide file tree

Showing 11 changed files with 285 additions and 188 deletions.
diff --git a/android/guava-testlib/pom.xml b/android/guava-testlib/pom.xml
@@ -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>

diff --git a/android/guava-tests/pom.xml b/android/guava-tests/pom.xml
@@ -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>

diff --git a/android/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java b/android/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java
@@ -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.

diff --git a/android/guava/pom.xml b/android/guava/pom.xml
@@ -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>
@@ -98,9 +105,6 @@
           </instructions>
         </configuration>
       </plugin>
-      <plugin>
-        <artifactId>maven-compiler-plugin</artifactId>
-      </plugin>
       <plugin>
         <artifactId>maven-source-plugin</artifactId>
       </plugin>

diff --git a/android/pom.xml b/android/pom.xml
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>
@@ -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>

diff --git a/guava-gwt/pom.xml b/guava-gwt/pom.xml
@@ -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>

diff --git a/guava-testlib/pom.xml b/guava-testlib/pom.xml
@@ -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>

diff --git a/guava-tests/pom.xml b/guava-tests/pom.xml
@@ -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>

diff --git a/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java b/guava-tests/test/com/google/common/collect/WriteReplaceOverridesTest.java
@@ -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.