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 targets hierarchy API #4349

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Use targets hierarchy API #4349

merged 3 commits into from
Sep 30, 2024

Conversation

osipxd
Copy link
Member

@osipxd osipxd commented Sep 26, 2024

Subsystem
Infrastructure

Motivation
KTOR-7501 Simplify targets hierarchy configuration
More context here: #4045

Solution
Configured target hierarchy using new targets hierarchy DSL

@osipxd osipxd force-pushed the osipxd/targets-hierarchy branch 5 times, most recently from bd34159 to f68b079 Compare September 26, 2024 20:41
@osipxd osipxd marked this pull request as ready for review September 27, 2024 06:46
@osipxd osipxd requested review from bjhham and e5l September 27, 2024 06:46
@whyoleg
Copy link
Contributor

whyoleg commented Sep 27, 2024

FYI: there was an attempt previously to migrate to target hierarchy. #4044 and #4045
But it was reverted (AFAIK by @e5l) because of some issues with long paths on Windows

configureJsTestTasks()
}

private fun Project.configureJsTasks() {
private fun Project.configureJsTasks(nodeJsEnabled: Boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: probably this could be inclined into DSL above to simplify the buildscript

sourceSets {
jvmMain {
dependencies {
if (jdk > 6) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: there is no need anymore to provide different versions of stdlib as everything now is available in just stdlib - which is applied by default.
the same for coroutines dependency.

So it's possible to simplify everything and just not add stdlib/coroutines in platform sourceSets, as coroutines are already added in CommonConfig

val linuxTest by getting {
findByName("nixTest")?.let { dependsOn(it) }
common {
withCompilations { true }
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this line is not needed

}
}
}
}

configureWasmTasks()
Copy link
Contributor

Choose a reason for hiding this comment

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

the same here as with JS

@osipxd osipxd force-pushed the osipxd/targets-hierarchy branch from f68b079 to a5a3b17 Compare September 27, 2024 10:56
@osipxd
Copy link
Member Author

osipxd commented Sep 27, 2024

FYI: there was an attempt previously to migrate to target hierarchy. #4044 and #4045
But it was reverted (AFAIK by @e5l) because of some issues with long paths on Windows

@e5l do you remember what problem was there? The only thing I found is that it was reverted as part of the migration to kotlinx-io (#4032)

@osipxd osipxd self-assigned this Sep 27, 2024
@e5l
Copy link
Member

e5l commented Sep 27, 2024

@osipxd, import, and build failed on the Windows machine because of the long cache path generated by the Kotlin Gradle plugin. We need to check Windows project import in the IDE

@osipxd
Copy link
Member Author

osipxd commented Sep 27, 2024

We need to check Windows project import in the IDE

Okay, I have a windows machine, so I will check

@osipxd
Copy link
Member Author

osipxd commented Sep 29, 2024

Import and build work fine on my Windows PC. Nevertheless, I've enabled Windows build on the CI to check it also there.

@osipxd
Copy link
Member Author

osipxd commented Sep 30, 2024

Let's give it a try 🤞

@osipxd osipxd merged commit 3f43f90 into main Sep 30, 2024
11 of 15 checks passed
@osipxd osipxd deleted the osipxd/targets-hierarchy branch September 30, 2024 10:23
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.

4 participants