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 #4044

Merged
merged 1 commit into from
May 23, 2024

Conversation

antohaby
Copy link
Collaborator

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.

Copy link
Contributor

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

just passing by...
it's just nice to see how this new hierarchy API simplifies complex sourceSet hierarchies

val jsAndWasmSharedTest by creating {
dependsOn(commonTest)
@OptIn(ExperimentalKotlinGradlePluginApi::class)
val ktorTargetsHierarchy = KotlinHierarchyTemplate {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious: is there a reason to extract this into a separate variable instead of just calling applyHierarchyTemplate {...}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is no reason. so it can be merged into one call.

@OptIn(ExperimentalKotlinGradlePluginApi::class)
val ktorTargetsHierarchy = KotlinHierarchyTemplate {
// extract nix group so it can be reused later as `group("nix")`
group("nix") {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious[2]: is there a reason to declare it top-level and not under common? Does it make any difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it can be moved under common. however then it might look not entirely correct as I don't want nix to be dependsOn common, but only on posix and jvmAndNix (but apply target hierarchy algorithm is capable to solve this problem as in this case it will not add direct dependsOn edge).

again I wasn't doing that with much of a through thinking of where exactly to place this group.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but apply target hierarchy algorithm is capable to solve this problem as in this case it will not add direct dependsOn edge

or maybe not, I need to double check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or maybe not, I need to double check
checked. it is alright. here is the source set structure I get via this script:

tasks.register("printKMPSourceSetStructure") {
    doFirst {
        println("digraph {")
        kotlin.sourceSets.forEach { sourceSet ->
            sourceSet.dependsOn.forEach { dependsOn ->
                println("  \"${sourceSet.name}\" -> \"${dependsOn.name}\"")
            }
        }
        println("}")
    }
}

image

Copy link
Contributor

Choose a reason for hiding this comment

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

cool, thanks for explaining and checking :)

getByName("${it}Test").dependsOn(windowsTest)
}
}
if (hasJvmAndNix) { jvm(); nixTargets(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

just minor unrelated remark: those functions (nixTargets/posixTargets/etc) are extensions on Project - so it would be more clear to call them at least at kotlin extension level (because of jvm function call).
Even better to refactor them to be extensions on KotlinMultiplatformExtension - but of course it's out of scope here :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agree

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 force-pushed the alakotka/kotlin-community/dev branch from 463a5db to 0552f54 Compare May 22, 2024 15:31
@antohaby
Copy link
Collaborator Author

Ok I pushed changed based on Oleg's input.

There are few interesting pictures I'd like to show:

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

But here is normalized version where all redundant dependsOn edges is removed. And it is much cleaner
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.

With applyTargetHierarchy it look now like this. Much better and comprehendable. And I don't have to normalize this version either. So it is already in a good shape.
image

@tbogdanova tbogdanova merged commit f12b139 into kotlin-community/dev May 23, 2024
14 of 16 checks passed
@tbogdanova tbogdanova deleted the alakotka/kotlin-community/dev branch May 23, 2024 10:12
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