Splitties / refreshVersions

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

Support Gradle configuration cache #228

Closed LouisCAD closed 2 years ago

LouisCAD commented 4 years ago

Relevant documentation: https://docs.gradle.org/nightly/userguide/configuration_cache.html

julioz commented 3 years ago

Hi, we're exploring configuration caching at SC; is this something you have planned on working soon, have an estimate you could provide, or a description of what prevents this work from happening?

LouisCAD commented 3 years ago

Hello, sorry for the late response.

I can't give any promises, but I hope we can address this issue in 2021.

pikzen commented 3 years ago

As a little update on this: the gradle configuration cache currently is very unhappy about this plugin using buildFinished (https://github.com/jmfayard/refreshVersions/blob/main/plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/internal/RefreshVersionsConfigHolder.kt#L132). I'll be trying to look into how this can be fixed.

LouisCAD commented 3 years ago

How is it unhappy?

pikzen commented 3 years ago

When running our project build (that makes use of refreshVersions) with --configuration-cache, this is the output:

2: Task failed with an exception.

1 problem was found storing the configuration cache.

See the complete report at file:///.../configuration-cache/as0e4uasippp0jkgc90s5xhk-1/configuration-cache-report.html

Listener registration 'Gradle.buildFinished' by build is unsupported.

Note that Gradle is as helpful as ever, and I've had to actually jump into debugging mode with it to learn that it came from this plugin in our project.

When running with configuration cache problems only shown as warnings, it technically caches it, but from experience with the build cache, it'll lead to some tasks not being re-run/run with the same configuration as previously. I am currently investigating how to remove those buildFinished calls, by registering a service with gradle that'll handle it, but since those buildFinished calls come from a place where there is no available Project, but just a ProjectDescriptor, this might get a little bit hairy.

EDIT: nvm, it can be registered through settings too. I'll keep investigating and come back with findings.

EDIT 2: That was surprisingly easy. Taking inspiration from what gradle-doctor did (https://github.com/runningcode/gradle-doctor/commit/120d9204c3962ba5e301589032364d9567575188), but by passing lambdas instead of requiring everything to be an autocloseable, allowing us to do any needed cleanup

    abstract class StaticStateClearOnBuildFinishedService : BuildService<BuildServiceParameters.None>, AutoCloseable {
        private val closeList = LinkedList<() -> Unit>()

        fun closeMeWhenFinished(lambda: () -> Unit) {
            closeList.push(lambda)
        }

        override fun close() {
            closeList.forEach {
                it()
            }

            closeList.clear()
        }

    }

then, in the places where clearStaticState was called, simply attempt to use that service when gradle allows it.

     if (GradleVersion.version(settings.gradle.gradleVersion) >= GradleVersion.version("6.5")) {
            // Closeable services were integrated in 6.5
            val closeService = settings.gradle.sharedServices
                .registerIfAbsent("close-resources-service", StaticStateClearOnBuildFinishedService::class.java) {}.get()

            closeService.closeMeWhenFinished { httpClient.dispatcher.executorService.shutdown() }
            closeService.closeMeWhenFinished { resettableDelegates.reset() }
        } else {
            settings.gradle.buildFinished {
                clearStaticState()
            }
        }

For tests, I had added a little bit of logging to ensure our lambda gets called, and it does. (Ignore the build failure, no phone plugged in): ./gradlew --configuration-cache application:installDevelopmentDebug

[about 200 modules worth of build info]
> Task :application:installDevelopmentDebug FAILED
Closing!
Closing!

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':application:installDevelopmentDebug'.
> com.android.builder.testing.api.DeviceException: No connected devices!

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 11m 37s
2803 actionable tasks: 1813 executed, 984 from cache, 6 up-to-date
Configuration cache entry stored.

Future runs do seem to reuse that cache:

./gradlew --configuration-cache installDevelopmentDebug
Configuration cache is an incubating feature.
Reusing configuration cache.
> Task :application:installDevelopmentDebug FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':application:installDevelopmentDebug'.
> com.android.builder.testing.api.DeviceException: No connected devices!

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 12s
2803 actionable tasks: 1 executed, 2802 up-to-date
Configuration cache entry reused.

Let me know if you'd like me to make this a pull request, or if you'd rather handle it/test yourself.

LouisCAD commented 3 years ago

Thanks for your research! That'll be helpful when I'll spend time on this issue.

My current focus is supporting automatic replacement of deprecated dependency notations, and it's a days long effort I'm trying to fit into my free time when I have the energy.

Once I'm done with it, this issue will likely be top priority, besides updating some dependency notations in AndroidX again for new WearOS artifacts.

yogurtearl commented 2 years ago

Also, you should make sure to test config cache with every task that the plugin has.

E.g. :refreshVersionsMigrate has these config cache issues:

Invocation of 'Task.project' by task ':refreshVersionsMigrate' at execution time is unsupported. Have to grap the needed values during configuration and pass them in as @Input

here: https://github.com/jmfayard/refreshVersions/blob/873210a27849ad252af8d561d042d8730d23190e/plugins/dependencies/src/main/kotlin/de/fayard/refreshVersions/RefreshVersionsMigrateTask.kt#L16

and here: https://github.com/jmfayard/refreshVersions/blob/873210a27849ad252af8d561d042d8730d23190e/plugins/dependencies/src/main/kotlin/de/fayard/refreshVersions/RefreshVersionsMigrateTask.kt#L24

Ideally this would be calculated at configuration time and passed an @Input to the task: https://github.com/jmfayard/refreshVersions/blob/873210a27849ad252af8d561d042d8730d23190e/plugins/core/src/main/kotlin/de/fayard/refreshVersions/core/MissingVersionEntries.kt#L51-L65

LouisCAD commented 2 years ago

Hello, FYI, I'm working on this issue right now, and I expect it to take a few days to complete.

If one of you want to join in a pair-programming session, let me know, I'll be happy to have one of you onboard!

LouisCAD commented 2 years ago

Thanks @pikzen for demonstrating it to work. I checked the doc, tried something similar, did some testing, and it works well so long you don't try to run the refreshVersions task with configuration cache enabled. That's already a big improvement as it no longer prevents you from using configuration cache in the build.

I'll see if I can quickly handle the issue affecting the refreshVersions task before the impending release.

LouisCAD commented 2 years ago

It's impossible (and pointless) to support Gradle configuration cache for the refreshVersions task (see why in the description here), so I'm dropping this part.

The main goal is to allow projects to take advantage of configuration cache safely, and this has been implemented successfully.