Skip to content

Prototype for project isolation #256

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

Merged
merged 7 commits into from
Jul 19, 2024
Merged

Prototype for project isolation #256

merged 7 commits into from
Jul 19, 2024

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

@shanshin shanshin merged commit f1e2076 into master Jul 19, 2024
3 checks passed
@shanshin shanshin deleted the prototype/isolation branch July 19, 2024 12:02
shanshin added a commit to JetBrains/kotlin that referenced this pull request Oct 28, 2024
shanshin added a commit to JetBrains/kotlin that referenced this pull request Dec 3, 2024
shanshin added a commit to JetBrains/kotlin that referenced this pull request Dec 13, 2024
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