Draft: Introduce basic Kotlin MPP project layout #77

TheTeXnician · 2021-01-03T10:23:38Z

This PR tries to tackle the ROADMAP item of introducing MPP project layout. It does not do much more than renaming. I have run the tests but did not do much more. Some aspects that I have not refactored which will need refactoring:

publishing: Kotlin MPP has to be published using multiple artifacts. Look at https://kotlinlang.org/docs/reference/mpp-publish-lib.html for a short introduction. However, I have not been able to get through all of the publication code.
sources: Kotlin MPP produces sources JARs in the publication process (cf. link above). The current code using source set main does not work as that source set is empty.
javadoc: This project produces a javadoc JAR but as I have not seen javadoc annotations, I have not done anything there. Currently, the javadocJAR is as empty as the sourcesJAR. If there were documentation comments I would recommend moving to dokka and using the output of the dokkaJavadoc task.
snakeYaml: This project (obviously) needs snakeYaml which is needed for the core components. One would probably rewrite those using expect and actual classes and use other serialization libraries based on the platform.

So, please see this as a starter in the direction of making kaml a MPP. If the publication code would be fixed, one could apply this layout to a JVM-only library as well until there is enough time for making it really platform-independent.

charleskorn

Thanks for the PR @TheTeXnician! This looks good to me. I've added a couple of comments on the Gradle configuration that I'm not sure about.

Also, is there a reason why some of the test files (YamlNodeReaderTest, YamlReadingTest and YamlWritingTest) are in jvmTest rather than commonTest?

In terms of the remaining work, I think you're correct in your assessment that the main thing is to get the publishing code working before this can be merged. Is that something you're willing to look into?

charleskorn · 2021-01-03T21:54:16Z

build.gradle.kts

-
-    apply { id("com.github.ben-manes.versions") version "0.36.0" }
+    kotlin("multiplatform") version "1.4.10"
+    id("org.jetbrains.kotlin.plugin.serialization") version "1.4.10"


I know I wasn't using it before, but it might be good to switch to the kotlin("plugin.serialization") method here as well, as described in the kotlinx.serialization readme.

Good catch. Of course it should.

charleskorn · 2021-01-03T21:54:43Z

build.gradle.kts

+        }
+        val jvmMain by getting {
+            dependencies {
+                implementation(project.dependencies.platform("org.jetbrains.kotlin:kotlin-bom"))


Should this be applied to kotlinMain?

I really don't know what's kotlinMain. The source sets are named like the targets (i.e. js, jvm, common, linuxX64, …) where there is no kotlin target. As per this forum post it seems that the platform is nothing that's even supposed to be used in Kotlin/MPP (maybe yet). So this seems to be the currently most viable workaround.

charleskorn · 2021-01-03T21:54:57Z

build.gradle.kts

+                implementation(project.dependencies.platform("org.jetbrains.kotlin:kotlin-bom"))
+                implementation(kotlin("stdlib-jdk8"))
+                implementation("org.snakeyaml:snakeyaml-engine:${Versions.snakeYaml}")
+                implementation("org.jetbrains.kotlinx:kotlinx-serialization-core:${Versions.serialization}")


Does this need to be applied again if it's already in commonMain?

Another good catch. It does not. jvmMain depends on commonMain automatically.

charleskorn · 2021-01-03T21:55:58Z

build.gradle.kts

+        val jvmTest by getting {
+            dependencies {
+                implementation("org.spekframework.spek2:spek-dsl-jvm:${Versions.spek}")
+                implementation("ch.tutteli.atrium:atrium-fluent-en_GB:${Versions.atrium}")


Same thing again - does this need to be here if it's also in commonTest? (I haven't worked extensively with a MPP project before, so forgive me if this is a dumb question 😄)

This is quite the opposite. jvmTest does not depend on commonTest and it would not make much sense as the JVM implementations of the frameworks are usually more powerful and you don't need conflicting definitions in your classpath. I'd leave it here as it does no harm (version is centralized elsewhere…).

charleskorn · 2021-01-03T21:56:50Z

buildSrc/src/main/kotlin/com/charleskorn/kaml/build/Versions.kt

+
+package com.charleskorn.kaml.build
+
+object Versions {


Does Renovate support updating versions when configured like this?

I don't know about Renovate (never heard of it) but you apply the versions plugin which I'm also using in a number of projects configured this way. That plugin works with such a versions object.

TheTeXnician · 2021-01-04T07:31:57Z

Thanks for the PR @TheTeXnician! This looks good to me. I've added a couple of comments on the Gradle configuration that I'm not sure about.

Thanks for the comments. Great pointers to things I overlooked :)

Also, is there a reason why some of the test files (YamlNodeReaderTest, YamlReadingTest and YamlWritingTest) are in jvmTest rather than commonTest?

Yes. Those tests (as far as I could see) depend on components that are JVM-only at the moment, e.g. YamlParser as they use SnakeYaml features. As soon as there is reliance on components from jvmMain they cannot be tested in commonTest but have to be tested in jvmMain. One should change this once there are expect definitions for these components so that the tests are available on all platforms.

In terms of the remaining work, I think you're correct in your assessment that the main thing is to get the publishing code working before this can be merged. Is that something you're willing to look into?

Unfortunately, I don't think I'll have the time at the moment. Your publishing code looks quite complex to me. I'm part of the Island of TeX which published a MPP library some weeks ago (see very simplistic build file of that publication in the repo). So if you would want to tackle it, you should ;)
Otherwise, I could try around April when I've got more time for open source again to address publishing code.

TheTeXnician added 5 commits January 3, 2021 10:20

Transform main build file into Kotlin MPP setup

fff656b

Move dependency-free classes to common

5bd60c6

Move multiple tests into common

8da5e07

Reduce version repetition

4fa1c21

Move more JVM testing to common

1673f81

TheTeXnician changed the title ~~Introduce basic Kotlin MPP project layout~~ Draft: Introduce basic Kotlin MPP project layout Jan 3, 2021

charleskorn reviewed Jan 3, 2021

View reviewed changes

Address review comments

35f28f7

Base automatically changed from master to main February 6, 2021 22:16

charleskorn added the work in progress label Feb 20, 2021

charleskorn marked this pull request as draft February 22, 2021 07:46

charleskorn merged commit f0e7e59 into charleskorn:main Mar 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Introduce basic Kotlin MPP project layout #77

Draft: Introduce basic Kotlin MPP project layout #77

TheTeXnician commented Jan 3, 2021

charleskorn left a comment

charleskorn Jan 3, 2021

TheTeXnician Jan 4, 2021

charleskorn Jan 3, 2021

TheTeXnician Jan 4, 2021

charleskorn Jan 3, 2021

TheTeXnician Jan 4, 2021

charleskorn Jan 3, 2021

TheTeXnician Jan 4, 2021

charleskorn Jan 3, 2021

TheTeXnician Jan 4, 2021

TheTeXnician commented Jan 4, 2021


		package com.charleskorn.kaml.build

		object Versions {

Draft: Introduce basic Kotlin MPP project layout #77

Draft: Introduce basic Kotlin MPP project layout #77

Conversation

TheTeXnician commented Jan 3, 2021

charleskorn 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheTeXnician commented Jan 4, 2021