Splitties / refreshVersions

Life is too short to google for dependencies and versions
https://splitties.github.io/refreshVersions/
MIT License
1.66k stars 107 forks source link

`rejectVersionIf` of different builds influence each other due to static state #667

Open Vampire opened 1 year ago

Vampire commented 1 year ago

Just to test, I configured

refreshVersions {
    rejectVersionIf {
        println("FOO: $moduleId")
        true
    }
}

But it does not reject any version and also does not print any FOO. :-/

Vampire commented 1 year ago

Hm, it calls the wrong one actually. I have build A which includeBuild B. And B includeBuild C. The println("FOO: $moduleId") is in the settings script of C. I also added println("BAR: $moduleId") to the settings script of B and println("BAZ: $moduleId") to the settings script of A.

Now if I call ./gradlew :C:refreshVersions, I see BAR logs instead of FOO logs. o_O

Vampire commented 1 year ago

./gradlew :B:refreshVersions also shows BAR as expected and ./gradlew :refreshVersions shows BAZ as expected

Vampire commented 1 year ago

I've thrown together a reproducer as it is not exactly trivial: showcase.zip If you unzip it and then call ./gradlew :bar:refreshVersions, you will see showcase messages while you should see bar messages, as the bar task is run and also the bar version catalog is updated.

Actually in my production project it is showing foo messages here, but maybe there is additionally some race condition.

Vampire commented 1 year ago

I think the problem is, RefreshVersionsConfigHolder. It is an object. As far as I remember this manifests as static state in the JVM. This is quite bad, as it can even influence builds from completely other projects running on the same Gradle daemon.

There was a similar problem in the past with the Spotbugs Gradle Plugin. It used Spotbugs directly in-process. Spotbugs uses quite some static state. Then builds from completely different projects running on the same Gradle daemon influenced each other, as the static state was carried over.

In the context of Gradle plugins (at least, if not almost anywhere) any static state should be avoided as hell. If you need something like static state that has to be present during one Gradle build execution, the idiomatic pattern is to use a Shared Build Service that holds the data and has a clearly defined lifetime.

Vampire commented 1 year ago

Here a small hacky work-around. Make a small plugin consisting of two files. One file plain Foo.kt:

inline fun Settings.onRefreshVersionsRequested(configure: Settings.() -> Unit) {
    val rootBuild = gradle.parent == null
    val startTaskNames = gradle.rootBuild.startParameter.taskNames
    if (
        (rootBuild && startTaskNames.contains(":refreshVersions"))
        || (!rootBuild && startTaskNames.contains(":${settings.rootProject.name}:refreshVersions"))
    ) {
        configure()
    }
}

inline fun Settings.conditionalRefreshVersions(configure: RefreshVersionsExtension.() -> Unit) {
    onRefreshVersionsRequested {
        refreshVersions(configure)
    }
}

val Gradle.rootBuild: Gradle
    get() = if (gradle.parent == null) gradle else gradle.parent!!.rootBuild

The other a precompiled script plugin (or whatever) doing

require(gradle.rootBuild.startParameter.taskNames.count { it.endsWith("refreshVersions") } <= 1) {
    "Only one refreshVersions task can be executed per Gradle run"
}

onRefreshVersionsRequested {
    apply(plugin = "de.fayard.refreshVersions")
}

and in the settings scripts instead of de.fayard.refreshVersions you apply the plugin from above and instead of refreshVersions { ... } you use conditionalRefreshVersions { ... }.

This way only if you execute one of the refreshVersions tasks explicitly and with its full path, the plugin gets applied and the settings done so they cannot interfere with each other.