kotlinx.coroutines icon indicating copy to clipboard operation
kotlinx.coroutines copied to clipboard

Add explicit module-info.java for JPMS compatibility

Open lion7 opened this issue 2 years ago • 3 comments

lion7 avatar May 19 '22 14:05 lion7

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: image

lion7 avatar May 19 '22 14:05 lion7

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.

lion7 avatar Jun 01 '22 18:06 lion7

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.

elizarov avatar Jun 02 '22 15:06 elizarov

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?

qwwdfsad avatar Oct 13 '22 12:10 qwwdfsad

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)

lion7 avatar Oct 27 '22 10:10 lion7

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

qwwdfsad avatar Oct 27 '22 22:10 qwwdfsad

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.

lion7 avatar Nov 07 '22 12:11 lion7

@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)

qwwdfsad avatar Nov 15 '22 14:11 qwwdfsad

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

CodeMason avatar Jan 02 '23 15:01 CodeMason

@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?

lion7 avatar Jan 07 '23 18:01 lion7

Thanks! Will take care of it during this week

qwwdfsad avatar Jan 09 '23 10:01 qwwdfsad

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?

qwwdfsad avatar Feb 16 '23 10:02 qwwdfsad

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

qwwdfsad avatar Feb 17 '23 15:02 qwwdfsad

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 avatar Feb 20 '23 13:02 qwwdfsad

Merged manually. Thanks!

qwwdfsad avatar Feb 21 '23 10:02 qwwdfsad