feat: add jpms definition for annotations

[sgammon]

Summary

Minimal set of changes to ship a module-info.java, and support JVM 1.8 for the annotations module only.

Reasoning

It costs almost nothing to support only the annotations on JVM 1.8, for projects which transitively include error-prone-annotations, which can happen via a simple dependency on something like Guava. But, it would be ideal to ship a module-info.java, so annotations is now a Multi-Release JAR, with a module-info.class in META-INF/versions/9.

I am working downstream to support JPMS in Guava, and this PR will be necessary for that to eventually land (without hackery). Checker Framework recently merged their support which means Error Prone is one of the last things on the classpath for Guava without a module-info.java definition.

Automatic Modules also aren't the answer because they cannot be used with jlink. Such dependencies must be processed by tools like Moditect before they can be used in fully modular Java builds. Because Error Prone Annotations is a dependency in Guava, and is itself very popular, many projects need Error Prone to adopt JPMS so they can ship their own code with modular Java support.

There may be libraries out there that depend on the annotations and not the compiler, who wish to ship a MRJAR and retain JVM 1.8 support (Guava).

Non-release version number change

One wrinkle here is that the Maven project version is used unconditionally as the --module-version; however, HEAD-SNAPSHOT is not a valid module identifier. This leaves only a few choices to support JPMS through recent (3.8.x+) Maven Compiler plugin versions:

• HEAD-SNAPSHOT needs to become 1.0-HEAD-SNAPSHOT, and then it is a valid --module-version
• Or, Maven Compiler Plugin needs to ship a bugfix and Error Prone will need to wait for a release (if a bugfix ships at all)

I understand this can be a breaking change somewhere within Google, but I don't see a way around this without resorting to severe build hacks. I've gotten it to build anyway by building the module-info.java only in a separate Maven project, and then copying it into the JAR at the last moment, but there are serious issues with that route so I suggest it be abandoned in favor of a version string change during dev, if possible.

Changelog

• feat: add module-info to annotations module
• feat: ship annotations as a Multi-Release JAR
• feat: support 1.8 through latest JDK for annotations module
• fix: remove automatic module definition from annotations module
• fix: HEAD-SNAPSHOT → 1.0-HEAD-SNAPSHOT, because of a Maven Compiler Plugin issue precluding use as a module version

Fixes and closes #2649

[sgammon]

cc / @cushon (hi again!), and @cpovirk

[sgammon]

Module versions must begin with a number, so the dev version has been updated to 1.0-HEAD-SNAPSHOT.

[sgammon]

Expressed as an open module to enable reflective introspection of annotations.

[sgammon]

Configures default compile at JDK 8.

[sgammon]

Extra execution covers JDK 9+.

[sgammon]

Avoids some warnings during the build about JDK 8 being removed eventually.

[cushon]

Thanks for the contribution, and for walking me through all of the changes. This looks great to me overall.

I'm curious if @cpovirk has any notes, but will otherwise plan on trying to get this merged soon and included in a future Error Prone release.

[cpovirk]

Thanks!

• I'm pretty sure we made peace with the idea that Guava has dependencies on annotations that require Java 11, since the Java runtime is usually willing to tolerate annotations that it can't load. (I want to say that the recent problems I've seen with that have involved either (a) enum elements in annotation or (b) type-use annotations, maybe?) I don't think we've heard complaints, at least. Still, all else being equal, I'm happy for the annotations to support a lower version of Java so that users can see them. I just wouldn't view that as an essential if it caused problems or even just requires more complexity than we'd like.
  • [edit: hmm, maybe? I dug up Require Java 11+ to *build* Guava but continue to support Java 8 at runtime guava#6549 (comment), which points out that we also need to worry about build time, at least until we require a newer JDK to build Guava]
• Do you happen to know the tradeoffs between a top-level module-info.class and one under the Java 9+ section of the mrjar? Years ago, I know that some tools too eagerly tried to read module-info.class before they were capable of it, and I think that moving the file out of the root helped with that. Is that still an issue? Are there other issues? I ask in part because we have gone back and forth for another project that we're involved with: Move module-info back to the main root? jspecify/jspecify#484. (I get the impression that part of the story is that it can be simplest to set up with some build configs: You just build everything with a destination of META-INF/versions/9, and then you remove/omit com. There doesn't have to be a deep reason here; I'm just wondering if there is anything I might be missing.)
• I had a previous run-in with the module-version syntax in jspecify/jspecify@0d39a0e. My impression is that the problem is in javac itself (or in the spec, or in my reading of the spec :)). I tried javac just now, and I see the same error:

  $ $HOME/jdk-21.0.1+12/bin/javac --module-version HEAD-SNAPSHOT
  error: bad value for --module-version option: 'HEAD-SNAPSHOT'
  

  So this is probably not within Maven's power to fix. I could try to nudge someone to report it to OpenJDK, but I'm not sure whether it's worth it when we can't rely on the the fix to be available to the average user for a while [edit: which would be especially important if downstream users could be affected?] (and when the "fix" might be to more officially ban that format, anyway :)).
  
  The approach that I ended up taking was 0.0.0-HEAD-SNAPSHOT, which I thought signalled more clearly that this isn't actually a snapshot of a 1.0 release. But I'm not clear on how any of this affects which version is chosen by tools like Gradle. Hopefully it's academic, since users aren't using snapshots to begin with? But I've wondered if the better choice would be 999.0.0-HEAD-SNAPSHOT or something.
  
  Probably 1.0-HEAD-SNAPSHOT is fine.

[cpovirk]

(Ah, sorry, I hadn't actually read the Maven bug link that I'd opened. I see there the possibility that Maven could let a project omit a module version (or I guess provide a version separate from the project version). If so, we could keep things working with our current version number without changes in javac.)

[sgammon]

@cpovirk

I'm pretty sure we made peace with the idea that Guava has dependencies on annotations that require Java 11

Yes, and I'll admit it feels awkward lowering the target on anything. This is coming from someone who daily drives JVM 21. But while I was making this change, it occurred to me that there is nothing stopping the annotations from supporting JVM 8 still, just for a little while longer.

Maybe one more release supporting JVM 8 is a good idea, and then future releases can drop it. It makes no difference to me in my workflows, but it would help to whittle away as many non-JPMS potential issues as possible. Package exports keep me up at night but thankfully this lib is all in one place.

Do you happen to know the tradeoffs between a top-level module-info.class and one under the Java 9+ section of the mrjar?

As I understand it, module-info.class at the root is inert in Java 8 anyway, and I did consider shipping a non-MRJAR. But ultimately, I worry that there is some tool out there that doesn't follow the rules, and it will likely be an old tool that can't be patched easily. So I put it in the META-INF/versions/9 just to be safe; at JDK 8, I believe META-INF was all resources, so loading a class from there is truly illegal. That offers a slightly stronger guarantee than a root module-info.class, which is illegal, but apparently only because it is named differently.

I had a previous run-in with the module-version syntax

I'm glad you've seen this one before. I have filed an issue upstream with the Maven compiler plugin, to see if we can withhold the --module-version flag if the user provides their own, but the compiler team has been against such changes before. Ultimately I think the quickest and easiest way is to provide a compliant version string.

But I'm not clear on how any of this affects which version is chosen by tools like Gradle

Gradle in general always picks the latest version of dependencies possible within a given closure, and it is pretty smart about snapshots. For example, if you add any dependency ending in SNAPSHOT, I believe Gradle considers the dependency "dynamic" and will warn you if you try to lock to it, etc etc.

Detection happens via:

In Maven repositories, changing versions are commonly referred to as snapshot versions. Snapshot versions contain the suffix -SNAPSHOT

(docs here)

So any of the proposed versions from your comment should work fine for Gradle. I am, of course, indifferent, so long as everyone gets their modules and can eat them too.

[cpovirk]

Thanks, that all sounds good to me!

If we someday learn of a reason to prefer one number in the version over another, we can easily switch as many projects as we need to: The format of that version is almost completely an implementation detail (unless people are actually pulling snapshots from Sonatype, hopefully at least to periodically update versions in their repos instead of to actually pull in things dynamically). (Guava will probably require light changes to some of our supporting scripts, but that's a problem for another day.)

[cowwoc]

Someone promote this man! :) @sgammon thank you for all your hard work.

[sgammon]

@cpovirk Thank you for being willing to tolerate that breaking change. I'm happy to offer a PR in Guava to fix it, that's only fair!

@cowwoc Thank you! That means a lot! And thank you for filing the original issue. OSS is a team sport.

@cushon Thanks for the quick review and approval 😄

[ben-manes]

When I tried upgrading Caffeine to use 2.26.0 it fails,

> Task :caffeine:compileJava FAILED
/Users/ben/projects/caffeine/caffeine/src/main/java/module-info.java:5: error: module not found: com.google.errorprone.annotations
  requires static com.google.errorprone.annotations;

Previously this was observed through the Automatic-Module-Name. I'm not super familiar, but don't see why this should be incompatible with 2.25.0?

module com.github.benmanes.caffeine {
  exports com.github.benmanes.caffeine.cache;
  exports com.github.benmanes.caffeine.cache.stats;

  requires static com.google.errorprone.annotations;
  requires static org.checkerframework.checker.qual;
}

[ben-manes]

oh! you changed the module name to be non-plural, which means the Automatic-Module-Name does not match the module-info. If I switch to annotation it works, but this seems like it should be fixed by a new release here instead since there is also an annotation package so there is a possible module naming conflict.

# annotations jar
$ wget -q https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotations/2.26.0/error_prone_annotations-2.26.0.jar
$ unzip -q error_prone_annotations-2.26.0.jar META-INF/MANIFEST.MF
$ grep Automatic-Module-Name META-INF/MANIFEST.MF
Automatic-Module-Name: com.google.errorprone.annotations
$ unzip -q error_prone_annotations-2.26.0.jar META-INF/versions/9/module-info.class
$ javap -v META-INF/versions/9/module-info.class | grep "open module"
open module com.google.errorprone.annotation@2.26.0

# annotation jar
$ wget -q https://repo1.maven.org/maven2/com/google/errorprone/error_prone_annotation/2.26.0/error_prone_annotation-2.26.0.jar
$ unzip -q -o error_prone_annotation-2.26.0.jar META-INF/MANIFEST.MF
$ grep Automatic-Module-Name META-INF/MANIFEST.MF
Automatic-Module-Name: com.google.errorprone.annotation

[cushon]

oh! you changed the module name to be non-plural, which means the Automatic-Module-Name does not match the module-info. If I switch to annotation it works, but this seems like it should be fixed by a new release here instead since there is also an annotation package so there is a possible module naming conflict.

Thanks for the catch. In hindsight having both an annotation and annotations package may have been a mistake, but that ship has sailed. Keeping the module name consistent with the package and with the previous Automatic-Module-Name definitely seems like the right thing to do, I will work on fixing that and cutting a 2.26.1 release soon.

[sgammon]

🤦 No good deed goes unpunished. Sorry @cushon, and thank you for the significant help you've provided on landing JPMS support.

[sgammon]

Oh, I see what happened here. The OSGi tools are setting the Automatic-Module-Name based on the symbolic bundle name, in the top-level pom.xml. That's why I missed it. PR incoming.

The frustrating part is suppressing the automatically calculated Automatic-Module-Name from the parent POM, but I figured out a graceful way to do it. In the followup PR, only the annotations module suppresses the parent OSGi execution and specifies its own. This way, we can keep the existing OSGi bundle logic but override it where we're providing a module descriptor.

It probably won't be sufficient for the automatic module name and module descriptor to just line up, as jlink rejects JARs with Automatic-Module-Name in the manifest. I'm not sure that check is negated by the presence of an actual matching module.

In any case, after #4317, this is what I see:

> jar --describe-module --file annotations/target/error_prone_annotations-1.0-HEAD-SNAPSHOT.jar --release 9
releases: 9

com.google.errorprone.annotations@1.0-HEAD-SNAPSHOT jar:file:///.../error_prone_annotations-1.0-HEAD-SNAPSHOT.jar!/META-INF/versions/9/module-info.class open
exports com.google.errorprone.annotations
exports com.google.errorprone.annotations.concurrent
requires java.base mandated
requires java.compiler

The Automatic-Module-Name, meanwhile, is no longer present in the MANIFEST.MF for annotations, having been replaced by Multi-Release (again, very sorry, cannot believe I missed that):

The other JARs in Error Prone have not changed:

[ben-manes]

thanks for the speedy fix! I can confirm that works.

I believe you should add a warning about the breaking changes, though, from moving to the module-info approach. This makes the modularization checks stricter and Java 17 enforces that more so than Java 11. Adding those missing requires static declarations fixes the problem, so a good change but is possibly breaking if the project is not actively maintained so users are impacted by a wrong module-info.

Java 11: Compiles

➜  caffeine git:(master) JAVA_VERSION=11 gradle clean compileTestJava --no-build-cache --console plain
executing gradlew instead of gradle
Starting a Gradle Daemon, 3 stopped Daemons could not be reused, use --status for details
Configuration on demand is an incubating feature.
Reusing configuration cache.
> Task :clean UP-TO-DATE
> Task :jcache:clean
> Task :guava:clean
> Task :simulator:clean
> Task :compileJava NO-SOURCE
> Task :processResources NO-SOURCE
> Task :guava:processResources NO-SOURCE
> Task :classes UP-TO-DATE
> Task :compileTestJava NO-SOURCE
> Task :caffeine:clean
> Task :caffeine:processResources NO-SOURCE
> Task :caffeine:processCodeGenResources NO-SOURCE
> Task :caffeine:processJavaPoetResources
> Task :jcache:processTestResourcesResources
> Task :jcache:processResources
> Task :simulator:processResources
> Task :guava:downloadCaffeine
> Task :jcache:downloadCaffeine
> Task :simulator:downloadCaffeine
> Task :jcache:compileTestResourcesJava NO-SOURCE
> Task :jcache:testResourcesClasses
> Task :caffeine:downloadCaffeine
> Task :jcache:testResourcesJar
> Task :caffeine:compileJavaPoetJava
> Task :caffeine:javaPoetClasses
> Task :caffeine:generateNodes
> Task :caffeine:generateLocalCaches
> Task :caffeine:compileJava
> Task :caffeine:classes
> Task :jcache:compileJava
> Task :jcache:classes
> Task :jcache:compileTestJava
> Task :simulator:compileJava
> Task :simulator:classes
> Task :simulator:compileTestJava
> Task :caffeine:compileCodeGenJava
> Task :caffeine:codeGenClasses
> Task :caffeine:jar
> Task :guava:compileJava
> Task :guava:classes
> Task :guava:compileTestJava
> Task :caffeine:compileTestJava

BUILD SUCCESSFUL in 1m 9s
27 actionable tasks: 26 executed, 1 up-to-date
Configuration cache entry reused.

Java 17: Fails

➜  caffeine git:(master) JAVA_VERSION=17 gradle clean compileTestJava --no-build-cache --console plain
executing gradlew instead of gradle
Starting a Gradle Daemon, 4 stopped Daemons could not be reused, use --status for details
Configuration on demand is an incubating feature.
Calculating task graph as configuration cache cannot be reused because environment variable 'JAVA_VERSION' has changed.
> Task :plugins:checkKotlinGradlePluginConfigurationErrors
> Task :plugins:generateExternalPluginSpecBuilders UP-TO-DATE
> Task :plugins:extractPrecompiledScriptPluginPlugins UP-TO-DATE
> Task :plugins:compilePluginsBlocks UP-TO-DATE
> Task :plugins:generatePrecompiledScriptPluginAccessors UP-TO-DATE
> Task :plugins:generateScriptPluginAdapters UP-TO-DATE
> Task :plugins:compileKotlin UP-TO-DATE
> Task :plugins:compileJava NO-SOURCE
> Task :plugins:pluginDescriptors UP-TO-DATE
> Task :plugins:processResources UP-TO-DATE
> Task :plugins:classes UP-TO-DATE
> Task :plugins:jar UP-TO-DATE
> Task :clean UP-TO-DATE
> Task :processResources NO-SOURCE
> Task :compileJava NO-SOURCE
> Task :classes UP-TO-DATE
> Task :compileTestJava NO-SOURCE
> Task :guava:clean
> Task :guava:processResources NO-SOURCE
> Task :jcache:clean
> Task :jcache:processTestResourcesResources
> Task :jcache:processResources
> Task :simulator:clean
> Task :caffeine:clean
> Task :caffeine:processCodeGenResources NO-SOURCE
> Task :caffeine:processResources NO-SOURCE
> Task :caffeine:processJavaPoetResources
> Task :simulator:processResources
> Task :simulator:downloadCaffeine
> Task :caffeine:downloadCaffeine
> Task :jcache:downloadCaffeine
> Task :jcache:compileTestResourcesJava NO-SOURCE
> Task :jcache:testResourcesClasses
> Task :jcache:testResourcesJar
> Task :guava:downloadCaffeine
> Task :caffeine:compileJavaPoetJava
> Task :caffeine:javaPoetClasses
> Task :caffeine:generateNodes
> Task :caffeine:generateLocalCaches
> Task :caffeine:compileJava
> Task :caffeine:classes
> Task :jcache:compileJava
> Task :jcache:classes
> Task :jcache:compileTestJava
> Task :simulator:compileJava
> Task :simulator:classes
> Task :simulator:compileTestJava
> Task :caffeine:compileCodeGenJava
> Task :caffeine:codeGenClasses
> Task :caffeine:jar

> Task :guava:compileJava FAILED
/Users/ben/projects/caffeine/guava/src/main/java/com/github/benmanes/caffeine/guava/CaffeinatedGuavaCache.java:43: error: package com.google.errorprone.annotations is not visible
import com.google.errorprone.annotations.CanIgnoreReturnValue;
                            ^
  (package com.google.errorprone.annotations is declared in module com.google.errorprone.annotations, but module com.github.benmanes.caffeine.guava does not read it)
/Users/ben/projects/caffeine/guava/src/main/java/com/github/benmanes/caffeine/guava/package-info.java:32: error: package com.google.errorprone.annotations is not visible
import com.google.errorprone.annotations.CheckReturnValue;
                            ^
  (package com.google.errorprone.annotations is declared in module com.google.errorprone.annotations, but module com.github.benmanes.caffeine.guava does not read it)
2 errors

> Task :caffeine:compileTestJava

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':guava:compileJava'.
> Compilation failed; see the compiler error output for details.

* Try:
> Run with --info option to get more log output.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.7-rc-3/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD FAILED in 1m 11s
36 actionable tasks: 26 executed, 10 up-to-date
Configuration cache entry stored.

Code fix

diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml
index 6e6dd225..3eb01dfc 100644
--- a/gradle/libs.versions.toml
+++ b/gradle/libs.versions.toml
@@ -23,7 +23,7 @@ coveralls = "2.12.2"
 dependency-check = "9.0.9"
 eclipse-collections = "12.0.0.M3"
 ehcache3 = "3.10.8"
-errorprone-core = "2.25.0"
+errorprone-core = "2.26.1"
 errorprone-plugin = "3.1.0"
 errorprone-support = "0.15.0"
 expiring-map = "0.5.11"
diff --git a/guava/src/main/java/module-info.java b/guava/src/main/java/module-info.java
index 417c567e..79ee06a2 100644
--- a/guava/src/main/java/module-info.java
+++ b/guava/src/main/java/module-info.java
@@ -1,4 +1,7 @@
 module com.github.benmanes.caffeine.guava {
   requires transitive com.github.benmanes.caffeine;
   requires transitive com.google.common;
+
+  requires static com.google.errorprone.annotations;
+  requires static org.checkerframework.checker.qual;
 }

[cushon]

I believe you should add a warning about the breaking changes, though, from moving to the module-info approach.

Thanks! I added a note to https://github.com/google/error-prone/releases/tag/v2.26.1, corrections are welcome if I missed anything.

[billrobertson42]

It probably won't be sufficient for the automatic module name and module descriptor to just line up, as jlink rejects JARs with Automatic-Module-Name in the manifest. I'm not sure that check is negated by the presence of an actual matching module.

@sgammon I am currently working on some scripts to convert netty and vertx jars to explicit modules, and they work by generating a module-info.java w/jdeps*, then compiling it, and then patching the class file into the appropriate jar w/o modifying the manifest. I'm in the early stages of this, but jlink (Java 21) seems to accept these just fine. At least so far.

(*) or copying a modified one if necessary

[sgammon]

@billrobertson42 Ah, thanks; great to know for future JPMS work.

[cpovirk]

If we someday learn of a reason to prefer one number in the version over another

I spent part of this morning playing with japicmp-maven-plugin for Guava. I discovered that it does a better job of automatically identifying the "previous version" if the current (in our case, snapshot) version is one that counts as "higher." "HEAD-SNAPSHOT" and "1.0-HEAD-SNAPSHOT," sadly, do not count as "higher." (I might once have been under the impression that "HEAD-SNAPSHOT" would count as higher, though I might have already been corrected on that at least once.) That would give us one small reason to use a version like "999.0.0-HEAD-SNAPSHOT" or just "999.0.0-SNAPSHOT."

However, there are at least three caveats:

• For Guava, the automatic identification of the previous version is of no help: We're going to need to set an explicit version so that our -jre and -android variants each compare against the previous release of the same variant.
• For non-Guava projects, it looks possible to work around the problem by explicitly specifying an <oldVersion> and setting <version>RELEASE</version>. That's more verbose than not having to do that, but it's at least a value that you can plug in and then not have to touch for new releases. Except:
• If a project performs a patch/minor release after a subsequent minor/major release, then the project should explicitly set the previous version anyway.

A pattern that I might employ for Guava is:

• Define an oldVersion Maven property.
• Set that property in pom.xml to the most recent -jre or -android release, as appropriate for the variant.
• Configure japicmp-maven-plugin to set its <oldVersion> from that Maven property.

The above is good enough for our snapshot releases. Then:

• As part of our preparation for a new release, bump oldVersion to the new version.
• Build the release with the Maven -DoldVersion=... flag to set the oldVersion property to the actual old version.

It would be simpler to set the new version afterward, but we already have a "preparation" step, so we might find it easiest to set things up as shown above.

I'm sure we can find a way to automatically identify the most recent release from the command line (during our release script) or maybe from a Maven property (in which case we could just plug that into <oldVersion> directly, at least if we don't care about the case of backport releases).

[ben-manes]

fwiw, see ComparableVersion if you want to understand whether Maven will consider a version higher than another. It special cases some common qualifiers. StaticVersionComparator is Gradle's equivalent, which are mostly the same but historically differed. Not sure that helps and just knowledge that I unfortunately have burned in my head by writing a dependency updates checker.