From 9e78e43c4903699847b6fbe12b8753c86267460f Mon Sep 17 00:00:00 2001 From: Alexander Likhachev Date: Sat, 10 Apr 2021 18:20:31 +0300 Subject: [PATCH] [Gradle] Always declare system property reads for compile tasks When Kotlin plugin is applied in buildSrc or included build that defines Gradle plugins it can cause Gradle to report undeclared system property reads at configuration time. Declaring all system properties reads through Gradle providers to cover all these cases. #KT-45910 Fixed --- .../jetbrains/kotlin/cli/common/Properties.kt | 33 ++++++++------ .../kotlin/gradle/ConfigurationCacheIT.kt | 11 +++++ .../build.gradle | 20 +++++++++ .../includedBuild/build.gradle | 27 ++++++++++++ .../includedBuild/src/main/kotlin/plugin.kt | 7 +++ .../settings.gradle | 1 + .../src/main/kotlin/Hello.kt | 1 + .../CompilerSystemPropertiesService.kt | 43 +++++++++---------- 8 files changed, 108 insertions(+), 35 deletions(-) create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/build.gradle create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/build.gradle create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/src/main/kotlin/plugin.kt create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/settings.gradle create mode 100644 libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/src/main/kotlin/Hello.kt diff --git a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt b/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt index 457c48aecee..71bb2eeca62 100644 --- a/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt +++ b/compiler/cli/cli-common/src/org/jetbrains/kotlin/cli/common/Properties.kt @@ -16,10 +16,10 @@ package org.jetbrains.kotlin.cli.common -import com.intellij.util.LineSeparator -import java.util.* - -enum class CompilerSystemProperties(val property: String) { +/** + * @param alwaysDirectAccess Gradle has a list of properties that can be read without declaring, see https://github.com/gradle/gradle/blob/f191a61cec61afe308f2b45184cb303d32706a6f/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/SystemPropertyAccessListener.kt#L32 + */ +enum class CompilerSystemProperties(val property: String, val alwaysDirectAccess: Boolean = false) { COMPILE_DAEMON_ENABLED_PROPERTY("kotlin.daemon.enabled"), COMPILE_DAEMON_JVM_OPTIONS_PROPERTY("kotlin.daemon.jvm.options"), COMPILE_DAEMON_OPTIONS_PROPERTY("kotlin.daemon.options"), @@ -36,24 +36,31 @@ enum class CompilerSystemProperties(val property: String) { KOTLIN_COMPILER_ENVIRONMENT_KEEPALIVE_PROPERTY("kotlin.environment.keepalive"), COMPILE_DAEMON_CUSTOM_RUN_FILES_PATH_FOR_TESTS("kotlin.daemon.custom.run.files.path.for.tests"), KOTLIN_COLORS_ENABLED_PROPERTY("kotlin.colors.enabled"), - OS_NAME("os.name"), + OS_NAME("os.name", alwaysDirectAccess = true), TMP_DIR("java.io.tmpdir"), - USER_HOME("user.home"), - JAVA_VERSION("java.specification.version"), - JAVA_HOME("java.home"), - JAVA_CLASS_PATH("java.class.path"), + USER_HOME("user.home", alwaysDirectAccess = true), + JAVA_VERSION("java.specification.version", alwaysDirectAccess = true), + JAVA_HOME("java.home", alwaysDirectAccess = true), + JAVA_CLASS_PATH("java.class.path", alwaysDirectAccess = true), ; - var value - get() = (systemPropertyGetter ?: System::getProperty)(property) + private fun getProperFunction(custom: T?, default: T): T { + if (alwaysDirectAccess) return default + return custom ?: default + } + + var value: String? + get() { + return getProperFunction(systemPropertyGetter, System::getProperty)(property) + } set(value) { - (systemPropertySetter ?: System::setProperty)(property, value!!) + getProperFunction(systemPropertySetter, System::setProperty)(property, value!!) } val safeValue get() = value ?: error("No value for $property system property") - fun clear(): String? = (systemPropertyCleaner ?: System::clearProperty)(property) + fun clear(): String? = getProperFunction(systemPropertyCleaner, System::clearProperty)(property) companion object { var systemPropertyGetter: ((String) -> String?)? = null diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/ConfigurationCacheIT.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/ConfigurationCacheIT.kt index 05718a45a98..9b5de08d85c 100644 --- a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/ConfigurationCacheIT.kt +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/ConfigurationCacheIT.kt @@ -77,6 +77,17 @@ class ConfigurationCacheIT : AbstractConfigurationCacheIT() { ) } + @Test + fun testInstantExecutionWithIncludedBuildPlugin() = + with(Project("instantExecutionWithIncludedBuildPlugin", gradleVersionRequirement = GradleVersionRequired.AtLeast("6.8"))) { + setupWorkingDir() + testConfigurationCacheOf( + "build", executedTaskNames = listOf( + ":compileKotlin", + ) + ) + } + @Test fun testInstantExecutionForJs() = with(Project("instantExecutionToJs")) { testConfigurationCacheOf("assemble", executedTaskNames = asList(":compileKotlin2Js")) diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/build.gradle b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/build.gradle new file mode 100644 index 00000000000..819b574b565 --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/build.gradle @@ -0,0 +1,20 @@ +buildscript { + repositories { + mavenLocal() + mavenCentral() + } + dependencies { + classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" + } +} + +plugins { + id("foo-plugin") +} + +apply plugin: 'kotlin' + +repositories { + mavenLocal() + mavenCentral() +} \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/build.gradle b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/build.gradle new file mode 100644 index 00000000000..ea32dfc53e7 --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/build.gradle @@ -0,0 +1,27 @@ +buildscript { + repositories { + mavenLocal() + mavenCentral() + } + dependencies { + classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" + } +} + +plugins { + id 'java-gradle-plugin' +} + +apply plugin: 'kotlin' + +repositories { + mavenLocal() + mavenCentral() +} + +gradlePlugin { + plugins.register("foo-plugin") { + id = "foo-plugin" + implementationClass = "FooPlugin" + } +} \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/src/main/kotlin/plugin.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/src/main/kotlin/plugin.kt new file mode 100644 index 00000000000..8661bb68d9b --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/includedBuild/src/main/kotlin/plugin.kt @@ -0,0 +1,7 @@ +import org.gradle.api.Plugin +import org.gradle.api.Project + +class FooPlugin : Plugin { + override fun apply(project: Project) { + } +} \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/settings.gradle b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/settings.gradle new file mode 100644 index 00000000000..b5bc25191ea --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/settings.gradle @@ -0,0 +1 @@ +includeBuild("includedBuild") \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/src/main/kotlin/Hello.kt b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/src/main/kotlin/Hello.kt new file mode 100644 index 00000000000..65490fbf75f --- /dev/null +++ b/libraries/tools/kotlin-gradle-plugin-integration-tests/src/test/resources/testProject/instantExecutionWithIncludedBuildPlugin/src/main/kotlin/Hello.kt @@ -0,0 +1 @@ +fun hello() = "Hello" \ No newline at end of file diff --git a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/compilerRunner/CompilerSystemPropertiesService.kt b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/compilerRunner/CompilerSystemPropertiesService.kt index fec32ca1687..1479c00813e 100644 --- a/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/compilerRunner/CompilerSystemPropertiesService.kt +++ b/libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/compilerRunner/CompilerSystemPropertiesService.kt @@ -5,7 +5,6 @@ package org.jetbrains.kotlin.compilerRunner -import org.gradle.api.Project import org.gradle.api.invocation.Gradle import org.gradle.api.provider.MapProperty import org.gradle.api.provider.Provider @@ -22,22 +21,25 @@ internal abstract class CompilerSystemPropertiesService : BuildService + if (key !in properties) { + return@setter System.setProperty(key, value) + } val oldValue = properties[key] - if (oldValue == value) return@setter oldValue properties[key] = value - System.setProperty(key, value) oldValue } - CompilerSystemProperties.systemPropertyCleaner = { + CompilerSystemProperties.systemPropertyCleaner = cleaner@{ + if (it !in properties) { + return@cleaner System.clearProperty(it) + } val oldValue = properties[it] properties.remove(it) - System.clearProperty(it) oldValue } } @@ -49,22 +51,19 @@ internal abstract class CompilerSystemPropertiesService : BuildService = - gradle.sharedServices.registerIfAbsent( - "${CompilerSystemPropertiesService::class.java.canonicalName}_${CompilerSystemPropertiesService::class.java.classLoader.hashCode()}", - CompilerSystemPropertiesService::class.java - ) { service -> - val rootProject = gradle.rootProject - if (rootProject.isBuildSrc && isConfigurationCacheAvailable(gradle)) { - service.parameters.properties.set( - CompilerSystemProperties.values() - .associate { - it.property to rootProject.providers.systemProperty(it.property).forUseAtConfigurationTime() - }.toMap() - ) - } + fun registerIfAbsent(gradle: Gradle): Provider = gradle.sharedServices.registerIfAbsent( + "${CompilerSystemPropertiesService::class.java.canonicalName}_${CompilerSystemPropertiesService::class.java.classLoader.hashCode()}", + CompilerSystemPropertiesService::class.java + ) { service -> + if (isConfigurationCacheAvailable(gradle)) { + service.parameters.properties.set( + CompilerSystemProperties.values() + .filterNot { it.alwaysDirectAccess } + .associate { + it.property to gradle.rootProject.providers.systemProperty(it.property).forUseAtConfigurationTime() + }.toMap() + ) } + } } }