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

Update dependencies and clean up build scripts #423

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

FenstonSingel
Copy link
Contributor

Quick summary:

  • Gradle 8.9
  • Kotlin 2.0.10
  • cleanup of deprecated API and outdated issue workarounds
  • (re)organization and (re)structuring of build scripts' code
  • Gradle version catalog
  • cleaner configuration code for builds as a Kotlin user project

Rationale is a slightly better DX when working with a version catalog (which is to be introduced in a later commit).

Usages of Property API now leverage the assignment Kotlin compiler plugin instead of invoking `set` methods directly.

Usages of deprecated Gradle API are either replaced with supported alternatives or removed due to not really doing anything.
KGP now uses the `.kotlin` folder for purposes that it previously used the `.gradle` folder for; so the `.kotlin` folder is now excluded from the Git repository as well.

Kotlin-related Gradle properties from `gradle.properties` are cleaned up and actualized:
* `kotlin.js.compiler` is removed
* `kotlin.native.ignoreIncorrectDependencies` is prepared for its deprecation in 2.0.20
* `kotlin.wasm.stability.nowarn` is temporarily added until its deprecation in 2.0.20

Usages of deprecated `kotlinOptions` API are migrated to supported `compilerOptions` API (see KT-57292 for more details).

(Application of `-version` to Kotlin compilation tasks is just removed altogether; applying it only to K/JVM tasks seems questionable, and applying it to K/N tasks stops them from actually compiling anything.)

Usages of all experimental KGP API now opt into said API. Usages of all outdated KGP API are removed.

An error suppression warning (see KT-61129 for more details) is disabled due to a high number of hits throughout the repository.

A usage of deprecated `kotlin.native.ThreadLocal` annotation is migrated to supported `kotlin.native.concurrent.ThreadLocal` annotation.

Workarounds for the following issues are removed from the repository's build scripts:
* node.js not supporting "the latest Wasm"
* node.js not supporting Wasm GC milestone 4
* KT-58303
It will be reintroduced in a later commit.
There are three potentially important changes in this commit:
* a workaround for Bintray publishing is removed because of, to quote a gradle.git#11412 commentator, "JFrog [electing] to shut down JCenter and Bintray rather than fix the problem"
* `:kotlinx-datetime:compileCommonJsMainKotlinMetadata` task is enabled back to preserve compilation correctness (see KT-66382 for more details)
* `:benchmarks` module's `jmh` source set is now also compiled during repository-wide builds (so that it's covered during builds as a Kotlin user project)

Other than that, this commit is a bundle of minor, mostly structural tweaks for the sake of easier understanding and maintainability.
@FenstonSingel FenstonSingel requested review from ilya-g and dkhalanskyjb and removed request for ilya-g and dkhalanskyjb August 9, 2024 02:24
@FenstonSingel
Copy link
Contributor Author

FenstonSingel commented Aug 9, 2024

It makes sense to wait until #366 is merged into the master branch so that changes in build scripts from that PR could also be accounted for in this PR.

Copy link
Member

@ilya-g ilya-g left a comment

Choose a reason for hiding this comment

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

I believe we're going to merge #366 first because it's already ready and is just waiting until we finish the version 0.6.1 and start developing 0.7.0 in master.
This branch will likely introduce many conflicts if merged before that.

target("linuxArm64")
// Tier 4 (deprecated, but still in demand)
// Deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Please retain the comment that it is still in demand

}

js {
@OptIn(ExperimentalWasmDsl::class)
wasmJs {
Copy link
Member

Choose a reason for hiding this comment

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

Why wasmJs was moved higher? It's the most recent target amongst all, so we put it last by convention

}
}
}

tasks {
Copy link
Member

Choose a reason for hiding this comment

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

What's wrong with using tasks dsl block to avoid repetitive tasks. prefix?

mavenCentral()
gradlePluginPortal()
maven(url = "https://maven.pkg.jetbrains.space/kotlin/p/kotlinx/maven")
Copy link
Member

Choose a reason for hiding this comment

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

We have put it first to ensure that infra plugin is obtained only from there.

}
}

// Publish benchmarks to the root for the easier 'java -jar benchmarks.jar`
// publish benchmarks to the repository root for easier `java -jar benchmarks.jar`
tasks.named<Jar>("jmhJar") {
Copy link

Choose a reason for hiding this comment

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

Suggested change
tasks.named<Jar>("jmhJar") {
tasks.jmhJar {

repositories {
mavenCentral()
// compile all Kotlin code from the module during full-repository builds
tasks.named("assemble") {
Copy link

Choose a reason for hiding this comment

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

Suggested change
tasks.named("assemble") {
tasks.assemble {


plugins {
id("kotlin")
id("me.champeau.jmh")
with(libs.plugins) {
Copy link

Choose a reason for hiding this comment

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

Prefer not using with to keep things as simple as possible in plugins.

Comment on lines +28 to +29
tasks.withType<KotlinCompilationTask<*>>().forEach { compileKotlinTask ->
dependsOn(compileKotlinTask)
Copy link

Choose a reason for hiding this comment

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

Suggested change
tasks.withType<KotlinCompilationTask<*>>().forEach { compileKotlinTask ->
dependsOn(compileKotlinTask)
dependsOn(tasks.withType<KotlinCompilationTask<*>>())

Comment on lines +42 to +43
freeCompilerArgs.add("-Xexpect-actual-classes")
freeCompilerArgs.add("-Xdont-warn-on-error-suppression")
Copy link

Choose a reason for hiding this comment

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

Suggested change
freeCompilerArgs.add("-Xexpect-actual-classes")
freeCompilerArgs.add("-Xdont-warn-on-error-suppression")
freeCompilerArgs.addAll(
"-Xexpect-actual-classes",
"-Xdont-warn-on-error-suppression",
)

jvm {
attributes {
attribute(TargetJvmVersion.TARGET_JVM_VERSION_ATTRIBUTE, 8)
targets.withType<KotlinNativeTarget> {
Copy link

Choose a reason for hiding this comment

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

Suggested change
targets.withType<KotlinNativeTarget> {
targets.withType<KotlinNativeTarget>().configureEach {

@@ -322,7 +295,7 @@ val downloadWindowsZonesMapping by tasks.registering {
val output = "$projectDir/windows/src/internal/WindowsZoneNames.kt"
outputs.file(output)
doLast {
val initialFileContents = try { File(output).readBytes() } catch(e: Throwable) { ByteArray(0) }
val initialFileContents = try { File(output).readBytes() } catch(_: Throwable) { ByteArray(0) }
Copy link

Choose a reason for hiding this comment

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

Can be replaced by runCatching.

Comment on lines +3 to +4
distributionSha256Sum=d725d707bfabd4dfdc958c624003b3c80accc03f7037b5122c4b1d0ef15cecab
distributionUrl=https\://services.gradle.org/distributions/gradle-8.9-bin.zip
Copy link

Choose a reason for hiding this comment

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

Suggested change
distributionSha256Sum=d725d707bfabd4dfdc958c624003b3c80accc03f7037b5122c4b1d0ef15cecab
distributionUrl=https\://services.gradle.org/distributions/gradle-8.9-bin.zip
distributionSha256Sum=5b9c5eb3f9fc2c94abaea57d90bd78747ca117ddbbf96c859d3741181a12bf2a
distributionUrl=https\://services.gradle.org/distributions/gradle-8.10-bin.zip

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