Incompatible with Gradle Configuration Cache

[dsvensson]

Originally posted as comment to #153, but deserves its own issue.

gradle 6.6 released, and with the new --configuration-cache feature this plugin causes:

- plugin 'com.adarshr.test-logger': read system property 'sun.jnu.encoding'
  See https://docs.gradle.org/6.6/userguide/configuration_cache.html#config_cache:requirements:undeclared_sys_prop_read

Looks like a trivial fix. Have to lookup the overrides specifically via ProviderFactory.systemProperty rather than searching for them like today in getOverrides.

Trying to fix (wild guesses here, never done any plugin development - nor groovy) that part results in the following error on the second execution after establishing the cache:

./gradlew test --configuration-cache
Configuration cache is an incubating feature.
Reusing configuration cache.

FAILURE: Build failed with an exception.

* What went wrong:
Could not load the value of field `testListenerBroadcaster` of task `:test` of type `org.gradle.api.tasks.testing.Test`.
> com.adarshr.gradle.testlogger.logger.TestLoggerWrapper

* 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 451ms
Configuration cache entry reused.

Crude diff to get rid of the undeclared_sys_prop_read warning:

diff --git a/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerExtension.groovy b/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerExtension.groovy
index e46768e..2a71e06 100644
--- a/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerExtension.groovy
+++ b/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerExtension.groovy
@@ -3,6 +3,7 @@ package com.adarshr.gradle.testlogger
 import com.adarshr.gradle.testlogger.theme.ThemeType
 import groovy.transform.CompileStatic
 import org.gradle.api.logging.LogLevel
+import org.gradle.api.provider.ProviderFactory
 import org.gradle.api.tasks.testing.logging.TestLogging
 
 import static com.adarshr.gradle.testlogger.theme.ThemeType.STANDARD
@@ -185,31 +186,35 @@ class TestLoggerExtension {
         this
     }
 
-    TestLoggerExtension applyOverrides(Map<String, String> overrides) {
-        override(overrides, 'theme', ThemeType)
-        override(overrides, 'showExceptions', Boolean)
-        override(overrides, 'showCauses', Boolean)
-        override(overrides, 'showStackTraces', Boolean)
-        override(overrides, 'showFullStackTraces', Boolean)
-        override(overrides, 'slowThreshold', Long)
-        override(overrides, 'showSummary', Boolean)
-        override(overrides, 'showStandardStreams', Boolean)
-        override(overrides, 'showPassedStandardStreams', Boolean)
-        override(overrides, 'showSkippedStandardStreams', Boolean)
-        override(overrides, 'showFailedStandardStreams', Boolean)
-        override(overrides, 'showPassed', Boolean)
-        override(overrides, 'showSkipped', Boolean)
-        override(overrides, 'showFailed', Boolean)
-        override(overrides, 'showSimpleNames', Boolean)
+    TestLoggerExtension applyOverrides(ProviderFactory providers) {
+        override(providers, 'theme', ThemeType)
+        override(providers, 'showExceptions', Boolean)
+        override(providers, 'showCauses', Boolean)
+        override(providers, 'showStackTraces', Boolean)
+        override(providers, 'showFullStackTraces', Boolean)
+        override(providers, 'slowThreshold', Long)
+        override(providers, 'showSummary', Boolean)
+        override(providers, 'showStandardStreams', Boolean)
+        override(providers, 'showPassedStandardStreams', Boolean)
+        override(providers, 'showSkippedStandardStreams', Boolean)
+        override(providers, 'showFailedStandardStreams', Boolean)
+        override(providers, 'showPassed', Boolean)
+        override(providers, 'showSkipped', Boolean)
+        override(providers, 'showFailed', Boolean)
+        override(providers, 'showSimpleNames', Boolean)
 
         this
     }
 
-    private void override(Map<String, String> overrides, String name, Class type) {
-        if (overrides.containsKey(name)) {
+    private void override(ProviderFactory providers, String name, Class type) {
+        def property = providers
+            .systemProperty("${TestLoggerPlugin.EXTENSION_NAME}.${name}" as String)
+            .forUseAtConfigurationTime()
+
+        if (property.isPresent()) {
             String method = Enum.isAssignableFrom(type) ? 'fromName' : 'valueOf'
 
-            setProperty(name, type.invokeMethod(method, overrides[name]))
+            setProperty(name, type.invokeMethod(method, property.get()))
         }
     }
 }
diff --git a/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerPlugin.groovy b/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerPlugin.groovy
index 9faba2e..674dee5 100644
--- a/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerPlugin.groovy
+++ b/src/main/groovy/com/adarshr/gradle/testlogger/TestLoggerPlugin.groovy
@@ -9,7 +9,7 @@ import org.gradle.api.tasks.testing.Test
 @CompileStatic
 class TestLoggerPlugin implements Plugin<Project> {
 
-    private static final String EXTENSION_NAME = 'testlogger'
+    public static final String EXTENSION_NAME = 'testlogger'
 
     @Override
     void apply(Project project) {
@@ -44,14 +44,6 @@ class TestLoggerPlugin implements Plugin<Project> {
             .undecorate()
             .reactTo(test.testLogging)
             .combine(projectExtension)
-            .applyOverrides(overrides)
-    }
-
-    private static Map<String, String> getOverrides() {
-        (System.properties as Map<String, String>).findAll { key, value ->
-            key.startsWith "${EXTENSION_NAME}."
-        }.collectEntries { key, value ->
-            [(key.replace("${EXTENSION_NAME}.", '')): value]
-        } as Map<String, String>
+            .applyOverrides(test.project.providers)
     }
 }

...and commenting out jacoco, build scans, and the overrides test, and bumping to 6.6

[radarsh]

Thank you for the groundwork done here. I had a brief look at the providers API and found that org.gradle.api.provider.ProviderFactory#systemProperty(java.lang.String) has only been available since Gradle 6.1 and org.gradle.api.provider.Provider#forUseAtConfigurationTime since Gradle 6.5. Unfortunately making use of such cutting edge features that are also incubating introduces instability in the plugin, not to mention the fact that I'll have to create a 3.x release as this will be a breaking change for all but the most recent users of Gradle.

Therefore, I would like to postpone this feature for some time until Gradle 6.5+ is more commonly used.

Let me know if you have a hard reason to use configuration caching and based on the number of votes this comment receives, I'll create and maintain a parallel 3.x branch.

[dsvensson]

The main reason is getting rid of recurring waiting times in the development experience that can be avoided. While I understand why it would be to be annoying to maintain two, if this plugin is indeed used by old gradle versions. Other high profile plugins such as shadowjar, kotlin, and jooq have already taken the leap.

[radarsh]

@dsvensson I hear you. I am mostly going to implement this as there is another bug, #156 which needs me to start using the Providers API and lazy configuration in order for the plugin to be compatible with Gradle 7.

[dsvensson]

Thanks for the great work you put into this project! Out of curiosity, do you as a gradle plugin developer get metrics on what gradle versions are downloading the plugin from the gradle portal?

[radarsh]

Unfortunately not. The Gradle Plugin Portal is one of the most primitive plugin repositories out there. NPM really puts everyone to shame.

Anyway guess what issue number 2 is on Gradle Plugin Portal? 🙂

[dsvensson]

This breaks again in 6.8 it seems:

* What went wrong:
Could not load the value of field `startParameter` of `com.adarshr.gradle.testlogger.logger.TestLoggerWrapper` bean found in field `testListenerBroadcaster` of task `:api:api-core:test` of type `org.gradle.api.tasks.testing.Test`.
> No service of type StartParameterInternal available in ProjectScopeServices.

[poldz123]

This break in 6.8.2

[asarkar]

Speaking of configuration cache, it really seems that it was introduced without much of impact analysis and migration guidance for plugin authors. I’m the author of build-time-tracker plugin, and had to work around several nagging issues introduced by configuration cache design, or lack of it. I don’t know if this plugin uses build listeners, probably not, but if you do, you’re in for a lot of trouble.
I’ve a ticket open with Gradle, not that it’s helping. gradle/gradle#18520