Skip to content

Conversation

shanshin
Copy link
Contributor

No description provided.

@shanshin shanshin requested a review from fzhinkin July 15, 2024 13:47
import org.gradle.api.plugins.*
import org.gradle.api.provider.*
import org.gradle.api.tasks.*
import org.jetbrains.kotlin.gradle.dsl.*
import org.jetbrains.kotlin.gradle.plugin.*
import org.jetbrains.kotlin.gradle.plugin.mpp.KotlinNativeTarget
import org.jetbrains.kotlin.gradle.plugin.mpp.pm20.util.libsDirectory
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like an unintended change

@@ -276,7 +275,8 @@ internal class KlibVerificationTests : BaseKotlinGradleTest() {

@Test
fun `apiCheck should work for Apple-targets`() {
Assume.assumeTrue(HostManager().isEnabled(KonanTarget.MACOS_ARM64))
// Assume.assumeTrue(HostManager().isEnabled(KonanTarget.MACOS_ARM64))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still need to do something with that.

if (KotlinVersion.CURRENT.major > 2) {
dependencySet.add(project.dependencies.create("org.jetbrains.kotlin:kotlin-metadata-jvm:${KotlinVersion.CURRENT}"))
} else {
// use older 0.6.2 kotlinx metadata version for Kotlin < 2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we need it: kotlin-metadata-jvm should work fine with older Kotlin versions

it.isCanBeDeclared = true
it.isVisible = false
it.defaultDependencies {
it.add(project.dependencies.create("org.jetbrains.kotlinx:kotlinx-metadata-jvm:0.6.2"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be kotlin-metadata-jvm after merging with master


// TODO:
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to enable logging back

dependencySet.add(project.dependencies.create("org.ow2.asm:asm:9.6"))
dependencySet.add(project.dependencies.create("org.ow2.asm:asm-tree:9.6"))

if (KotlinVersion.CURRENT.major >= 2) {
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in person, I would suggest instead of KotlinVersion.CURRENT using something like

val metadataDependencyVersion = project.objects.property(String::class.java).convention("2.0.0")
    project.plugins.withType(KotlinBasePlugin::class.java) {
        metadataDependencyVersion.set(project.getKotlinPluginVersion())
    }

to be more safe against future versions of Kotlin.
KotlinVersion.CURRENT returns version of the stdlib in current runtime which depends on version of Gradle rather than KGP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we can't use getKotlinPluginVersion because BCV plugin and KGP may be placed in different classloaders.
I implemented workaround.

it.isVisible = false

it.defaultDependencies { dependencySet ->
dependencySet.add(project.dependencies.create("org.jetbrains.kotlin:kotlin-compiler-embeddable:${KotlinVersion.CURRENT}"))
Copy link
Member

Choose a reason for hiding this comment

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

Here too, and here it is even more critical as it configures an incorrect compiler version. For example, on Gradle 7.6, it will configure 1.7.10

@ALikhachev
Copy link
Member

In general LGTM, thanks!

@shanshin shanshin requested a review from ALikhachev July 18, 2024 16:16

project.dependencies.add(dependencyConfiguration.name, "org.ow2.asm:asm:9.6")
project.dependencies.add(dependencyConfiguration.name, "org.ow2.asm:asm-tree:9.6")
project.dependencies.addProvider(dependencyConfiguration.name, metadataDependencyVersion.map { version -> "org.jetbrains.kotlin:kotlin-metadata-jvm:$version" })
Copy link
Member

@ALikhachev ALikhachev Jul 18, 2024

Choose a reason for hiding this comment

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

This method is available since Gradle 7.0, but the README states

Binary compatibility validator plugin requires Gradle 6.0 or newer.

But I guess that's fine for now. We in KGP going to drop support for Gradle 6 soon . anyway. So, I propose to update README to reflect the actual compatibility

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