Work around Gradle modifying system properties at configuration time #30210
417
−73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
When compiling Kotlin scripts, Gradle sets up some system properties. However, because Gradle doesn't instrument the Kotlin runtime, these modifications go unnoticed to environment change tracker infrastructure. If any user code then observes these system properties after modification, it becomes part of the configuration cache fingerprint. Moreover, as nothing restores the changed values when checking the fingerprint, it never matches, causing a CC miss. We can figure out the fixed set of properties set by the compiler and ignore them, but this set comes from the Kotlin (==third party) code so we'll have to maintain it and update alongside Kotlin releases. Instrumenting the Kotlin runtime to track system property updates in a usual way is an option, but it is much more involved and can bring in many unwanted inputs because of the Gradle runtime calling the Kotlin's stdlib functions. Restoring the system properties after compilation completes may also work but introduces a potential for race conditions when other, instrumented code may modify the system properties concurrently. Rolling back that changes would be an error.
The KotlinCompilerEnvironment instance serves as a token to mark all functions that may change the environment to configure the compiler. It also separates the tracking setup from the code in KotlinCompiler.
mlopatkin
force-pushed
the
ml/30145/workaround-nio-sysprop-change
branch
from
4b700ea
to
1b72e9f
Compare
|
@bot-gradle test ACC |
|
I've triggered the following builds for you. Click here to see all build failures. |
|
@bot-gradle test this |
|
I've triggered the following builds for you. Click here to see all build failures. |
mlopatkin
commented
Aug 25, 2024
|
|
||
| buildFile("plugin/build.gradle", """ | ||
| plugins { | ||
| id "org.gradle.kotlin.kotlin-dsl" version "5.1.0" // TODO(mlopatkin): this should be the "current" Kotlin DSL version but it isn't available for Groovy scripts. |
There was a problem hiding this comment.
💭 Could not find another way to apply kotlin-dsl plugin with Groovy DSL.
mlopatkin
commented
Aug 25, 2024
| @@ -177,10 196,31 @@ class ConfigurationCacheEnvironmentChangeTracker(private val problemFactory: Pro | |||
| systemPropertiesCleared = true | |||
| mutatedSystemProperties.clear() | |||
| } | |||
|
|
|||
| override fun <T : Any> withTrackingSystemPropertyChanges(action: Supplier<out T>): T { | |||
There was a problem hiding this comment.
☝️ This is the core non-trivial change.
mlopatkin
requested review from
abstratt and
6hundreds
and removed request for
a team
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When compiling Kotlin scripts, Gradle sets up some system properties. However, because Gradle doesn't instrument the Kotlin runtime, these modifications go unnoticed to environment change tracker infrastructure. If any user code then observes these system properties after modification, it becomes part of the configuration cache fingerprint. Moreover, as nothing restores the changed values when checking the fingerprint, it never matches, causing a CC miss.
With this PR, a special "scope function" of EnvironmentChangeTracker can be used to detect changes made to system properties and update the state of the tracker accordingly. This isn't a perfect solution for concurrent use cases, but it should work well when there is only one "scope" that modifies system properties directly, and everything else is instrumented.
It is possible to enter the scope function after restoring the CC state, for example, for a precompiled script plugin project. There is no change tracking in this case.
Fixes #30145.
Reviewing cheatsheet
Before merging the PR, comments starting with