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

Use KotlinHierarchyTemplate for structuring source sets #4045

Merged
merged 1 commit into from
May 28, 2024

Conversation

antohaby
Copy link
Collaborator

@antohaby antohaby commented May 22, 2024

KotlinHierarchyTemplate allows writing KMP source set structures with less code and without explicit dependsOn relations.

There is one downside is that all source sets are created now if at least one of the target is declared. i.e. for example if JS target is declared but wasm is not. Then jsAndWasmSharedMain source set will be created anyway.

It also fixes the issue with Kotlin 2.0
In :ktor-network jvmAndNixTest has an expect that in K1 was actualized in iosTest there was no direct or transitive dependsOn relation from iosTest to jvmAndNixTest and thus in K2 such actualization was prohibited. In this commit Source Set structure is sanitized and now dependsOn relations are in good order.

Subsystem
Gradle build

Motivation
Depends on relations between Kotlin Source Sets are messed in Ktor project. And Ktor is not to blame here, but Kotlin Multiplatform's dependsOn relations. They are error prone and very verbose. And since K2 Kotlin Compiler requires correctness in Source Set structure. i.e. in K1 due to its compilation model it was possible to violate symbols visibility and expect/actual. A bit more details you can find in related issue: https://youtrack.jetbrains.com/issue/KT-68212/K2-KMP-has-no-corresponding-expected-declaration-with-dependsOn-relationship

This is how SourceSet structure looked like in :ktor-network project. Impossible to read.
image

Same Source Set structure but normalized i.e. redundant depends on edges are removed
image

But take a look for example at iosTest you can see that it doesn't connect neither to commonTest nor to jvmAndNixTest.
And other source sets are also dongle here and there.

Solution
Apply kotlin target hierarchy template. there is some information on kotlinlang web site https://kotlinlang.org/docs/multiplatform-hierarchy.html#default-hierarchy-template

After applying the target hierarchy source set structure look much cleaner for both main and test source sets. And it doesn't have to be normalized. It is already in a good shape.
image

PS: kotlin-community/dev branch also has related PR (!4044)

KotlinHierarchyTemplate allows writing KMP source set structures with less code
and without explicit dependsOn relations.

There is one downside is that all source sets are created now if at least
one of the target is declared. i.e. for example if JS target is declared
but wasm is not. Then jsAndWasmSharedMain source set will be created anyway.

It also fixes the issue with Kotlin 2.0 KT-68212
In :ktor-network jvmAndNixTest has an expect that in K1 was actualized in iosTest
there was no direct or transitive dependsOn relation from iosTest to jvmAndNixTest
and thus in K2 such actualization was prohibited. In this commit Source Set
structure is sanitized and now dependsOn relations are in good order.

^KT-68212 Fixed
@antohaby antohaby requested review from e5l and igoriakovlev May 22, 2024 16:23
@e5l
Copy link
Member

e5l commented May 23, 2024

Hey @antohaby, thanks for the PR. I'll check it next week after the conference!

@e5l e5l merged commit c03c0f0 into main May 28, 2024
11 of 16 checks passed
@e5l e5l deleted the alakotka/k2_source_set_structure_fix branch May 28, 2024 07:08
@e5l
Copy link
Member

e5l commented May 28, 2024

Hey @antohaby, thanks for the PR. LGTM

e5l added a commit that referenced this pull request May 30, 2024
tbogdanova added a commit that referenced this pull request Jun 7, 2024
e5l added a commit that referenced this pull request Jul 3, 2024
e5l added a commit that referenced this pull request Jul 15, 2024
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