-
Notifications
You must be signed in to change notification settings - Fork 531
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
feat(jpms): add module-info.java
descriptors
#556
base: master
Are you sure you want to change the base?
Conversation
- feat(jpms): add `module-info.java` to `api` - chore: adjust builds where needed to release as MRJAR artifacts - chore: adjust BND tools to not interfere with MRJAR classes - chore: upgrade gradle → `7.6.4` - chore: apply updates to groovy dsl for publishing with gradle v7 Signed-off-by: Sam Gammon <[email protected]>
if (isJdk9OrGreater) java9 { | ||
java { | ||
srcDirs = ['src/main/java', 'src/main/java9'] | ||
} | ||
} |
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.
This is now a second source set, instead of an additional directory in the main source set. This allows a second compile task to target a different JVM bytecode level.
configurations { | ||
mrjarArtifact | ||
} |
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.
The MRJAR artifact is exported from the project, so that tests can depend on it. This is important to accurately test JPMS, otherwise Gradle will put these classes on the classpath downstream, which runs tests without modular encapsulation.
if (isJdk9OrGreater) into('META-INF/versions/9') { | ||
from(sourceSets.java9.output) { | ||
include '**/FlowAdapters*' | ||
include '**/module-info.class' | ||
} | ||
} |
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.
Amends the JAR with the META-INF/versions/9
root, from the output of the sourceSets.java9
compile. include
is used here to avoid duplicating classes which reside in the root of the module, at JDK 6, which are identical anyway.
'Export-Package': 'org.reactivestreams.*', | ||
'Automatic-Module-Name': 'org.reactivestreams', | ||
'Bundle-SymbolicName': 'org.reactivestreams.reactive-streams' | ||
'Export-Package': 'org.reactivestreams.*,!META-INF.*', | ||
'Multi-Release': true, | ||
'Bundle-SymbolicName': 'org.reactivestreams.reactive-streams', | ||
'-fixupmessages': '^Classes found in the wrong directory: .*' |
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.
Export-Package
has added !META-INF.*
to avoid processing the Java 9 class root; Multi-Release
was added, to replace Automatic-Module-Name
. -fixupmessages
avoids a BND warning about classes in the Java 9 root.
if (isJdk9OrGreater) { | ||
compileJava9Java { | ||
group = "build" | ||
description = "Compile Java 9 classes for MR JAR" | ||
|
||
sourceCompatibility = 9 | ||
targetCompatibility = 9 | ||
} |
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.
Compile task; in this PR, the task is conditional based on whether the build-time JVM is at least Java 9. The existing build behavior is preserved in this way.
In the other PR, this is addressed by Gradle Toolchains, so the build task is not conditional and is always expected to run at JDK 9 or greater during build time.
module org.reactivestreams { | ||
exports org.reactivestreams; |
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.
The module name matches the previous Automatic-Module-Name
. The exported package is the only package in the JAR, so export-related risk is minimal. If it needs to be an open module
let me know.
def sonatypeRepo = 'https://oss.sonatype.org/service/local/staging/deploy/maven2/' | ||
def sonatypeSnapshots = 'https://oss.sonatype.org/content/repositories/snapshots/' | ||
|
||
def distributionRepository = properties["distributionRepository"] ?: sonatypeRepo | ||
def snapshotsRepository = properties["snapshotsRepository"] ?: sonatypeSnapshots |
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.
I included a small unrelated change which allows parameterization of the deploy repositories. This will allow me to easily test integration of this JPMS support with downstream libraries. It should be completely inert to this codebase.
The username
and password
for releases are still required, but only when the deployment repo is Sonatype.
apply plugin: "maven" | ||
apply plugin: "signing" | ||
apply plugin: "publishing" | ||
apply plugin: "maven-publish" |
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.
Small build cleanup because Gradle 7.x
eliminates the maven
plugin in favor of publishing
. Other than DSL updates, the logic is unchanged.
compileJava { | ||
options.release = 9 | ||
} | ||
|
||
compileTestJava { | ||
options.release = 9 | ||
} |
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.
Always tests at --release=9
, because Flow isn't available until then.
@@ -1,7 +1,7 @@ | |||
description = 'reactive-streams-tck' | |||
dependencies { | |||
api group: 'org.testng', name: 'testng', version:'7.3.0' | |||
api project(':reactive-streams') | |||
api project(path: ':reactive-streams', configuration: 'mrjarArtifact') |
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.
Wires together that the TCK depends on the MRJAR, rather than the unwrapped classes.
cc / @viktorklang, @ktoso This is ready for review. Let me know if you guys are open to this 😁 |
- chore: add `org.reactivestreams` submodule - chore: pin to reactive-streams/reactive-streams-jvm#556 - chore: sync repository - chore: add to version catalog Relates-To: #1 Signed-off-by: Sam Gammon <[email protected]>
- chore: add `org.reactivestreams` submodule - chore: pin to reactive-streams/reactive-streams-jvm#556 - chore: sync repository - chore: add to version catalog Relates-To: #1 Signed-off-by: Sam Gammon <[email protected]>
- chore: add `org.reactivestreams` submodule - chore: pin to reactive-streams/reactive-streams-jvm#556 - chore: sync repository - chore: add to version catalog Relates-To: #1 Signed-off-by: Sam Gammon <[email protected]>
Summary
This PR offers a minimal changeset to safely ship a
module-info.java
descriptor for theorg.reactivestreams
API artifact. The JAR is shipped as an MRJAR, with amodule-info.class
descriptor located inMETA-INF/versions/9
.This keeps the JAR compatible with JDK 6, while supporting full modularity for newer versions of Java.
Based on the current structure of the codebase, I tried to keep this PR as minimal as possible. Unfortunately, Gradle 6 does not yet have the
release
flag for the Java compiler, so I've upgraded Gradle to 7.x. The latest version is7.6.4
. Other than this and themodule-info.java
itself, I've added a task to validate the multi-release JAR as a module.The tests pass and everything seems fine from local, of course, but this should probably be tested with downstream artifacts. I can put together a small integration test harness which uses these JPMS-enabled artifacts with popular Reactive Streams projects to make sure there is a clear picture of impacts.
Fixes and closes #531
Build tooling
The Gradle upgrade clears the way for some other changes, which are more optional/unrelated; for example, using Gradle Toolchains to enforce JDK 6 compatibility. I'm not sure if the Reactive Stream authors/maintainers want these changes, so I have split them off into another PR, which is a superset of this one: #557
Both PRs enable JPMS support, and only one or the other should be merged (if any). That PR has its own explanation about the changes enclosed.
JAR Structure
After applying this PR, the JAR structure for the
api
artifact is:JAR manifest:
OSGi manifest (Java 9+):
Changelog
module-info.java
toapi
7.6.4