Fix cache kind management with nested subprojects (#3519)

* Fix cache kind management with nested subprojects Previously, cache kind property management worked incorrectly when Compose Gradle plugin was applied to both parent and child subprojects, e.g. :compose-subproject-1:compose-subproject-2. With this example the plugin would successfully set the property for compose-subproject-1, but then for compose-subproject-2 the following snippet would fail: ``` if (project.hasProperty(targetCacheKindPropertyName)) { project.setProperty(targetCacheKindPropertyName, NONE_VALUE) } ``` because project.hasProperty would have return true (because it checks parent subproject properties too), but project.setProperty would fail, because parent project's properties are read only. Warnings were also handled incorrectly in this case, because during the configuration of compose-subproject-1 we might set `kotlin.native.cacheKind.ios*=none`, which would then cause a warning during the configuration of compose-subproject-2. To avoid incorrect warnings, we now record the snapshot of relevant properties during Compose Multiplatform build service initialization Resolves #3515 * Fix issues from code review
JetBrains · Aug 17, 2023 · 350a5df · 350a5df
1 parent 5eda586
commit 350a5df
Show file tree

Hide file tree

Showing 11 changed files with 194 additions and 44 deletions.
diff --git a/...plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposeMultiplatformBuildService.kt b/...plugins/compose/src/main/kotlin/org/jetbrains/compose/ComposeMultiplatformBuildService.kt
@@ -2,24 +2,35 @@ package org.jetbrains.compose
 
 import org.gradle.api.Project
 import org.gradle.api.logging.Logging
+import org.gradle.api.provider.MapProperty
 import org.gradle.api.provider.Provider
 import org.gradle.api.provider.SetProperty
 import org.gradle.api.services.BuildService
 import org.gradle.api.services.BuildServiceParameters
 import org.gradle.tooling.events.FinishEvent
 import org.gradle.tooling.events.OperationCompletionListener
+import org.jetbrains.compose.experimental.internal.SUPPORTED_NATIVE_CACHE_KIND_PROPERTIES
 import org.jetbrains.compose.internal.utils.BuildEventsListenerRegistryProvider
+import org.jetbrains.compose.internal.utils.loadProperties
+import org.jetbrains.compose.internal.utils.localPropertiesFile
 import org.jetbrains.kotlin.gradle.plugin.SubpluginArtifact
 
 // The service implements OperationCompletionListener just so Gradle would use the service
 // even if the service is not used by any task or transformation
-abstract class ComposeMultiplatformBuildService : BuildService<BuildServiceParameters.None>,
+abstract class ComposeMultiplatformBuildService : BuildService<ComposeMultiplatformBuildService.Parameters>,
     OperationCompletionListener, AutoCloseable {
 
+    abstract class Parameters : BuildServiceParameters {
+        abstract val gradlePropertiesCacheKindSnapshot: MapProperty<String, String>
+        abstract val localPropertiesCacheKindSnapshot: MapProperty<String, String>
+    }
+
     private val log = Logging.getLogger(this.javaClass)
 
     internal abstract val unsupportedCompilerPlugins: SetProperty<Provider<SubpluginArtifact?>>
     internal abstract val delayedWarnings: SetProperty<String>
+    internal val gradlePropertiesSnapshot: Map<String, String> = parameters.gradlePropertiesCacheKindSnapshot.get()
+    internal val localPropertiesSnapshot: Map<String, String> = parameters.localPropertiesCacheKindSnapshot.get()
 
     fun warnOnceAfterBuild(message: String) {
         delayedWarnings.add(message)
@@ -74,6 +85,9 @@ abstract class ComposeMultiplatformBuildService : BuildService<BuildServiceParam
             return null
         }
 
+        fun getInstance(project: Project): ComposeMultiplatformBuildService =
+            findExistingComposeService(project) ?: error("ComposeMultiplatformBuildService was not initialized!")
+
         @Suppress("UnstableApiUsage")
         fun init(project: Project) {
             val existingService = findExistingComposeService(project)
@@ -82,13 +96,24 @@ abstract class ComposeMultiplatformBuildService : BuildService<BuildServiceParam
             }
 
             val newService = project.gradle.sharedServices.registerIfAbsent(COMPOSE_SERVICE_FQ_NAME, ComposeMultiplatformBuildService::class.java) {
+                it.parameters.initPropertiesSnapshots(project.rootProject)
             }
             // workaround to instanciate a  service even if it not binded to a task
             BuildEventsListenerRegistryProvider.getInstance(project).onTaskCompletion(newService)
         }
 
-        fun getInstance(project: Project): ComposeMultiplatformBuildService =
-            findExistingComposeService(project) ?: error("ComposeMultiplatformBuildService was not initialized!")
+        private fun Parameters.initPropertiesSnapshots(rootProject: Project) {
+            // we want to record original properties (explicitly set by a user)
+            // before we possibly change them in configureNativeCompilerCaching.kt
+            val localProperties = loadProperties(rootProject.localPropertiesFile)
+            for (cacheKindProperty in SUPPORTED_NATIVE_CACHE_KIND_PROPERTIES) {
+                rootProject.findProperty(cacheKindProperty)?.toString()?.let { value ->
+                    gradlePropertiesCacheKindSnapshot.put(cacheKindProperty, value)
+                }
+                localProperties[cacheKindProperty]?.toString()?.let { value ->
+                    localPropertiesCacheKindSnapshot.put(cacheKindProperty, value)
+                }
+            }
+        }
     }
 }
-
diff --git a/...main/kotlin/org/jetbrains/compose/experimental/internal/configureNativeCompilerCaching.kt b/...main/kotlin/org/jetbrains/compose/experimental/internal/configureNativeCompilerCaching.kt
@@ -12,24 +12,40 @@ import org.jetbrains.compose.internal.mppExt
 import org.jetbrains.compose.internal.utils.KGPPropertyProvider
 import org.jetbrains.compose.internal.utils.configureEachWithType
 import org.jetbrains.kotlin.gradle.plugin.mpp.KotlinNativeTarget
+import org.jetbrains.kotlin.konan.target.KonanTarget
 import org.jetbrains.kotlin.konan.target.presetName
 
 private const val PROJECT_CACHE_KIND_PROPERTY_NAME = "kotlin.native.cacheKind"
 private const val COMPOSE_NATIVE_MANAGE_CACHE_KIND = "compose.kotlin.native.manageCacheKind"
 private const val NONE_VALUE = "none"
 
+private val SUPPORTED_NATIVE_TARGETS = setOf(
+    KonanTarget.IOS_ARM32,
+    KonanTarget.IOS_X64,
+    KonanTarget.IOS_ARM64,
+    KonanTarget.IOS_SIMULATOR_ARM64,
+    KonanTarget.MACOS_X64,
+    KonanTarget.MACOS_ARM64,
+)
+
+internal val SUPPORTED_NATIVE_CACHE_KIND_PROPERTIES =
+    SUPPORTED_NATIVE_TARGETS.map { it.targetCacheKindPropertyName } +
+        PROJECT_CACHE_KIND_PROPERTY_NAME
+
 internal fun Project.configureNativeCompilerCaching() {
     if (findProperty(COMPOSE_NATIVE_MANAGE_CACHE_KIND) == "false") return
 
     plugins.withId(KOTLIN_MPP_PLUGIN_ID) {
         mppExt.targets.configureEachWithType<KotlinNativeTarget> {
-            checkCacheKindUserValueIsNotNone()
-            disableKotlinNativeCache()
+            if (konanTarget in SUPPORTED_NATIVE_TARGETS) {
+                checkExplicitCacheKind()
+                disableKotlinNativeCache()
+            }
         }
     }
 }
 
-private fun KotlinNativeTarget.checkCacheKindUserValueIsNotNone() {
+private fun KotlinNativeTarget.checkExplicitCacheKind() {
     // To determine cache kind KGP checks kotlin.native.cacheKind.<PRESET_NAME> first, then kotlin.native.cacheKind
     // For each property it tries to read Project.property, then checks local.properties
     // See https://github.com/JetBrains/kotlin/blob/d4d30dcfcf1afb083f09279c6f1ba05031efeabb/libraries/tools/kotlin-gradle-plugin/src/common/kotlin/org/jetbrains/kotlin/gradle/plugin/PropertiesProvider.kt#L416
@@ -43,35 +59,41 @@ private fun KotlinNativeTarget.checkCacheKindUserValueIsNotNone() {
         for (provider in propertyProviders) {
             val value = provider.valueOrNull(cacheKindProperty)
             if (value != null) {
-                if (value.equals(NONE_VALUE, ignoreCase = true)) {
-                    ComposeMultiplatformBuildService
-                        .getInstance(project)
-                        .warnOnceAfterBuild(cacheKindPropertyWarningMessage(cacheKindProperty, provider))
-                }
+                ComposeMultiplatformBuildService
+                    .getInstance(project)
+                    .warnOnceAfterBuild(
+                        explicitCacheKindWarningMessage(cacheKindProperty, value, provider)
+                    )
                 return
             }
         }
     }
 }
 
-private fun cacheKindPropertyWarningMessage(
+private fun explicitCacheKindWarningMessage(
     cacheKindProperty: String,
+    value: String,
     provider: KGPPropertyProvider
 ) = """
-    |Warning: '$cacheKindProperty' is explicitly set to `none`.
-    |Compose Multiplatform Gradle plugin can manage this property automatically 
-    |based on a Kotlin compiler version being used.
+    |Warning: '$cacheKindProperty' is explicitly set to '$value'.
+    |Compose Multiplatform Gradle plugin manages this property automatically based on a Kotlin compiler version being used.
     |In future versions of Compose Multiplatform this warning will become an error.
-    |  * Recommended action: remove explicit '$cacheKindProperty=$NONE_VALUE' from ${provider.location}. 
+    |  * Recommended action: remove explicit '$cacheKindProperty=$value' from ${provider.location}. 
     |  * Alternative action: disable cache kind management by adding '$COMPOSE_NATIVE_MANAGE_CACHE_KIND=false' to your 'gradle.properties'.
 """.trimMargin()
 
 
 private val KotlinNativeTarget.targetCacheKindPropertyName: String
-    get() = "$PROJECT_CACHE_KIND_PROPERTY_NAME.${konanTarget.presetName}"
+    get() = konanTarget.targetCacheKindPropertyName
+
+private val KonanTarget.targetCacheKindPropertyName: String
+    get() = "$PROJECT_CACHE_KIND_PROPERTY_NAME.${presetName}"
 
 private fun KotlinNativeTarget.disableKotlinNativeCache() {
-    if (project.hasProperty(targetCacheKindPropertyName)) {
+    val existingValue = project.findProperty(targetCacheKindPropertyName)?.toString()
+    if (NONE_VALUE.equals(existingValue, ignoreCase = true)) return
+
+    if (targetCacheKindPropertyName in project.properties) {
         project.setProperty(targetCacheKindPropertyName, NONE_VALUE)
     } else {
         project.extensions.extraProperties.set(targetCacheKindPropertyName, NONE_VALUE)

diff --git a/...ugins/compose/src/main/kotlin/org/jetbrains/compose/internal/utils/KGPPropertyProvider.kt b/...ugins/compose/src/main/kotlin/org/jetbrains/compose/internal/utils/KGPPropertyProvider.kt
@@ -6,6 +6,7 @@
 package org.jetbrains.compose.internal.utils
 
 import org.gradle.api.Project
+import org.jetbrains.compose.ComposeMultiplatformBuildService
 import java.util.*
 
 /**
@@ -23,13 +24,14 @@ internal abstract class KGPPropertyProvider {
     abstract val location: String
 
     class GradleProperties(private val project: Project) : KGPPropertyProvider() {
-        override fun valueOrNull(propertyName: String): String? = project.findProperty(propertyName)?.toString()
+        override fun valueOrNull(propertyName: String): String? =
+            ComposeMultiplatformBuildService.getInstance(project).gradlePropertiesSnapshot[propertyName]
         override val location: String = "gradle.properties"
     }
 
-    class LocalProperties(project: Project) : KGPPropertyProvider() {
-        private val localProperties: Properties by lazyLoadProperties(project.localPropertiesFile)
-        override fun valueOrNull(propertyName: String): String? = localProperties.getProperty(propertyName)
+    class LocalProperties(private val project: Project) : KGPPropertyProvider() {
+        override fun valueOrNull(propertyName: String): String? =
+            ComposeMultiplatformBuildService.getInstance(project).localPropertiesSnapshot[propertyName]
         override val location: String = "local.properties"
     }
 }
diff --git a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/internal/utils/fileUtils.kt b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/internal/utils/fileUtils.kt
@@ -59,11 +59,14 @@ private fun Array<out Provider<out FileSystemLocation>>.ioFiles(): Array<File> =
     let { providers -> Array(size) { i -> providers[i].ioFile } }
 
 internal fun lazyLoadProperties(propertiesFile: File): Lazy<Properties> = lazy {
+    loadProperties(propertiesFile)
+}
+
+internal fun loadProperties(propertiesFile: File): Properties =
     Properties().apply {
         if (propertiesFile.isFile) {
             propertiesFile.inputStream().use {
                 load(it)
             }
         }
-    }
-}
+    }
diff --git a/.../compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/GradlePluginTest.kt b/.../compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/GradlePluginTest.kt
@@ -116,7 +116,7 @@ class GradlePluginTest : GradlePluginTestBase() {
             defaultTestEnvironment.copy(kotlinVersion = kotlinVersion, useGradleConfigurationCache = false)
         )
 
-        val task = ":linkDebugFrameworkIosX64"
+        val task = ":subproject:linkDebugFrameworkIosX64"
         with(nativeCacheKindProject(kotlinVersion = TestKotlinVersions.v1_8_20)) {
             gradle(task, "--info").checks {
                 check.taskSuccessful(task)
@@ -136,26 +136,55 @@ class GradlePluginTest : GradlePluginTestBase() {
     @Test
     fun nativeCacheKindWarning() {
         Assumptions.assumeTrue(currentOS == OS.MacOS)
-        fun nativeCacheKindWarningProject(kotlinVersion: String) = testProject(
-            TestProjects.nativeCacheKindWarning,
-            defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
-        )
-
-        val cacheKindWarning = "'kotlin.native.cacheKind' is explicitly set to `none`"
-
-        val args = arrayOf("build", "--dry-run", "-Pkotlin.native.cacheKind=none")
-        with(nativeCacheKindWarningProject(kotlinVersion = TestKotlinVersions.v1_8_20)) {
-            gradle(*args).checks {
-                check.logContains(cacheKindWarning)
+        fun withNativeCacheKindWarningProject(kotlinVersion: String, fn: TestProject.() -> Unit) {
+            with(testProject(
+                TestProjects.nativeCacheKindWarning,
+                defaultTestEnvironment.copy(kotlinVersion = kotlinVersion)
+            )) {
+                fn()
+                testWorkDir.deleteRecursively()
+                testWorkDir.mkdirs()
             }
         }
-        testWorkDir.deleteRecursively()
-        testWorkDir.mkdirs()
-        with(nativeCacheKindWarningProject(kotlinVersion = TestKotlinVersions.v1_9_0) ) {
-            gradle(*args).checks {
-                check.logContains(cacheKindWarning)
+
+        fun testKotlinVersion(kotlinVersion: String) {
+            val args = arrayOf("build", "--dry-run")
+            val commonPartOfWarning = "Compose Multiplatform Gradle plugin manages this property automatically"
+            withNativeCacheKindWarningProject(kotlinVersion = kotlinVersion) {
+                gradle(*args).checks {
+                    check.logDoesntContain("Warning: 'kotlin.native.cacheKind")
+                    check.logDoesntContain(commonPartOfWarning)
+                }
+            }
+            withNativeCacheKindWarningProject(kotlinVersion = kotlinVersion) {
+                gradle(*args, "-Pkotlin.native.cacheKind=none").checks {
+                    check.logContainsOnce("Warning: 'kotlin.native.cacheKind' is explicitly set to 'none'")
+                    check.logContainsOnce(commonPartOfWarning)
+                }
+            }
+            withNativeCacheKindWarningProject(kotlinVersion = kotlinVersion) {
+                gradle(*args, "-Pkotlin.native.cacheKind=static").checks {
+                    check.logContainsOnce("Warning: 'kotlin.native.cacheKind' is explicitly set to 'static'")
+                    check.logContainsOnce(commonPartOfWarning)
+                }
+            }
+            withNativeCacheKindWarningProject(kotlinVersion = kotlinVersion) {
+                gradle(*args, "-Pkotlin.native.cacheKind.iosX64=none").checks {
+                    check.logContainsOnce("Warning: 'kotlin.native.cacheKind.iosX64' is explicitly set to 'none'")
+                    check.logContainsOnce(commonPartOfWarning)
+                }
+            }
+            withNativeCacheKindWarningProject(kotlinVersion = kotlinVersion) {
+                gradle(*args, "-Pkotlin.native.cacheKind.iosX64=static").checks {
+                    check.logContainsOnce("Warning: 'kotlin.native.cacheKind.iosX64' is explicitly set to 'static'")
+                    check.logContainsOnce(commonPartOfWarning)
+                }
             }
         }
+
+
+        testKotlinVersion(TestKotlinVersions.v1_8_20)
+        testKotlinVersion(TestKotlinVersions.v1_9_0)
     }
 
     @Test

diff --git a/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKind/settings.gradle b/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKind/settings.gradle
@@ -9,4 +9,5 @@ pluginManagement {
         maven { url "https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev" }
     }
 }
-rootProject.name = "nativeCacheKind"
+rootProject.name = "nativeCacheKind"
+include(":subproject")
diff --git a/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKind/subproject/build.gradle b/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKind/subproject/build.gradle
@@ -0,0 +1,27 @@
+plugins {
+    id "org.jetbrains.kotlin.multiplatform"
+    id "org.jetbrains.compose"
+}
+
+kotlin {
+    iosX64 {
+        binaries.framework {
+            isStatic = true
+            baseName = "shared"
+        }
+    }
+    iosArm64 {
+        binaries.framework {
+            isStatic = true
+            baseName = "shared"
+        }
+    }
+
+    sourceSets {
+        commonMain {
+            dependencies {
+                implementation(compose.runtime)
+            }
+        }
+    }
+}
diff --git a/...mpose/src/test/test-projects/misc/nativeCacheKind/subproject/src/commonMain/kotlin/App.kt b/...mpose/src/test/test-projects/misc/nativeCacheKind/subproject/src/commonMain/kotlin/App.kt
@@ -0,0 +1,10 @@
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
+
+@Composable
+fun App() {
+    var text by remember { mutableStateOf("Hello, World!") }
+}
diff --git a/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKindWarning/settings.gradle b/gradle-plugins/compose/src/test/test-projects/misc/nativeCacheKindWarning/settings.gradle
@@ -9,4 +9,5 @@ pluginManagement {
         maven { url "https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev" }
     }
 }
-rootProject.name = "nativeCacheKind"
+rootProject.name = "nativeCacheKind"
+include(":subproject")
diff --git a/...lugins/compose/src/test/test-projects/misc/nativeCacheKindWarning/subproject/build.gradle b/...lugins/compose/src/test/test-projects/misc/nativeCacheKindWarning/subproject/build.gradle
@@ -0,0 +1,20 @@
+plugins {
+    id "org.jetbrains.kotlin.multiplatform"
+    id "org.jetbrains.compose"
+}
+
+kotlin {
+    iosX64()
+    iosArm64()
+    iosSimulatorArm64()
+    macosX64()
+    macosArm64()
+
+    sourceSets {
+        commonMain {
+            dependencies {
+                implementation(compose.runtime)
+            }
+        }
+    }
+}
diff --git a/...rc/test/test-projects/misc/nativeCacheKindWarning/subproject/src/commonMain/kotlin/App.kt b/...rc/test/test-projects/misc/nativeCacheKindWarning/subproject/src/commonMain/kotlin/App.kt
@@ -0,0 +1,10 @@
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
+import androidx.compose.runtime.setValue
+
+@Composable
+fun App() {
+    var text by remember { mutableStateOf("Hello, World!") }
+}