-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add module-info.java to checker-qual #6326
Conversation
@mtf90
Could you please fix that? I would love to get this pull request merged. Thanks! |
this is in accordance with the declaration order of JDK module definitions
make sure to compile all files with Java 9 first so that the compile may detect missing "requires" statements
Dear @mernst, I disabled the module path for the other javadoc processes as well which seemed to do the trick. However, I am still not that satisfied with the PR.
|
I managed to clean up the configs some more, given that slowly I get the hang of Also, the two pipelines seem to have failed due to a timeout, not an actual error (as all other pipelines have passed). |
@sgammon, as mentioned in #6326 (comment), I don't see a clean way of implementing this without some larger refactoring (which is not my decision to make). So I can understand the reluctance of the maintainers. As mentioned in #4559 (comment), there is a way to workaround this in downstream projects. For example, I managed to successfully implement it in LearnLib/learnlib#122 including a passing jlink integration test. |
My apologies; I lost track of this pull request. I'm going to assign it for review and hopefully we can get it merged. |
@mernst No problem, and thank you guys for merging this! (Tagging google/guava#2970 as well) |
## 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`](#2649), 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](google/guava#2970), and this PR will be necessary for that to eventually land (without hackery). [Checker Framework recently merged their support](typetools/checker-framework#6326) 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](https://github.com/apache/maven-compiler-plugin/blob/74cfc72acae4f55708bca189b2170167e83df6b3/src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java#L304-L305) [as the `--module-version`][0]; however, [`HEAD-SNAPSHOT` is not a valid module identifier](https://lists.apache.org/thread/qtghq13qgjoc4pn6ovsdxgnjnm4ozv4d). 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][0] Fixes and closes #2649 [0]: https://issues.apache.org/jira/browse/MCOMPILER-579 Fixes #4311 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4311 from sgammon:feat/jpms d209b0c PiperOrigin-RevId: 614005681
## 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`](#2649), 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](google/guava#2970), and this PR will be necessary for that to eventually land (without hackery). [Checker Framework recently merged their support](typetools/checker-framework#6326) 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](https://github.com/apache/maven-compiler-plugin/blob/74cfc72acae4f55708bca189b2170167e83df6b3/src/main/java/org/apache/maven/plugin/compiler/CompilerMojo.java#L304-L305) [as the `--module-version`][0]; however, [`HEAD-SNAPSHOT` is not a valid module identifier](https://lists.apache.org/thread/qtghq13qgjoc4pn6ovsdxgnjnm4ozv4d). 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][0] Fixes and closes #2649 [0]: https://issues.apache.org/jira/browse/MCOMPILER-579 Fixes #4311 COPYBARA_INTEGRATE_REVIEW=#4311 from sgammon:feat/jpms d209b0c PiperOrigin-RevId: 614026439
*/ | ||
module org.checkerframework.checker.qual { | ||
// javadoc-only dependencies | ||
requires static java.compiler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is really for Javadoc only, wouldn't it be cleaner to remove those requires static
statements and replace them by --add-reads
options when invoking the javadoc
tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, you can even skip the --add-reads
because the javadoc phase does not read the module descriptor (due to circular dependencies which are not allowed with JPMS).
I mainly added them to be as explicit as possible with the dependency declarations. For example, IntelliJ would flag even more references if these dependencies were not specified. I don't know how well IDEs are able to include phase-specific build-flags.
Do you have any issues with these declarations? As far as I know, requires static
should not affect dependent modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issue that I'm aware of for now. I thought that those declarations are a risk, because the code could accidentally use those dependencies without warning from the compiler. The argument about being explicit can also be turned the other way around, i.e. they are not describing the reality if the code has no dependency to those modules. Anyway, this is not a major issue.
This PR adds a
module-info.java
to thechecker-qual
project so that the checker-framework annotations can be directly used in projects that want to use thejlink
orjpackage
tools for distributing software. The current automatic modules (viaMANIFEST.MF
'sAutomatic-Module-Name
s) are not supported byjlink
.The corresponding issue is #4559.