Skip to content
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 explicit module-info for JPMS compatibility #135

Merged
merged 17 commits into from
Nov 14, 2021
Merged

Add explicit module-info for JPMS compatibility #135

merged 17 commits into from
Nov 14, 2021

Conversation

lion7
Copy link
Contributor

@lion7 lion7 commented Aug 3, 2021

In this pull request I added a module-info.java file so the kotlinx.datetime library can be used in Java JPMS (Jigsaw) projects. I also extended the build.gradle.kts script so it does 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, which 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. This PR is closely related to Kotlin/kotlinx.serialization#1624.

@lion7
Copy link
Contributor Author

lion7 commented Aug 7, 2021

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.

Note that I updated the version of kotlinx.serialization to 1.2.2-SNAPSHOT, which is a locally built SNAPSHOT of Kotlin/kotlinx.serialization#1624. For this to work I also added mavenLocal() as a repository in the root Gradle build script.

I also had to update the kotlin version to 1.5.20 so it matches the version used by kotlinx.serialization. Without this change I got a weird exception about resolving some klib, which seems similar to https://youtrack.jetbrains.com/issue/KT-43911:

java.lang.IllegalStateException: e: Failed to resolve Kotlin library: C:\IdeaProjects\lion7\kotlinx-datetime\core\build\kotlinSourceSetMetadata\commonMain\org.jetbrains.kotlinx-kotlinx-serialization-core\org.jetbrains.kotlinx-kotlinx-serialization-core-commonMain.klib
	at org.jetbrains.kotlin.library.SingleFileResolveKt$resolveSingleFileKlib$1.error(SingleFileResolve.kt:19)
	at org.jetbrains.kotlin.library.SingleFileResolveKt$resolveSingleFileKlib$1.error(SingleFileResolve.kt:17)
	at org.jetbrains.kotlin.library.KotlinLibrarySearchPathResolver.resolve(SearchPathResolver.kt:154)
	at org.jetbrains.kotlin.library.KotlinLibrarySearchPathResolver.resolve(SearchPathResolver.kt:162)
	at org.jetbrains.kotlin.library.CompilerSingleFileKlibResolveStrategy.resolve(SearchPathResolver.kt:283)
	at org.jetbrains.kotlin.library.SingleFileResolveKt.resolveSingleFileKlib(SingleFileResolve.kt:24)
	at org.jetbrains.kotlin.library.SingleFileResolveKt.resolveSingleFileKlib$default(SingleFileResolve.kt:15)
	at org.jetbrains.kotlin.cli.metadata.KlibMetadataDependencyContainer.<init>(K2MetadataKlibSerializer.kt:118)
	at org.jetbrains.kotlin.cli.metadata.K2MetadataKlibSerializer.serialize(K2MetadataKlibSerializer.kt:52)
	at org.jetbrains.kotlin.cli.metadata.K2MetadataCompiler.doExecute(K2MetadataCompiler.kt:109)
	at org.jetbrains.kotlin.cli.metadata.K2MetadataCompiler.doExecute(K2MetadataCompiler.kt:40)
	at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:88)
	at org.jetbrains.kotlin.cli.common.CLICompiler.execImpl(CLICompiler.kt:44)
	at org.jetbrains.kotlin.cli.common.CLITool.exec(CLITool.kt:98)
	at org.jetbrains.kotlin.daemon.CompileServiceImpl.compile(CompileServiceImpl.kt:1575)

@lion7
Copy link
Contributor Author

lion7 commented Sep 7, 2021

I reworked this branch / PR based on the comments from Kotlin/kotlinx.serialization#1624.
In summary:

  • The module-info.java file is now stored in the Git repository
  • The jdkHome used for compilation is now JDK_11
  • Kotlin version is raised to 1.5.30
  • Kotlinx.serialization version is raised to 1.3.0-RC

@ilya-g ilya-g self-requested a review October 29, 2021 16:22
@lion7
Copy link
Contributor Author

lion7 commented Nov 6, 2021

A similar solution has been merged in kotlinx.serialization. That caused https://youtrack.jetbrains.com/issue/KTIJ-19614 which was fixed in Kotlin/kotlinx.serialization#1665.
I synchronized the final code of kotlinx.serialization to this PR as well, so we won't run into issues here.

Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.

We'd like to pursue a slightly different approach in this project than in kotlinx-serialization:

  • first, we'll continue building and testing the JVM target with JDK 8 to avoid accidentally using a newer API or depending on a newer behavior.
  • module-info.java should be compiled in a separate java compilation task that uses another sufficient JDK with the -release 9 option
  • the result of that compilation shall be added into the resulting jar in the META-INF/versions/9 directory
  • we'll be using Gradle toolchains feature to detect or provision the required JDKs: JDK 8 for the main compilation and tests and JDK 11 for compiling module-info. This work has started in the PR Use java toolchains to setup JDK 8 path #155.

Could you modify the PR according to the points above?

@@ -0,0 +1,7 @@
module kotlinx.datetime {
requires transitive kotlin.stdlib;
requires transitive static kotlinx.serialization.core;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted here that we need requires static because the dependency is compile only. Does it mean that a client module has to require kotlinx.serialization.core (or another module that requires it transitively) in order to use this aspect of kotlinx.datetime?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. Normally a modular application won't start if 1 of the required modules isn't on the module path.
Because of the requires static an application will start anyway, ignoring the 'missing' module.
Obviously if you want to use the datetime serializers you will have to explicitly require kotlinx.serialization.core anyway because all the serializer interfaces and logic is in there.


val kotlinVersion = file("../gradle.properties").inputStream().use {
Properties().apply { load(it) }
}.getProperty("kotlinVersion") ?: throw IllegalStateException("Property 'kotlinVersion' must be defined in ../gradle.properties")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach significantly limits the ways how kotlinVersion can be defined. Due to that we'd like to avoid having the code that depends on kotlin plugin in buildSrc, especially considering that it is only needed in one subproject so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree, this was actually the most annoying part. I just wished that the buildSrc project had access to the properties of the root project, but that is unfortunately not the case.

Anyway, I'll move the relevant code to an (simplified) inline block inside the core subproject, since that is the only place that uses this.

@lion7
Copy link
Contributor Author

lion7 commented Nov 11, 2021

@ilya-g I processed your feedback and moved the Gradle configuration to an inline block in the core project's build.gradle.kts. Could you please re-review?

Also note this line, which can be removed once #155 is merged:
https://github.com/Kotlin/kotlinx-datetime/pull/135/files#diff-a6fced4b4e4a99784a2b92028b1adbc8c0ca28ff54e264b0e73727b3d4a460cfR259

core/build.gradle.kts Outdated Show resolved Hide resolved
core/build.gradle.kts Outdated Show resolved Hide resolved
core/build.gradle.kts Outdated Show resolved Hide resolved
@lion7
Copy link
Contributor Author

lion7 commented Nov 12, 2021

@ilya-g updated this PR per your suggestions.

Both the doFirst {} and doLast {} sections are now no longer necessary.
The main kotlin classes are now patched instead of using the same destination dir. The downside of that is that I had to specify the module name twice, namely in the module-info.java and as an argument to --patch-module in the build.gradle.kts. It's not a big deal here, because the configuration is only used in 1 project. But it makes the whole solution a bit less generic.

Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good. A small polishing is required, but I'll do it myself after merging.

@ilya-g ilya-g changed the title Add explicit module-info for JPMS compatability Add explicit module-info for JPMS compatibility Nov 14, 2021
@ilya-g ilya-g merged commit e6401c6 into Kotlin:master Nov 14, 2021
@ilya-g
Copy link
Member

ilya-g commented Nov 14, 2021

Merged. The resulting commits are e6401c6~1...bbbbd9c
Thanks for a good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants