Add explicit module-info.java for JPMS compatibility #3297

lion7 · 2022-05-19T14:45:55Z

No description provided.

lion7 · 2022-05-19T14:53:10Z

Note that there is still a highlighting issue with IntelliJ IDEA when using Java modules in a multi-platform Kotlin project.
The relevant issues are https://youtrack.jetbrains.com/issue/KTIJ-15498 and https://youtrack.jetbrains.com/issue/KTIJ-19614.
Right now this results in highlighting errors like these:

qwwdfsad

Thanks for taking care of it!

This is a change we'll be able to ship in the next major release as it carries compatibility risks and may require and RC first, so I will properly investigate the PR around kotlinx.coroutines 1.7.0

Unfortunately, IDEA issues won't be fixed in a foreseeable future, so could you please conditionally enable modules if and only if Idea.isActive is false? In kotlinx.serialization we had plenty of IDEA issues due to that in JVM-nly modules

qwwdfsad · 2022-06-01T15:53:00Z

build.gradle

+// kotlinx.coroutines.debug and kotlinx.coroutines.test
+// because they export packages that also exist in kotlinx.coroutines.core.
+// Note that only 1 module can export a package.
+def invalidModules = ["kotlinx-coroutines-debug", "kotlinx-coroutines-test"]


kotlinx-coroutines-test has been fixed in 1.6.2 and no longer contains split package

lion7 · 2022-06-01T18:57:40Z

Unfortunately I cannot use Idea.isActive because whatever the Gradle script configures is ignored by IDEA.
It just immediately starts analyzing the module graph if it detects a module-info.java in the source root
(if I understood the logic in https://github.com/JetBrains/intellij-community/blob/master/java/java-analysis-impl/src/com/intellij/codeInsight/daemon/impl/analysis/JavaModuleGraphUtil.java correctly).

For now I just moved the module-info.java files to a separate java9 source folder which is included explicitly when compile the module descriptor, similar to how it's done in the kotlinx.serialization project.

elizarov · 2022-06-02T15:24:03Z

AFAIU, module declarations must also declare all the services with uses and provides declarations or the corresponding functionality (like the main dispatcher) will not work. For this purse, it also makes sense to start running all the tests with a module path. This way, these kinds of omissions will be caught during testing.

qwwdfsad · 2022-10-13T12:29:17Z

A short update: the corresponding support of JPMS should land in IDEA 2022.3, so we are ready to proceed with the change.
Could you please rebase it on the most recent develop when you have time to spare?

lion7 · 2022-10-27T10:10:13Z

I just rebased and can confirm that the highlighting issues described in https://youtrack.jetbrains.com/issue/KTIJ-19614 are resolved in IDEA 2022.3 (EAP)

build.gradle

buildSrc/src/main/kotlin/Java9Modularity.kt

integration-testing/src/debugAgentTest/kotlin/DebugProbes.kt

integration/kotlinx-coroutines-guava/src/module-info.java

qwwdfsad · 2022-10-27T16:10:45Z

kotlinx-coroutines-core/jvm/src/module-info.java

+    exports kotlinx.coroutines.debug;
+    exports kotlinx.coroutines.debug.internal;
+    exports kotlinx.coroutines.flow;
+    exports kotlinx.coroutines.flow.internal;


Could you please elaborate on how exactly you got these packages?

It seems like some of them should not be exported in the first place -- all internal (though I have to double check some of them), scheduling and intrinsics

These packages are mostly used in reactive/kotlinx-coroutines-reactive/src/ReactiveFlow.kt. So I need to export them from core so I don't get a compile error in kotlinx-coroutines-reactive.

Have you considered exporting the internal packages just to the kotlinx.coroutines modules that depend on them? For example:

Suggested change

exports kotlinx.coroutines.flow.internal;

exports kotlinx.coroutines.flow.internal to kotlinx.coroutines.reactive;

qwwdfsad · 2022-10-27T16:11:30Z

kotlinx-coroutines-debug/src/module-info.java

+    requires kotlin.stdlib;
+    requires kotlinx.coroutines.core;
+    requires net.bytebuddy;
+    requires net.bytebuddy.agent;


Is it going to work with our ByteBuddy shadowing?
https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-debug/build.gradle#L40

Good question, probably not. I will check this out further.

reactive/kotlinx-coroutines-reactive/src/module-info.java

qwwdfsad · 2022-10-27T22:24:18Z

Also, it seems like some modules cannot be compiled: https://teamcity.jetbrains.com/viewLog.html?buildId=3984907&tab=buildResultsDiv&buildTypeId=KotlinTools_KotlinxCoroutines_Build

lion7 · 2022-11-07T12:35:07Z

AFAIU, module declarations must also declare all the services with uses and provides declarations or the corresponding functionality (like the main dispatcher) will not work. For this purse, it also makes sense to start running all the tests with a module path. This way, these kinds of omissions will be caught during testing.

I also added the necessary uses and provides declarations to the module descriptors. I haven't touched any tests yet, so I have no guarantee that it works as intended though.

qwwdfsad · 2022-11-15T14:34:48Z

@lion7 Is it ready for review or are you still working on it? (I'm not sure if "re-request review" button is available for external contributors, thus the question)

CodeMason · 2023-01-02T15:13:46Z

Hey @lion7 -- is there any holdup on this? Having proper JPMS support would really be fantastic.

lion7 · 2023-01-07T18:44:03Z

@qwwdfsad I think this is ready enough for review, I'm not working on it anymore and just rebased it on the latest develop. The only thing that still bothers me is that there are currently no tests executed with a module path, especially after re-reading the comment from @elizarov.
I did do some manual testing and it should all work, but there might be regressions or that I missed something. Should that kind of testing also be part of this PR or is it ok to do that in a separate PR?

qwwdfsad · 2023-01-09T10:36:14Z

Thanks! Will take care of it during this week

qwwdfsad · 2023-02-16T10:46:25Z

I've started the internal testing (mostly our internal projects, Ktor and some Android apps) process and, as soon as all perturbations are resolved, will publish the dev build for everybody interested in trying it out.

For now, it seems like JavaFx ahs some non-trivial setup issues: none of the tests is able to start and fails with java.lang.ClassNotFoundException: kotlinx.coroutines.javafx.JavaFxStressTest (or any other tests).
@lion7 could you please provide any additional pointers here while I'm looking at other modules?

ALikhachev · 2023-02-16T12:18:16Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+
+    @JvmStatic
+    fun configure(project: Project) = with(project) {
+        val javaToolchains = extensions.getByName("javaToolchains") as JavaToolchainService


Please change it to

Suggested change

val javaToolchains = extensions.getByName("javaToolchains") as JavaToolchainService

val javaToolchains = extensions.findByType(JavaToolchainService::class.java)

?: error("Gradle JavaToolchainService is not available")

ALikhachev · 2023-02-16T12:23:41Z

buildSrc/src/main/kotlin/java-modularity-conventions.gradle.kts

+val invalidModules = listOf("kotlinx-coroutines-play-services")
+
+configure(subprojects.filter {
+    !unpublished.contains(it.name) && !invalidModules.contains(it.name) && it.extensions.findByName("kotlin") != null


It'll be more correct to run the configuration once some Kotlin plugin is applied, like

plugins.withType(org.jetbrains.kotlin.gradle.plugin.KotlinBasePlugin::class) { // perform some configuration }

instead of checking for the extension presence right now. The order of plugin appliance should not matter.

ALikhachev · 2023-02-16T12:26:18Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+
+            // Patch the compileKotlinJvm output classes into the compilation so exporting packages works correctly.
+            val kotlinCompileDestinationDir = compileKotlinTask.destinationDirectory.asFile.get()
+            options.compilerArgs.addAll(listOf("--patch-module", "$moduleName=$kotlinCompileDestinationDir"))


Please try to preserve laziness here, e.g. like

val destinationDirProperty = compileKotlinTask.destinationDirectory.asFile options.compilerArgumentProviders.add({ val kotlinCompileDestinationDir = destinationDirProperty.get() listOf("--patch-module", "$moduleName=$kotlinCompileDestinationDir") })

ALikhachev · 2023-02-16T12:29:50Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+            if (!sourceFile.exists()) {
+                throw IllegalStateException("$sourceFile not found in $project")
+            }
+            val compileKotlinTask = compilation.compileKotlinTask as org.jetbrains.kotlin.gradle.tasks.KotlinCompile


compilation.compileKotlinTask is deprecated.
You could use

val compileKotlinTask = compilation.compileTaskProvider.get() as? org.jetbrains.kotlin.gradle.tasks.KotlinCompile ?: error("Cannot access Kotlin compile task ${compilation.compileKotlinTaskName}")

ALikhachev · 2023-02-16T12:59:56Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+            // but it currently won't compile to a module-info.class file.
+            // Note that module checking only works on JDK 9+,
+            // because the JDK built-in base modules are not available in earlier versions.
+            val javaVersion = compileKotlinTask.kotlinJavaToolchain.javaVersion.orNull


I'd like to keep it lazy as well.

You could do it by defining such class

private class ModuleInfoFilter( private val compileKotlinTaskPath: String, private val javaVersionProvider: Provider<JavaVersion>, private val moduleInfoFile: File, private val logger: Logger ) : Spec<FileTreeElement> { private val isJava9Compatible get() = javaVersionProvider.orNull?.isJava9Compatible == true private var logged = false private fun logStatusOnce() { if (logged) return if (isJava9Compatible) { logger.info("Module-info checking is enabled; $compileKotlinTaskPath is compiled using Java ${javaVersionProvider.get()}") } else { logger.info("Module-info checking is disabled") } logged = true } override fun isSatisfiedBy(element: FileTreeElement): Boolean { logStatusOnce() if (isJava9Compatible) return false return element.file == moduleInfoFile } }

and then use it like

val javaVersionProvider = compileKotlinTask.kotlinJavaToolchain.javaVersion compileKotlinTask.exclude(ModuleInfoFilter(compileKotlinTask.path, javaVersionProvider, sourceFile, logger))

Yes, it's more verbose, but performs the check lazily. Otherwise it depends on the order, so if the task configured before toolchains, then it may get an incorrect Java version.

ALikhachev · 2023-02-16T13:26:24Z

buildSrc/src/main/kotlin/Java9Modularity.kt

+            modularity.inferModulePath.set(true)
+        }
+
+        tasks.getByName<Jar>(target.artifactsTaskName) {


Please change it to

Suggested change

tasks.getByName<Jar>(target.artifactsTaskName) {

tasks.named<Jar>(target.artifactsTaskName) {

You're triggering unconditional task configuration even on running ./gradlew help:

qwwdfsad · 2023-02-17T15:32:06Z

I've applied all the proposed suggestions in #3629, will keep you posted regarding further actions

qwwdfsad · 2023-02-20T13:20:24Z

I've run a few out integration tests, so far so good. Our JPMS-related testing capabilities are limited (because we are not really using JPMS ourselves anywhere), so I've published 1.7.0-jpms-preview to https://maven.pkg.jetbrains.space/public/p/kotlinx-coroutines/maven repository; if you are relying on JPMS in your project, consider checking it out.

qwwdfsad · 2023-02-21T10:50:28Z

Merged manually. Thanks!

lion7 mentioned this pull request May 19, 2022

Support the Java Module System #2237

Closed

qwwdfsad self-requested a review June 1, 2022 15:44

qwwdfsad reviewed Jun 1, 2022

View reviewed changes

qwwdfsad self-assigned this Jun 1, 2022

qwwdfsad added the for 1.7 label Jun 1, 2022

lion7 force-pushed the develop branch from b02e2a4 to ca247ee Compare June 1, 2022 18:54

lion7 force-pushed the develop branch from ca247ee to 65773f0 Compare October 27, 2022 09:50

qwwdfsad self-requested a review October 27, 2022 10:11

lion7 force-pushed the develop branch from 65773f0 to 4393520 Compare October 27, 2022 11:41

qwwdfsad requested changes Oct 27, 2022

View reviewed changes

qwwdfsad mentioned this pull request Oct 31, 2022

Support Java Platform Module System (JPMS) #2901

Closed

lion7 force-pushed the develop branch from 4393520 to 97502a4 Compare November 5, 2022 18:24

qwwdfsad self-requested a review December 7, 2022 12:52

Add explicit module-info.java for JPMS compatibility

bf8e323

lion7 force-pushed the develop branch from 97502a4 to bf8e323 Compare January 7, 2023 14:19

ALikhachev requested changes Feb 16, 2023

View reviewed changes

qwwdfsad mentioned this pull request Feb 17, 2023

Add explicit module-info.java for JPMS compatibility #3629

Merged

qwwdfsad closed this Feb 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit module-info.java for JPMS compatibility #3297

Add explicit module-info.java for JPMS compatibility #3297

lion7 commented May 19, 2022

lion7 commented May 19, 2022

qwwdfsad left a comment

qwwdfsad Jun 1, 2022

lion7 commented Jun 1, 2022

elizarov commented Jun 2, 2022

qwwdfsad commented Oct 13, 2022

lion7 commented Oct 27, 2022

qwwdfsad Oct 27, 2022

lion7 Nov 7, 2022

TheMrMilchmann Nov 7, 2022

qwwdfsad Oct 27, 2022

lion7 Nov 7, 2022

qwwdfsad commented Oct 27, 2022

lion7 commented Nov 7, 2022 •

edited

Loading

qwwdfsad commented Nov 15, 2022 •

edited

Loading

CodeMason commented Jan 2, 2023

lion7 commented Jan 7, 2023 •

edited

Loading

qwwdfsad commented Jan 9, 2023

qwwdfsad commented Feb 16, 2023

ALikhachev Feb 16, 2023

ALikhachev Feb 16, 2023

ALikhachev Feb 16, 2023

ALikhachev Feb 16, 2023

ALikhachev Feb 16, 2023 •

edited

Loading

ALikhachev Feb 16, 2023 •

edited

Loading

qwwdfsad commented Feb 17, 2023

qwwdfsad commented Feb 20, 2023

qwwdfsad commented Feb 21, 2023

	exports kotlinx.coroutines.flow.internal;
	exports kotlinx.coroutines.flow.internal to kotlinx.coroutines.reactive;

	val javaToolchains = extensions.getByName("javaToolchains") as JavaToolchainService
	val javaToolchains = extensions.findByType(JavaToolchainService::class.java)
	?: error("Gradle JavaToolchainService is not available")

	tasks.getByName<Jar>(target.artifactsTaskName) {
	tasks.named<Jar>(target.artifactsTaskName) {

Add explicit module-info.java for JPMS compatibility #3297

Add explicit module-info.java for JPMS compatibility #3297

Conversation

lion7 commented May 19, 2022

lion7 commented May 19, 2022

qwwdfsad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lion7 commented Jun 1, 2022

elizarov commented Jun 2, 2022

qwwdfsad commented Oct 13, 2022

lion7 commented Oct 27, 2022

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

qwwdfsad commented Oct 27, 2022

lion7 commented Nov 7, 2022 • edited Loading

qwwdfsad commented Nov 15, 2022 • edited Loading

CodeMason commented Jan 2, 2023

lion7 commented Jan 7, 2023 • edited Loading

qwwdfsad commented Jan 9, 2023

qwwdfsad commented Feb 16, 2023

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

ALikhachev Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

ALikhachev Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

qwwdfsad commented Feb 17, 2023

qwwdfsad commented Feb 20, 2023

qwwdfsad commented Feb 21, 2023

lion7 commented Nov 7, 2022 •

edited

Loading

qwwdfsad commented Nov 15, 2022 •

edited

Loading

lion7 commented Jan 7, 2023 •

edited

Loading

ALikhachev Feb 16, 2023 •

edited

Loading

ALikhachev Feb 16, 2023 •

edited

Loading