Add explicit module-info's for JPMS compatability #1624

lion7 · 2021-08-03T14:17:48Z

In this pull request I added a few module-info.java files so the kotlinx.serialization library can be used in Java JPMS (Jigsaw) projects. I also extended the root build.gradle script so that the appropriate subprojects do a separate compilation of the module-info.java file.

To maintain backwards compatibility with Java 8, I marked the resulting JAR as a multi-release JAR with the module-info.class moved to META-INF/versions/9/module-info.class which is similar as how it's done in the Kotlin stdlib. See https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/jdk8/build.gradle#L80, https://github.com/JetBrains/kotlin/blob/master/buildSrc/src/main/kotlin/LibrariesCommon.kt#L22 and https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/jdk8/java9/module-info.java for more details.

Note that moving the module-info.class was / is not really necessary for JPMS to work, but I believe it's a bit cleaner than just adding the compiled module-info.class to the root of the JAR.

This change allows bundling projects using jlink, such as requested in #940. jlink requires that all dependencies are explicit JPMS (Jigsaw) modules. That means all JAR's must contain a module-info.class, automatic modules don't work with jlink.
In 1 of my own projects I am trying to bundle an OpenJFX / TornadoFX application, which currently works using a SNAPSHOT version of this branch.

Note that at least JDK 9 is needed to build this branch (for compiling the module-info.java file). Existing code can of course be compiled using a Java 8 target.

lion7 · 2021-08-07T12:24:41Z

After some more testing I observed that the Kotlin compiler does not check if the correct requires <package> is present when importing a class. The Java compiler does check this when it's running in 'module mode', which is automatically activated when a module-info.java is present. As a result of this behaviour I got ClassNotFoundErrors at runtime (instead of compile errors) because some of the needed requires <package> lines were missing in my module-info.java files.

To circumvent this problem altogether I took an alternative route where the module-info.java is generated by the jdeps tool. The jdeps tool can generate a module-info.java file by inspecting the imports of all class files in a (non-modular) JAR. For this to work I only had to change the Gradle build script. It basically comes down to the following steps:

Create a Jar task that generates a non-modular JAR with the same contents / classes as the main jvmJar task.
Use jdeps to generate a module-info.java based on this non-modular JAR.
Create a JavaCompile task that compiles the generated file into a module-info.class file. Here I explicitly specify the module-path that should be used during compilation. I also had to patch the main classes dir so the compiler 'knows' which packages can be exported using exports <package> (I actually figured this out after finding this post of @ilya-g).
Add the compiled file as META-INF/versions/9/module-info.class to the main jvmJar task and mark it as a multi-release JAR.

build.gradle

buildSrc/src/main/kotlin/Java9Modularity.kt

ilya-g

I believe it would be ok to use jdeps once to find the dependencies of a module, but after that the resulting module-info file should be committed in the repository and maintained manually in order to have control on what is being exported.

ilya-g · 2021-08-16T14:17:15Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+                            "--release", "9",
+                            "--module-path", compileKotlinJvm.classpath.asPath,
+                            "--patch-module", "$moduleName=${compileKotlinJvm.destinationDir}",
+                            "-Xlint:-requires-transitive-automatic"


This will fail (as expected) when running build on JDK 8. Do we have a manual about setting up the environment for building kotlinx.serialization? It may need a note about required JDKs and their location.

Ah, good point. Is that something I should add to the README?

I figured docs/building.md would be the correct place.
An alternative would be to only configure the compilation task of the module-info if the JDK is version 9 or higher.
Maybe with a warning that the resulting JAR is not JPMS compatible due to the used JDK?

lion7 · 2021-08-16T14:35:25Z

I could add the resulting module-info to the repo and configure only the compilation task, but that does have the downside that the imported packages are not checked by the Kotlin compiler. In other words, if you use a new package from a library and forget to add the needed requires, it would still all compile and package (but it will break at runtime).

ilya-g · 2021-08-16T15:11:17Z

AFAIK, requires clauses are specified not for packages, but for modules. So, it's not the case when you started using new package from a library, but rather when you add a dependency on a new library.

sandwwraith · 2021-08-16T15:16:00Z

Is it possible to go with a similar to binary compatibility validator and apiCheck approach? I.e. add two tasks:

moduleInfoDump will generate human-readable module-info.java to commit to repository
moduleInfoCheck will check that no new dependencies were accidentally introduced

sandwwraith · 2021-08-16T15:17:50Z

But it seems that we anyway have to use Java 9 to compile module-info.java even if it is committed to the repository? Or it can be simply ignored by JDK 8

lion7 · 2021-08-16T15:39:10Z

My mistake, requires is per module and not per package. But still, if you would add a dependency it would not break the compilation of the module-info, even if it is at that point incomplete.

As for the required JDK: to compile the module-info file you would need at least JDK 9. But since the module-info file is compiled separately you could choose to skip the compilation completely instead of throwing an exception. Of course the resulting JAR would not work with the Java 9 module system if that direction is chosen.

My personal preference would be to make JDK 9+ mandatory for building (but not for running) Kotlin serialization. To support older JVM runtimes the compiled module-info.class file should be stored in the multi-release way, so for example a JVM 8 runtime won't break when loading all class files.

lion7 · 2021-08-16T15:43:41Z

I do like the idea of having a separate moduleInfoCheck task that visually shows the repository version of the module-info vs a generated one. I think this is actually the purpose of jdeps 'check' command, I'll do a bit of research there.

lion7 · 2021-08-17T07:56:06Z

Per your suggestions I reworked my branch so it now has the (generated) module-info files stored in the repository.
I also reworked the configureJava9ModuleInfo method so it works correctly with Kotlin multiplatform JVM targets.
Note that this now requires Java 9 to compile, so I added a note in docs/building.md.

For checking the contents of the module-info I added a jvmCheckModuleInfo task (must be run manually).
This uses jdeps under the hood to diff the compiled module-info with the file that jdeps would generate.

This is the output when the contents of the module-info file is identical to the generated output:

> Task :kotlinx-serialization-core:jvmCheckModuleInfo
kotlinx.serialization.core (file:///C:/IdeaProjects/lion7/kotlinx.serialization/core/build/libs/kotlinx-serialization-core-jvm-1.2.2-SNAPSHOT.jar)
  [Module descriptor]
    requires mandated java.base;
    requires transitive kotlin.stdlib;

And this is the output when the contents of the module-info file is different (an extra, unused requires) to the generated output:

> Task :kotlinx-serialization-core:jvmCheckModuleInfo
kotlinx.serialization.core (file:///C:/IdeaProjects/lion7/kotlinx.serialization/core/build/libs/kotlinx-serialization-core-jvm-1.2.2-SNAPSHOT.jar)
  [Module descriptor]
    requires mandated java.base;
    requires transitive kotlin.stdlib;
    requires some.library;
  [Suggested module descriptor for kotlinx.serialization.core]
    requires mandated java.base;
    requires transitive kotlin.stdlib;
  [Transitive reduced graph for kotlinx.serialization.core]
    requires mandated java.base;
    requires transitive kotlin.stdlib;

buildSrc/src/main/kotlin/Java9Modularity.kt

sandwwraith · 2021-08-18T15:22:11Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+                dependsOn(outputJarTask)
+                doLast {
+                    val jdeps = ToolProvider.findFirst("jdeps").orElseThrow { IllegalStateException("Tool 'jdeps' is not available") }
+                    jdeps.run(


It would be nice if we could detect if jdeps reports different dependencies from the current module-info and fail the task. This is needed to check automatically on CI that no new dependencies are accidentally introduced.

However, this may block us from removing some dependencies manually

I also don't see task to 'write' module-info. But from your comment I assume the same CheckModuleInfo task can be invoked and results copied from console, right?

To automatically check differences I could capture the (console) output that comes from jdeps and throw an exception if something along the lines of [Suggested module descriptor for xxx] is in the output. If you remove dependencies you would need to remove the appropriate requires line from the module-info too, which I believe to be the desired effect.

For 'writing' the module-info you would indeed need to copy the suggested module-descriptor manually from the console output into the repository version of the module-info.

sandwwraith · 2021-08-18T15:25:44Z

formats/json/jvmJava9/src/module-info.java

@@ -0,0 +1,7 @@
+module kotlinx.serialization.json {
+    requires transitive kotlin.stdlib;
+    requires transitive kotlinx.serialization.core;


Although json definitely uses some things from kotlinx.serialization.internal package, it isn't mentioned in 'requires' here. Can this cause issues?

If not, I'd prefer removing exports kotlinx.serialization.*.internal compiletely.

This line requires the kotlinx.serialization.core module, not the package. Since the kotlinx.serialization.core module exports the kotlinx.serialization.internal package it can be used by kotlinx.serialization.json.

As an alternative, we could export kotlinx.serialization.internal to only the format modules.

lion7 · 2021-08-19T12:58:49Z

I reworked the whole modularity configuration a bit. It now also supports 'regular' Kotlin projects such as the Hocon format. Another thing that has changed, is that the module-info.java file now resides in the main sourceset instead of in a separate sourceset. And instead of creating a full-blown compilation, now only a single JavaCompile task (compileModuleInfo[Jvm]) is registered to compile the relevant module-info file. This also prevents unwanted effects if someone uses kotlin.sourceSets.forEach {} or the equivalent for compilations.

A nice side-effect of the dedicated JavaCompile task is that I don't have to know the Java module name up front. The checkModuleInfo task does need to know the module name, but that task can simply parse the (by that time) compiled module descriptor and retrieve the name from there. The same checkModuleInfo task now also throws an exception if the module descriptor is different compared to what jdeps reports. This check has become quite strict, I was a bit unsure if I should keep the exact matching of the output or that pattern matching would be better suitable there..

Lastly, I couldn't remove the export of kotlinx.serialization.internal in the core module since it's used by the formats, but I could remove the exports of internal packages in all the formats without breaking stuff.

sandwwraith

Looks good to me (modulo comments). I'm still not sure whether need to put module-info in a separate compilation (as in previous version) or just as a separate task (current), but it seems it doesn't matter much

buildSrc/src/main/kotlin/Java9Modularity.kt

sandwwraith · 2021-08-20T17:41:37Z

NB: I've changed base branch to dev, but it seems that now your branch contains unrelated commits. Can you please rebase your PR on latest dev?

lion7 · 2021-08-21T05:34:37Z

I rebased my branch on dev as suggested and force-pushed it. I also updated the Hocon config library to a version that better supports JPMS, see lightbend/config#546. Could you double check?

sandwwraith · 2021-08-25T12:38:25Z

It looks like module-info.java in the main source set causes issues in binary compatibility validator: Kotlin/binary-compatibility-validator#68

So I think I'll merge this after that issue would be resolved

sandwwraith · 2021-08-25T12:39:11Z

I've checked resulting jars — it has META-INF/versions/9/module-info.class with major version 53 and all other classes have major version 50, as intended

buildSrc/src/main/kotlin/Java9Modularity.kt

lion7 · 2021-08-26T09:54:39Z

I noticed that version 0.7.1 of the binary compatibility validator just came available on Maven central, so I updated the version accordingly. The build succeeds locally now 😄

sandwwraith · 2021-09-03T14:08:54Z

Many thanks for your efforts!

lion7 · 2021-09-04T06:04:44Z

You're welcome. I'm planning to create a PR for kotlinx.datetime too, as I'm using both in my projects.

sandwwraith · 2021-09-07T11:12:42Z

@lion7 Have you opened the project jvmMain souceset in the IDE? I'm now experiencing problems with it: https://youtrack.jetbrains.com/issue/KTIJ-19614
Maybe a separate sourceset for module-info was a better idea.

lion7 · 2021-09-07T19:29:07Z

Follow up is in #1665

lion7 mentioned this pull request Aug 3, 2021

Add explicit module-info for JPMS compatibility Kotlin/kotlinx-datetime#135

Merged

sandwwraith requested a review from ilya-g August 12, 2021 18:03

sandwwraith reviewed Aug 12, 2021

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

buildSrc/src/main/kotlin/Java9Modularity.kt Outdated Show resolved Hide resolved

lion7 requested a review from sandwwraith August 13, 2021 04:25

ilya-g requested changes Aug 16, 2021

View reviewed changes

ilya-g reviewed Aug 16, 2021

View reviewed changes

sandwwraith reviewed Aug 18, 2021

View reviewed changes

sandwwraith approved these changes Aug 20, 2021

View reviewed changes

buildSrc/src/main/kotlin/Java9Modularity.kt Outdated Show resolved Hide resolved

buildSrc/src/main/kotlin/Java9Modularity.kt Outdated Show resolved Hide resolved

sandwwraith changed the base branch from master to dev August 20, 2021 17:40

sandwwraith requested a review from ilya-g August 20, 2021 17:41

lion7 added 2 commits August 21, 2021 07:19

Add module-info files for JPMS compatability

a00925a

Update Hocon to a JPMS compatible version

1671ccd

sandwwraith mentioned this pull request Aug 25, 2021

module-info.java in compilation source causes IllegalStateException Kotlin/binary-compatibility-validator#68

Closed

sandwwraith reviewed Aug 25, 2021

View reviewed changes

buildSrc/src/main/kotlin/Java9Modularity.kt Outdated Show resolved Hide resolved

Update binary compatibility validator

6194d91

sandwwraith merged commit cf6e41a into Kotlin:dev Sep 3, 2021

TheMrMilchmann mentioned this pull request Sep 6, 2021

Explicit module-descriptor for Java 9 and later #940

Closed

lion7 mentioned this pull request Sep 7, 2021

Add explicit module-info for JPMS compatibility Kotlin/kotlinx-atomicfu#201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit module-info's for JPMS compatability #1624

Add explicit module-info's for JPMS compatability #1624

lion7 commented Aug 3, 2021 •

edited

Loading

lion7 commented Aug 7, 2021

ilya-g left a comment

ilya-g Aug 16, 2021

lion7 Aug 16, 2021

lion7 Aug 16, 2021

lion7 commented Aug 16, 2021

ilya-g commented Aug 16, 2021 •

edited

Loading

sandwwraith commented Aug 16, 2021

sandwwraith commented Aug 16, 2021

lion7 commented Aug 16, 2021

lion7 commented Aug 16, 2021

lion7 commented Aug 17, 2021 •

edited

Loading

sandwwraith Aug 18, 2021

sandwwraith Aug 18, 2021

lion7 Aug 18, 2021

sandwwraith Aug 18, 2021

lion7 Aug 18, 2021

lion7 commented Aug 19, 2021

sandwwraith left a comment

sandwwraith commented Aug 20, 2021

lion7 commented Aug 21, 2021

sandwwraith commented Aug 25, 2021

sandwwraith commented Aug 25, 2021

lion7 commented Aug 26, 2021

sandwwraith commented Sep 3, 2021

lion7 commented Sep 4, 2021

sandwwraith commented Sep 7, 2021

lion7 commented Sep 7, 2021

Add explicit module-info's for JPMS compatability #1624

Add explicit module-info's for JPMS compatability #1624

Conversation

lion7 commented Aug 3, 2021 • edited Loading

lion7 commented Aug 7, 2021

ilya-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lion7 commented Aug 16, 2021

ilya-g commented Aug 16, 2021 • edited Loading

sandwwraith commented Aug 16, 2021

sandwwraith commented Aug 16, 2021

lion7 commented Aug 16, 2021

lion7 commented Aug 16, 2021

lion7 commented Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lion7 commented Aug 19, 2021

sandwwraith left a comment

Choose a reason for hiding this comment

sandwwraith commented Aug 20, 2021

lion7 commented Aug 21, 2021

sandwwraith commented Aug 25, 2021

sandwwraith commented Aug 25, 2021

lion7 commented Aug 26, 2021

sandwwraith commented Sep 3, 2021

lion7 commented Sep 4, 2021

sandwwraith commented Sep 7, 2021

lion7 commented Sep 7, 2021

lion7 commented Aug 3, 2021 •

edited

Loading

ilya-g commented Aug 16, 2021 •

edited

Loading

lion7 commented Aug 17, 2021 •

edited

Loading