3x: Add full module descriptor

[aalmiray]

As described at #7240, add a full Java module descriptor to version 3.x.

This setup lets the 3.x series remain Java 8 compatible while the module-info.class is compiled with Java 9+ and stored inside the jar at /META-INF/versions/9.

Resolves #7240.

[aalmiray]

Converting to draft as there are 2 standing issues:

• org.reactivestreams is not a full module, yet. See Consider adding a full Java module descriptor reactive-streams/reactive-streams-jvm#531
• bnd Gradle plugin 5.3.0 can't handle multi-release JARs correctly OSGi Java 9 support: Multi-release JAR bndtools/bnd#2227

See build error: error : Classes found in the wrong directory: {META-INF/versions/9/module-info.class=module-info}

[A248]

@aalmiray , regarding the two issues you mentioned:

• It isn't a requirement that reactive-streams become a full module in order for RxJava to do so. There's nothing stopping a full module from depending on an automatic one; rather, that's the purpose of automatic modules.

• The last time I ran into this warning from the Bnd gradle plugin, it was an annoying message in the build log but didn't fail the build. Regardless, you should be able to fix it by adapting this example for gradle kotlin: OSGi Java 9 support: Multi-release JAR bndtools/bnd#2227 (comment).

Also, I'm not a veteran RxJava user, but I believe the flowables package should be exported, and the requirement on org.reactivestreams should be transitive.

[aalmiray]

@A248 as mentioned in #7240 the motivation for adding a full module descriptor is to facilitate the use of RxJava with custom Java Runtimes created with jlink. Custom modular JRs only support full modules, not automatic modules.

[ZacSweers]

This seems like an unnecessarily roundabout way to do this. Here's a simple example that you could just adapt instead: https:/melix/jdoctor/blob/main/jdoctor-build-logic/src/main/kotlin/me.champeau.java-module.gradle.kts

[aalmiray]

That would require the use of toolchains and ensuring a Java 9+ JDK to be available at all times. Toolchains are not stable enough, just have a look at the Gradle issue tracker.

[ZacSweers]

Can you offer a stronger citation? We are using it in several projects with no issues.

[aalmiray]

Browse Gradle's issue tracker, they have a label matching toolchains. Look at the number of issues and their assigned milestone. The engineer in charge of toolchains left the company in early June 2021.

[ZacSweers]

Unless you can point to a specific issue that prohibits its use here, I don't think "go look at the issue tracker" is very constructive. No tool is perfect. We use this in about a dozen projects with varying degrees of complexity and it's been fine. Happy to toss up a PR if that would quell your concerns.

[aalmiray]

As you wish. I'm more interested in the outcome (RxJava as a Java module) than how that goal it's achieved.

A maintainable option for the project must be found, after all it's "easy" for me to suggest ModiTect as I know it but the RxJava team would have to live with it if the PR is accepted as is.

If ModiTect it's not the way then so be it, again the "what" is more important to me than the means to achieve it.

One more thing, regardless of how the build is made modular you'd still have to find a solution for reactive-streams becoming modular which is currently blocked by an OSGi compatibility, due to bnd and its companion Gradle plugin.

[ZacSweers]

Sure, opened a PR here #7332.

One more thing, regardless of how the build is made modular you'd still have to find a solution for reactive-streams becoming modular which is currently blocked by an OSGi compatibility, due to bnd and its companion Gradle plugin.

I think I've worked around this just suppressing the warning message from bnd (per the original issue you linked), but I'm not sure if/what else would be needed. Considering the issue has been open for 4 years with no movement, it seems unlikely to change.

[akarnokd]

If this only needs a module-info.class, I took #7332 and injected the file as resource: #7333. I don't expect the exported packages to change much in the future. Is this enough for the module system compatibility?

[aalmiray]

Following this last option would require build instructions to re-generate module-info.class when updates to the descriptor are warranted, allowing anyone to test their changes otherwise you risk a bottle neck when only a few can make the change and push it before every one else may continue.

[akarnokd]

when updates to the descriptor are warranted

It is rare for the main export packages to change. We just had a new package operators added after 1.5 years. I don't expect any new package in the forseable future.

Another middle ground I see is to generate the module file via ASM so there is no need for 9+ compiler and those mockup classes to satisfy it.

[aalmiray]

FWIW generating module-info.class via ASM is exactly what the ModiTect plugin does. There's more than one way to configure generation of that file, this PR presented but one option.

[akarnokd]

I see. In that case, the BND failure can be suppressed. Unless the first point in #7241 is still a concern, this would be good to go.

[codecov]

Codecov Report

Merging #7241 (9852c6b) into 3.x (476aa5c) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 9852c6b differs from pull request most recent head 82b036b. Consider uploading reports for the commit 82b036b to get more accurate results

@@             Coverage Diff              @@
##                3.x    #7241      +/-   ##
============================================
+ Coverage     99.53%   99.55%   +0.02%     
- Complexity     6782     6783       +1     
============================================
  Files           751      751              
  Lines         47482    47482              
  Branches       6378     6378              
============================================
+ Hits          47259    47270      +11     
+ Misses          103       98       -5     
+ Partials        120      114       -6     

Impacted Files	Coverage Δ
...l/operators/observable/ObservableFlatMapMaybe.java	94.36% <0.00%> (-4.23%)	⬇️
...ternal/operators/completable/CompletableMerge.java	98.64% <0.00%> (-1.36%)	⬇️
...rnal/operators/flowable/FlowableFlatMapSingle.java	93.60% <0.00%> (-0.59%)	⬇️
...ternal/operators/observable/ObservableFlatMap.java	97.87% <0.00%> (-0.36%)	⬇️
...operators/flowable/FlowableConcatMapScheduler.java	99.60% <0.00%> (+0.39%)	⬆️
...nternal/operators/observable/ObservableReplay.java	100.00% <0.00%> (+0.53%)	⬆️
...a/io/reactivex/rxjava3/subjects/ReplaySubject.java	99.58% <0.00%> (+0.62%)	⬆️
...3/internal/operators/flowable/FlowablePublish.java	100.00% <0.00%> (+0.99%)	⬆️
...va3/internal/operators/parallel/ParallelRunOn.java	100.00% <0.00%> (+1.46%)	⬆️
...ernal/operators/flowable/FlowableFlatMapMaybe.java	97.92% <0.00%> (+2.07%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 476aa5c...82b036b. Read the comment docs.

[akarnokd]

I have updated the GHA for the JDK 11 build to verify the module-info. Could you rebase this onto the current 3.x branch?

[aalmiray]

@akarnokd PR has been rebased and updated with hints for bnd to skip the error due to module-info.class, following what @ZacSweers did for #7332