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

Bump Kotlin versions to 2.0.0-RC1 #3516

Merged
merged 16 commits into from Apr 22, 2024
Merged

Bump Kotlin versions to 2.0.0-RC1 #3516

merged 16 commits into from Apr 22, 2024

Conversation

IgnatBeresnev
Copy link
Member

@IgnatBeresnev IgnatBeresnev commented Mar 5, 2024

  • Bumps KGP to 2.0.0-RC1
  • Bumps kotlin-compiler to 2.0.0-RC1 (only K1)

# See: https://kotlinlang.org/docs/gradle-configure-project.html#apply-the-plugin
gradlePlugin-android = "4.2.2"
gradlePlugin-android-dokkatoo = "8.0.2"
gradlePlugin-android = "7.1.3"
Copy link
Member Author

Choose a reason for hiding this comment

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

gradlePlugin-android = "4.2.2"
gradlePlugin-android-dokkatoo = "8.0.2"

These are unused at the moment, so I've removed them (and the dependencies that depend on them) to reduce noise. Can be added once it's needed

gradlePlugin-android = "7.1.3"

KGP 2.0.0 now requires AGP 7.1.3+, so I had to bump it as well

Comment on lines 8 to 9
gradlePlugin-gradle-customUserData = "1.12"
gradlePlugin-gradle-enterprise = "3.14.1"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense to have the most important / frequently updated dependencies on the top, and it's unlikely that Gradle enterprise will be looked for / updated all that often. Just moved it closer to the end to reduce noise

kotlin-compiler-k2 = "2.0.0-dev-8561"

# MUST match the version of the intellij platform used in the kotlin compiler,
# otherwise this will lead to different versions of psi API and implementations
# on the classpath and will fail with hard to debug problems in runtime.
# See: https://github.com/JetBrains/kotlin/blob/e6633d3d9214402fcf3585ae8c24213a4761cc8b/gradle/versions.properties#L1
intellij-platform = "213.7172.25"
intellij-platform = "213.7172.53"
Copy link
Member Author

Choose a reason for hiding this comment

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

@IgnatBeresnev
Copy link
Member Author

Some context on the konanDistribution hack

The solution in #3147 uses an internal property from KGP with @Suppress("INVISIBLE_MEMBER").

In KGP <= 1.9.20 the type of this property is String (Project.konanHome: String), so everything works as expected.

In KGP 2.0.0 the type of the property was changed to File: JetBrains/kotlin@7165d15#diff-c0ac64e93aa7f4c033b27d586757c6493f31699ca28d0b5873a60d9693fefa2cL35-R38

This property is accessed only if the version of KGP in the user project is <= 1.9.20. However, if we compile Dokka 2.0.0 against KGP 2.0.0 (in which konanHome has a different type), and run it with KGP <=1.9.20, we'll get

java.lang.NoSuchMethodError: org.jetbrains.kotlin.compilerRunner.NativeToolRunnersKt.getKonanHome(Lorg/gradle/api/Project;)Ljava/io/File;
  at org.jetbrains.dokka.gradle.kotlin.KotlinNativeDistributionAccessor.<init>(KotlinNativeDistributionAccessor.kt:31)

Because Dokka was compiled against konanHome: File (2.0.0), but only konanHome: String (1.9.20) is present.

This hack adds reflection access to the expected 1.9.20 property (String). The code for alternativeKonanHome is taken from #3145 as a backup option - everything seems to work without it, but you never know.

I've also added a project property org.jetbrains.dokka.classpath.excludeKonanPlatformDependencyFiles in case both solutions return null and start failing user builds. If some user is unable to update KGP to 2.0.0 to fix this problem, but for some reason needs to update the version of Dokka to 2.0.0 - they will be able to generate documentation, albeit incomplete.

@IgnatBeresnev IgnatBeresnev marked this pull request as ready for review March 14, 2024 10:31
@IgnatBeresnev IgnatBeresnev requested review from vmishenev and whyoleg and removed request for vmishenev and whyoleg March 14, 2024 10:31
@IgnatBeresnev IgnatBeresnev marked this pull request as draft March 14, 2024 10:33
@@ -2,6 +2,6 @@
# Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
#

dokka_it_kotlin_version=1.9.22
dokka_it_kotlin_version=2.0.0-Beta4
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, why do we need to have this property here?
AFAIU we override this property (and dokka_it_android_gradle_plugin_version) anyway when running integration tests via template.settings.gradle.kts and so this value doesn't affect anything

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I have no idea :) We can try to remove it in a separate PR, see if the tests fail

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the version is set as a default in IT projects, so if someone wants to open them in IJ and edit them, it's easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

But the version is used mostly in template.settings.gradle.kts (with small exceptions), so to open the project in IDEA, you will still need to copy it inside project folder, or copy it's content
So, I would propose to remove this (in some future PR), if possible, to reduce amount of places where we need to update Kotlin version (or any other version), so that at some point we will not have some situation, that this version is not updated or not overridden in integration tests

@IgnatBeresnev
Copy link
Member Author

The integration tests are failing with

Execution failed for task ':commonizeNativeDistribution'.
    > Could not resolve all files for configuration ':kotlinNativeBundleConfiguration'.
        > Could not find org.jetbrains.kotlin:kotlin-native-prebuilt:2.0.0-Beta4.

At first I thought that it was an incompatible change in Kotlin / KGP (https://youtrack.jetbrains.com/issue/KT-64181), but after manually checking it, I realized it could be specific to our integration tests, and indeed this configuration might be causing the failure:

// Declare Kotlin/Native dependencies - workaround for https://youtrack.jetbrains.com/issue/KT-51379
exclusiveContent {
forRepository {
ivy("https://download.jetbrains.com/kotlin/native/builds") {
name = "Kotlin Native"
patternLayout {
// example download URLs:
// https://download.jetbrains.com/kotlin/native/builds/releases/1.7.20/linux-x86_64/kotlin-native-prebuilt-linux-x86_64-1.7.20.tar.gz
// https://download.jetbrains.com/kotlin/native/builds/releases/1.7.20/windows-x86_64/kotlin-native-prebuilt-windows-x86_64-1.7.20.zip
// https://download.jetbrains.com/kotlin/native/builds/releases/1.7.20/macos-x86_64/kotlin-native-prebuilt-macos-x86_64-1.7.20.tar.gz
listOf(
"macos-x86_64",
"macos-aarch64",
"osx-x86_64",
"osx-aarch64",
"linux-x86_64",
"windows-x86_64"
).forEach { os ->
listOf("dev", "releases").forEach { stage ->
artifact("$stage/[revision]/$os/[artifact]-[revision].[ext]")
}
}
}
metadataSources { artifact() }
}
}
filter { includeModuleByRegex(".*", ".*kotlin-native-prebuilt.*") }
}

@adam-enko could you please help here? It's a recent change from #3492, so hopefully it's still fresh in memory

@adam-enko
Copy link
Contributor

adam-enko commented Mar 25, 2024

@adam-enko could you please help here? It's a recent change from #3492, so hopefully it's still fresh in memory

Sure thing, it's a simple fix.

Explanation of the situation:

  • KGP 1.x automatically added a custom Ivy repo for downloading K/N artifacts.
  • template.settings.gradle.kts set repositoriesMode.set(RepositoriesMode.PREFER_SETTINGS) (which is a good thing, preventing Gradle plugins from adding random repositories is more secure/stable/predictable), which blocked KGP from automatically adding the Ivy repo, so instead it had to be specified manually.
  • In KGP 2.x the K/N artifacts are uploaded to Maven Central, so no custom repo is necessary.
  • BUT the workaround I added used an exclusiveContent {} filter (to help Gradle performance, so it wouldn't unnecessarily try to search the Ivy repo for non-K/N content)
  • So when KGP 2.0 tried to download K/N artifacts, Gradle would only look in the custom Ivy repo, which doesn't contain K/N 2.0 content.

I mad a commit to just remove the exclusiveContent {} filter, so Gradle will look for K/N artifacts in either the Ivy repo or Maven Central.

@whyoleg whyoleg changed the title Bump Kotlin versions to 2.0.0-Beta4 Bump Kotlin versions to 2.0.0-RC1 Apr 9, 2024
@IgnatBeresnev IgnatBeresnev linked an issue Apr 9, 2024 that may be closed by this pull request
@whyoleg whyoleg marked this pull request as ready for review April 9, 2024 12:13
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.

LGTM

@whyoleg
Copy link
Contributor

whyoleg commented Apr 9, 2024

How important is the failing SequentialTasksExecutionStressTest?

Theoretically it could mean, that there is some kind of memory leak. But also, this could just mean, that more memory is needed.
After some short investigation (with profiler/memory graphs), I haven't found anything suspicious. So may be some more in-depth checks should be performed.

@IgnatBeresnev
Copy link
Member Author

IgnatBeresnev commented Apr 9, 2024

I think it's certainly worth investigating why the test is failing so consistently and for K2 only:

image

It's relatively stable with K1 and in the master branch.

As Oleg has pointed out, we've had instances where updating the Kotlin version led to memory leaks, but we've also had situations where the new Kotlin version required just a bit more memory than the previous one, in which case the allocated memory should be increased. Just have to rule memory leaks out as the result of the investigation, because if there's a problem somewhere in Kotlin - it's worth reporting it upstream as it's likely to affect other Kotlin users too

We'll try to take a look at it as part of this PR, so I guess we can halt the review

@whyoleg
Copy link
Contributor

whyoleg commented Apr 17, 2024

I've done some additional checks on how much memory is used, here are some graphs:

Test with K2 analysis built with Kotlin 2.0.0-RC1: current PR, this is what is failing on CI, works fine locally image
Test with K2 analysis built with Kotlin 1.9.22: current master image
Test with K1 analysis built with Kotlin 2.0.0-RC1: current PR, doesn't fail on CI image
Test with K1 analysis built with Kotlin 1.9.22: current master image

Note: integration test was run locally with the same Gradle (8.7) and KGP (2.0.0-RC1) versions.

Overall, all the graphs look fine, and most likely there is no memory leaks.
Still, K2 analysis uses slightly more metaspace memory (±60 MB), that's why most likely at some point this test starts to fail, as we do reinitialize ClassLoader for Dokka execution in Gradle plugin for every task execution. While locally it works fine (no crash), on CI there could be less resources - and so at some point ClassLoader for Dokka execution can not be loaded, and so fails with OOM.

I would propose to increase -XX:MaxMetaspaceSize in this specific test from 400 to 500 and see what's going on.
If it works, then most likely there is no leaks.

Update: integration tests are green with MaxMetaspaceSize=500!

@whyoleg
Copy link
Contributor

whyoleg commented Apr 19, 2024

I've performed additional stress tests for this test (SequentialTasksExecutionStressTest) with increased amount of tasks (500 vs 100).

Here are some results:

Test with K2 analysis built with Kotlin 2.0.0-RC1: current PR image
Test with K2 analysis built with Kotlin 1.9.22: current master image
Test with K1 analysis built with Kotlin 2.0.0-RC1: current PR image
Test with K1 analysis built with Kotlin 1.9.22: current master image

Note: each test duration was ±30 minutes at my side.
The heap is increasing in each test in a similar way: this PR behaves similar to master for both K1 and K2.
So, I do think, that we can safely merge this PR.

Additionally this could mean two things:

  1. we do have some memory leak somewhere in Dokka (Engine or Gradle plugin)
  2. we don't have memory leaks and it's just normal behaviour for Gradle, as we do start 500 tasks.

For now, I don't have any proofs that we do have memory leak somewhere Dokka after some profiling (not just looking at memory graphs). Still, it's possible, as profiling Dokka execution in Gradle from multiple tasks is not that informative, as there is a lot of Gradle machinery involved under the hood...

@IgnatBeresnev IgnatBeresnev merged commit 4f5a650 into master Apr 22, 2024
13 checks passed
@IgnatBeresnev IgnatBeresnev deleted the bump-kotlin-2.0.0 branch April 22, 2024 08:35
@adam-enko
Copy link
Contributor

Still, it's possible, as profiling Dokka execution in Gradle from multiple tasks is not that informative, as there is a lot of Gradle machinery involved under the hood...

I agree.

Just writing down what's on my mind, I don't have any action planned.:

This performance test also makes the CI tests slower, it shouldn't need to be run every time (e.g. scheduling it to be once a day should be fine), and it makes the CI tests much more sensitive to limited resources (I suspect without the performance test we could probably just run all of the tests on one machine, without limiting parallelism or needing to batch the tests.

I think it'd make sense to create a separate subproject for profiling tests.

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.

Problems with updating Kotlin to 2.0.0
3 participants