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

In particular: - Use JDK 22 for compilation (also, for any other [affected plugins](https://maven.apache.org/guides/mini/guide-using-toolchains.html#prerequisites)) 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. There are probably other simplifications that we could perform, as well, such as `maven-javadoc-plugin.additionalJOptions`. - Originally, I'd set up this CL to explicitly set only the toolchain of `maven-compiler-plugin` to 22. I had it using 11 for any other plugins (just Animal Sniffer, maybe?), I think from when I was trying to get toolchains to take effect at all. I've since changed this CL to set the _default_ toolchain to 22 while still including overrides for `maven-javadoc-plugin` and `maven-surefire-plugin`. - 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)).) Fixes #7331 RELNOTES=n/a PiperOrigin-RevId: 655647768
google · Jul 24, 2024 · 71666ca · 71666ca
1 parent 76f87bb
commit 71666ca
Show file tree

Hide file tree

Showing 11 changed files with 277 additions and 174 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>

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>
@@ -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>
@@ -221,6 +315,11 @@
           <artifactId>maven-javadoc-plugin</artifactId>
           <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>
@@ -253,6 +352,10 @@
           <artifactId>maven-surefire-plugin</artifactId>
           <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.

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