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

Switching to Kotlin Multiplatform for JVM use #1654

Conversation

takahirom
Copy link
Contributor

@takahirom takahirom commented Aug 4, 2023

#304

This Pull Request aims to transition our codebase from org.jetbrains.kotlin.jvm to org.jetbrains.kotlin.multiplatform with JVM configuration. The primary intention behind this shift is to establish a foundation for adaptation to other platforms in the future.

Additionally, I encountered an issue when trying to run Dokka with a single-target multiplatform project using Kotlin 1.9.0. As a workaround, I've had to comment out the Dokka-related processes for now. You can find more information about this issue here: Kotlin/dokka#3038

Please review these changes, and let me know if there are any concerns or suggestions. Thanks!

Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

I'm fine with this as a starting point, but I would prefer it only be done for the main artifact.

build.gradle.kts Outdated Show resolved Hide resolved
@JakeWharton
Copy link
Member

I'll also note that the biggest inhibitor to doing this work already is needing something like https://youtrack.jetbrains.com/issue/KT-34668 or https://youtrack.jetbrains.com/issue/KT-20427. I tried migrating the project many years ago and found the duplication/indirection needed to be too much to bear.

@takahirom
Copy link
Contributor Author

@JakeWharton
Just to confirm, when you say 'the main artifact', are you referring to the ':kotlinpoet' module? So, should I apply the Kotlin Multiplatform plugin to the ':kotlinpoet' module and leave the other modules as they are?

@takahirom takahirom force-pushed the takahirom/switch-to-kotlin-mpp-plugin/2023-08-04 branch from 305930d to b85cfb6 Compare August 5, 2023 01:10
@takahirom
Copy link
Contributor Author

I have applied the KMP plugin exclusively to the kotlinpoet module. Please review the changes and let me know if there are any concerns or suggestions for improvement.

@takahirom
Copy link
Contributor Author

I don't know why, but it seems that the format has not been applied to the main branch.
I'm not sure if I should fix this or use excludeTarget("kotlinpoet/src/commonMain/kotlin/com/squareup/kotlinpoet/ClassName.kt") as a spotless setting. 👀

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':kotlinpoet:spotlessKotlinCheck'.
> The following files had format violations:
      src/commonMain/kotlin/com/squareup/kotlinpoet/ClassName.kt
          @@ -1,5 +1,5 @@
           /*
8 actionable tasks: 8 executed
          -·*·Copyright·(C)·2014·Google,·Inc.
          +·*·Copyright·(C)·2014·Square,·Inc.

@JakeWharton
Copy link
Member

Add the exclude, please. Basically assume whatever is in the file currently is correct.

@takahirom
Copy link
Contributor Author

Oh, sorry, I overlooked the 'targetExclude' in the kotlinpoet module. I've fixed it since the source set name changed.
665b933

@takahirom
Copy link
Contributor Author

TypeNameTest and JavaClassWithArrayValueAnnotation are Java classes in the test. We can separate the Java test module. But it may take some maintenance costs. What do you think?

@takahirom
Copy link
Contributor Author

takahirom commented Aug 10, 2023

I was able to run the Java-related tests. 564b448
I didn't know that we could compile Java code in jvmTest/java.
image

Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

So this all looks great, but I'm not sure we can land it without figuring out the Dokka situation because that's a bit of a blocker.

@takahirom takahirom force-pushed the takahirom/switch-to-kotlin-mpp-plugin/2023-08-04 branch from 564b448 to 06bfb03 Compare August 27, 2023 08:30
@takahirom takahirom force-pushed the takahirom/switch-to-kotlin-mpp-plugin/2023-08-04 branch from d8d8e0f to 6ede2fd Compare August 27, 2023 08:39
@takahirom
Copy link
Contributor Author

I've applied the workaround for Dokka from this link: Kotlin/dokka#3122. Now, we can view the Dokka report.
image

@Ayfri
Copy link

Ayfri commented Sep 19, 2023

Any update ?

@JakeWharton
Copy link
Member

Note that merging this does not immediately change the supported targets. It's still JVM only.

kotlinpoet/build.gradle.kts Outdated Show resolved Hide resolved
kotlinpoet/build.gradle.kts Outdated Show resolved Hide resolved
kotlinpoet/build.gradle.kts Outdated Show resolved Hide resolved
kotlinpoet/build.gradle.kts Outdated Show resolved Hide resolved
gradle/libs.versions.toml Outdated Show resolved Hide resolved
Comment on lines +66 to +70
tasks.named<org.jetbrains.kotlin.gradle.tasks.KotlinCompile>("compileTestKotlinJvm") {
compilerOptions {
freeCompilerArgs.add("-opt-in=com.squareup.kotlinpoet.DelicateKotlinPoetApi")
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@JakeWharton JakeWharton merged commit 69c8b38 into square:main Oct 18, 2023
2 checks passed
@takahirom takahirom deleted the takahirom/switch-to-kotlin-mpp-plugin/2023-08-04 branch November 19, 2023 09:20
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.

3 participants