diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/ConfigurationCacheValueSourceIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/ConfigurationCacheValueSourceIntegrationTest.groovy index c3062884ab86..b1277ade193e 100644 --- a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/ConfigurationCacheValueSourceIntegrationTest.groovy +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/ConfigurationCacheValueSourceIntegrationTest.groovy @@ -158,7 +158,9 @@ class ConfigurationCacheValueSourceIntegrationTest extends AbstractConfiguration then: output.count("ON CI") == 1 - output.contains("because a build logic input of type 'IsSystemPropertySet' has changed") + // TODO(mlopatkin) the system property becomes an input itself because it is accessed in the ValueSource implementation. + // This assert should be changed back if ValueSource implementations are allowed to access environment freely. + output.contains("because system property 'ci' has changed") configurationCache.assertStateStored() } } diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInDynamicGroovyIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInDynamicGroovyIntegrationTest.groovy new file mode 100644 index 000000000000..e4434a0c76cd --- /dev/null +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInDynamicGroovyIntegrationTest.groovy @@ -0,0 +1,114 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.inputs.undeclared + +import org.gradle.api.Plugin +import org.gradle.api.Project +import org.gradle.configurationcache.AbstractConfigurationCacheIntegrationTest + +class SystemPropertyInstrumentationInDynamicGroovyIntegrationTest extends AbstractConfigurationCacheIntegrationTest { + def "#method is instrumented in dynamic Groovy"() { + def configurationCache = newConfigurationCacheFixture() + + given: + // Why the separate plugin? The Project.getProperties() is available in the build.gradle as getProperties(). + // Therefore, it is impossible to call System.getProperties() with static import there, and testing static + // import is important because Groovy generates different code in this case. + file("buildSrc/src/main/groovy/SomePlugin.groovy") << """ + import ${Plugin.name} + import ${Project.name} + import static ${System.name}.clearProperty + import static ${System.name}.getProperties + import static ${System.name}.getProperty + import static ${System.name}.setProperty + + class SomePlugin implements Plugin { + void apply(Project project) { + def returned = $method + println("returned = \$returned") + } + } + """ + + buildScript(""" + apply plugin: SomePlugin + """) + + when: + configurationCacheRun("-Dsome.property=some.value") + + then: + configurationCache.assertStateStored() + outputContains("returned = some.value") + problems.assertResultHasProblems(result) { + withInput("Plugin class 'SomePlugin': system property 'some.property'") + } + + where: + method | _ + "System.properties['some.property']" | _ + "System.getProperties().get('some.property')" | _ + "getProperties().get('some.property')" | _ + "System.getProperty('some.property')" | _ + "getProperty('some.property')" | _ + "System.getProperty('some.property', 'default.value')" | _ + "getProperty('some.property', 'default.value')" | _ + "System.setProperty('some.property', 'new.value')" | _ + "setProperty('some.property', 'new.value')" | _ + "System.clearProperty('some.property')" | _ + "clearProperty('some.property')" | _ + } + + def "#setProperties is instrumented in dynamic Groovy"() { + def configurationCache = newConfigurationCacheFixture() + + given: + buildScript(""" + import static ${System.name}.setProperties + + def newProps = new Properties() + System.properties.forEach { k, v -> newProps[k] = v } + newProps.replace("some.property", "new.value") + ${setProperties}(newProps) + + tasks.register("printProperty") { + doLast { + println("returned = \${System.getProperty("some.property")}") + } + } + """) + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateStored() + outputContains("returned = new.value") + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("returned = new.value") + + where: + setProperties | _ + "System.setProperties" | _ + "setProperties" | _ + } +} diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInJavaIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInJavaIntegrationTest.groovy new file mode 100644 index 000000000000..08e165276561 --- /dev/null +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInJavaIntegrationTest.groovy @@ -0,0 +1,107 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.inputs.undeclared + + +import org.gradle.api.Plugin +import org.gradle.api.Project +import org.gradle.configurationcache.AbstractConfigurationCacheIntegrationTest + +class SystemPropertyInstrumentationInJavaIntegrationTest extends AbstractConfigurationCacheIntegrationTest { + def "#method is instrumented in Java"() { + def configurationCache = newConfigurationCacheFixture() + + given: + file("buildSrc/src/main/java/SomePlugin.java") << """ + import ${Plugin.name}; + import ${Project.name}; + + public class SomePlugin implements Plugin { + public void apply(Project project) { + Object returned = $method; + System.out.println("returned = " + returned); + } + } + """ + + buildScript(""" + apply plugin: SomePlugin + """) + + when: + configurationCacheRun("-Dsome.property=some.value") + + then: + configurationCache.assertStateStored() + outputContains("returned = some.value") + problems.assertResultHasProblems(result) { + withInput("Plugin class 'SomePlugin': system property 'some.property'") + } + + where: + method | _ + "System.getProperties().get(\"some.property\")" | _ + "System.getProperty(\"some.property\")" | _ + "System.getProperty(\"some.property\", \"default.value\")" | _ + "System.setProperty(\"some.property\", \"new.value\")" | _ + "System.clearProperty(\"some.property\")" | _ + } + + def "setProperties is instrumented in Java"() { + def configurationCache = newConfigurationCacheFixture() + + given: + file("buildSrc/src/main/java/SomePlugin.java") << """ + import ${Plugin.name}; + import ${Project.name}; + import ${Properties.name}; + + public class SomePlugin implements Plugin { + public void apply(Project project) { + Properties newProps = new Properties(); + System.getProperties().forEach(newProps::put); + newProps.replace("some.property", "new.value"); + System.setProperties(newProps); + } + } + """ + + buildScript(""" + apply plugin: SomePlugin + + tasks.register("printProperty") { + doLast { + println("returned = \${System.getProperty("some.property")}") + } + } + """) + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateStored() + outputContains("returned = new.value") + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("returned = new.value") + } +} diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInKotlinIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInKotlinIntegrationTest.groovy new file mode 100644 index 000000000000..b7b0b3c0e955 --- /dev/null +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInKotlinIntegrationTest.groovy @@ -0,0 +1,74 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.inputs.undeclared + + +import org.gradle.api.Plugin +import org.gradle.api.Project +import org.gradle.configurationcache.AbstractConfigurationCacheIntegrationTest + +class SystemPropertyInstrumentationInKotlinIntegrationTest extends AbstractConfigurationCacheIntegrationTest { + def "#method is instrumented in Kotlin"() { + def configurationCache = newConfigurationCacheFixture() + + given: + // Why the separate plugin? The Project.getProperties() is available in the build.gradle.kts as getProperties(). + file("buildSrc/src/main/kotlin/SomePlugin.kt") << """ + import ${Plugin.name} + import ${Project.name} + + class SomePlugin : Plugin { + override fun apply(project: Project) { + val returned = $method + println("returned = \$returned") + } + } + """ + + file("buildSrc/build.gradle.kts") << """ + plugins { + `kotlin-dsl` + } + repositories { + mavenCentral() + } + """ + + buildScript(""" + apply plugin: SomePlugin + """) + + when: + configurationCacheRun("-Dsome.property=some.value") + + then: + configurationCache.assertStateStored() + outputContains("returned = some.value") + problems.assertResultHasProblems(result) { + withInput("Plugin class 'SomePlugin': system property 'some.property'") + ignoringUnexpectedInputs() + } + + where: + method | _ + "System.getProperties().get(\"some.property\")" | _ + "System.getProperty(\"some.property\")" | _ + "System.getProperty(\"some.property\", \"default.value\")" | _ + "System.setProperty(\"some.property\", \"new.value\")" | _ + "System.clearProperty(\"some.property\")" | _ + } +} diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInStaticGroovyIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInStaticGroovyIntegrationTest.groovy new file mode 100644 index 000000000000..b0f5812ed81f --- /dev/null +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/SystemPropertyInstrumentationInStaticGroovyIntegrationTest.groovy @@ -0,0 +1,125 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.inputs.undeclared + +import groovy.transform.CompileStatic +import org.gradle.api.Plugin +import org.gradle.api.Project +import org.gradle.configurationcache.AbstractConfigurationCacheIntegrationTest + +class SystemPropertyInstrumentationInStaticGroovyIntegrationTest extends AbstractConfigurationCacheIntegrationTest { + def "#method is instrumented in static Groovy"() { + def configurationCache = newConfigurationCacheFixture() + + given: + // Why the separate plugin? The Project.getProperties() is available in the build.gradle as getProperties(). + // Therefore, it is impossible to call System.getProperties() with static import there, and testing static + // import is important because Groovy generates different code in this case. + file("buildSrc/src/main/groovy/SomePlugin.groovy") << """ + import ${CompileStatic.name} + import ${Plugin.name} + import ${Project.name} + import static ${System.name}.clearProperty + import static ${System.name}.getProperties + import static ${System.name}.getProperty + import static ${System.name}.setProperty + + @CompileStatic + class SomePlugin implements Plugin { + void apply(Project project) { + def returned = $method + println("returned = \$returned") + } + } + """ + + buildScript(""" + apply plugin: SomePlugin + """) + + when: + configurationCacheRun("-Dsome.property=some.value") + + then: + configurationCache.assertStateStored() + outputContains("returned = some.value") + problems.assertResultHasProblems(result) { + withInput("Plugin class 'SomePlugin': system property 'some.property'") + } + + where: + method | _ + "System.properties['some.property']" | _ + "System.getProperties().get('some.property')" | _ + "getProperties().get('some.property')" | _ + "System.getProperty('some.property')" | _ + "getProperty('some.property')" | _ + "System.getProperty('some.property', 'default.value')" | _ + "getProperty('some.property', 'default.value')" | _ + "System.setProperty('some.property', 'new.value')" | _ + "setProperty('some.property', 'new.value')" | _ + "System.clearProperty('some.property')" | _ + "clearProperty('some.property')" | _ + } + + def "#setProperties is instrumented in static Groovy"() { + def configurationCache = newConfigurationCacheFixture() + + given: + buildScript(""" + import ${CompileStatic.name} + import static ${System.name}.setProperties + + @CompileStatic + class SomePlugin implements Plugin { + void apply(Project project) { + def newProps = new Properties() + System.properties.forEach { k, v -> newProps[k] = v } + newProps.replace("some.property", "new.value") + ${setProperties}(newProps) + } + } + + apply plugin: SomePlugin + + tasks.register("printProperty") { + doLast { + println("returned = \${System.getProperty("some.property")}") + } + } + """) + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateStored() + outputContains("returned = new.value") + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("returned = new.value") + + where: + setProperties | _ + "System.setProperties" | _ + "setProperties" | _ + } +} diff --git a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/UndeclaredBuildInputsIntegrationTest.groovy b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/UndeclaredBuildInputsIntegrationTest.groovy index 4c3b71b351ba..db5d51d5033e 100644 --- a/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/UndeclaredBuildInputsIntegrationTest.groovy +++ b/subprojects/configuration-cache/src/integTest/groovy/org/gradle/configurationcache/inputs/undeclared/UndeclaredBuildInputsIntegrationTest.groovy @@ -286,6 +286,226 @@ class UndeclaredBuildInputsIntegrationTest extends AbstractConfigurationCacheInt ] } + def "system property set at the configuration phase is restored when running from cache"() { + given: + buildFile(""" + $propertySetter + + tasks.register("printProperty") { + doLast { + println("some.property = \${System.properties["some.property"]}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("printProperty") + + then: + outputContains("some.property = $propertyValue") + + when: + configurationCacheRun("printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("some.property = $propertyValue") + + where: + propertyValue | propertySetter + "some.value" | """System.setProperty("some.property", "$propertyValue")""" + "some.value" | """System.properties["some.property"]="$propertyValue" """ + "1" | """System.properties["some.property"]=$propertyValue""" + } + + def "system property removed at the configuration phase is removed when running from cache"() { + given: + buildFile(""" + $propertyRemover + + tasks.register("printProperty") { + doLast { + println("some.property present = \${System.properties.containsKey("some.property")}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + outputContains("some.property present = false") + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("some.property present = false") + + where: + propertyRemover | _ + """System.clearProperty("some.property")""" | _ + """System.properties.remove("some.property")""" | _ + """System.getProperties().keySet().remove("some.property")""" | _ + } + + def "build logic can use setProperties at configuration phase"() { + given: + buildFile(""" + System.setProperty("some.removed.property", "removed.value") + def newProps = new Properties() + System.properties.forEach { k, v -> newProps.put(k, v) } + newProps.setProperty("some.property", "some.value") + newProps.remove("some.removed.property") + + System.setProperties(newProps) + tasks.register("printProperty") { + doLast { + println("some.property = \${System.properties.getProperty("some.property")}") + println("some.removed.property = \${System.properties.getProperty("some.removed.property")}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("printProperty") + + then: + configurationCache.assertStateStored() + outputContains("some.property = some.value") + outputContains("some.removed.property = null") + + when: + configurationCacheRun("printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("some.property = some.value") + outputContains("some.removed.property = null") + } + + def "properties set after clearing system properties with #systemPropsCleaner do not become inputs"() { + given: + buildFile(""" + def copiedProps = new HashMap<>(System.properties) + $systemPropsCleaner + + copiedProps.forEach { k, v -> System.setProperty(k, v) } + System.setProperty("some.property", "some.value") + + tasks.register("printProperty") { + doLast { + println("some.property = \${System.properties.getProperty("some.property")}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("printProperty") + + then: + configurationCache.assertStateStored() + outputContains("some.property = some.value") + + when: + configurationCacheRun("-Dsome.property=other.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("some.property = some.value") + + where: + systemPropsCleaner | _ + 'System.properties.clear()' | _ + 'System.properties.keySet().clear()' | _ + 'System.properties.entrySet().clear()' | _ + 'System.setProperties(new Properties())' | _ + } + + def "system property removed after update at the configuration phase is removed when running from cache"() { + given: + buildFile(""" + System.setProperty("some.property", "some.value") + System.clearProperty("some.property") + + tasks.register("printProperty") { + doLast { + println("some.property present = \${System.properties.containsKey("some.property")}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + outputContains("some.property present = false") + + when: + configurationCacheRun("-Dsome.property=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("some.property present = false") + } + + def "system property added and removed at the configuration phase is removed when running from cache even if set externally"() { + given: + buildFile(""" + System.properties.putAll(someProperty: "some.value") // Use putAll to avoid recording property as an input + System.clearProperty("someProperty") + + tasks.register("printProperty") { + doLast { + println("someProperty present = \${System.properties.containsKey("someProperty")}") + } + } + """) + + def configurationCache = newConfigurationCacheFixture() + + when: + configurationCacheRun("printProperty") + + then: + outputContains("someProperty present = false") + + when: + configurationCacheRun("-DsomeProperty=some.value", "printProperty") + + then: + configurationCache.assertStateLoaded() + outputContains("someProperty present = false") + } + + def "non-serializable system property is reported"() { + given: + buildFile(""" + System.properties["some.property"] = new Thread() + """) + + when: + configurationCacheFails() + + then: + problems.assertFailureHasProblems(failure) { + totalProblemsCount = 1 + withProblem("Build file 'build.gradle': cannot serialize object of type 'java.lang.Thread', a subtype of 'java.lang.Thread', as these are not supported with the configuration cache.") + problemsWithStackTraceCount = 0 + } + } + @Issue("https://github.com/gradle/gradle/issues/13155") def "plugin can bundle multiple resources with the same name"() { file("buildSrc/build.gradle") << """ @@ -433,8 +653,7 @@ class UndeclaredBuildInputsIntegrationTest extends AbstractConfigurationCacheInt then: configurationCache.assertStateLoaded() - // TODO(https://github.com/gradle/gradle/issues/18432) This should be changed to "true". - outputContains("execution time value=false") + outputContains("execution time value=true") } def "reports build logic reading environment variables with getenv(String) using GString parameters"() { diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/ConfigurationCacheState.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/ConfigurationCacheState.kt index 941ba16bb759..dd4149f66d5f 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/ConfigurationCacheState.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/ConfigurationCacheState.kt @@ -49,6 +49,7 @@ import org.gradle.configurationcache.serialization.withGradleIsolate import org.gradle.configurationcache.serialization.writeCollection import org.gradle.configurationcache.serialization.writeFile import org.gradle.configurationcache.serialization.writeStrings +import org.gradle.configurationcache.services.EnvironmentChangeTracker import org.gradle.execution.plan.Node import org.gradle.initialization.BuildOperationFiringSettingsPreparer import org.gradle.initialization.BuildOperationSettingsProcessor @@ -300,6 +301,9 @@ class ConfigurationCacheState( private suspend fun DefaultWriteContext.writeBuildTreeState(gradle: GradleInternal) { withGradleIsolate(gradle, userTypesCodec) { + withDebugFrame({ "environment state" }) { + writeCachedEnvironmentState(gradle) + } withDebugFrame({ "build cache" }) { writeBuildCacheConfiguration(gradle) } @@ -310,6 +314,7 @@ class ConfigurationCacheState( private suspend fun DefaultReadContext.readBuildTreeState(gradle: GradleInternal) { withGradleIsolate(gradle, userTypesCodec) { + readCachedEnvironmentState(gradle) readBuildCacheConfiguration(gradle) readGradleEnterprisePluginManager(gradle) } @@ -553,6 +558,19 @@ class ConfigurationCacheState( } } + private + suspend fun DefaultWriteContext.writeCachedEnvironmentState(gradle: GradleInternal) { + val environmentChangeTracker = gradle.serviceOf() + write(environmentChangeTracker.getCachedState()) + } + + private + suspend fun DefaultReadContext.readCachedEnvironmentState(gradle: GradleInternal) { + val environmentChangeTracker = gradle.serviceOf() + val storedState = read() as EnvironmentChangeTracker.CachedEnvironmentState + environmentChangeTracker.loadFrom(storedState) + } + private suspend fun DefaultWriteContext.writeGradleEnterprisePluginManager(gradle: GradleInternal) { val manager = gradle.serviceOf() diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/DefaultBuildTreeModelControllerServices.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/DefaultBuildTreeModelControllerServices.kt index df1f99ca5612..783e33230b9c 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/DefaultBuildTreeModelControllerServices.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/DefaultBuildTreeModelControllerServices.kt @@ -26,6 +26,7 @@ import org.gradle.configurationcache.problems.ConfigurationCacheProblems import org.gradle.configurationcache.serialization.beans.BeanStateReaderLookup import org.gradle.configurationcache.serialization.beans.BeanStateWriterLookup import org.gradle.configurationcache.serialization.codecs.jos.JavaSerializationEncodingLookup +import org.gradle.configurationcache.services.EnvironmentChangeTracker import org.gradle.internal.buildtree.BuildActionModelRequirements import org.gradle.internal.buildtree.BuildModelParameters import org.gradle.internal.buildtree.BuildTreeModelControllerServices @@ -94,6 +95,7 @@ class DefaultBuildTreeModelControllerServices : BuildTreeModelControllerServices registration.add(BuildModelParameters::class.java, modelParameters) registration.add(BuildActionModelRequirements::class.java, requirements) if (modelParameters.isConfigurationCache) { + registration.add(EnvironmentChangeTracker::class.java) registration.add(ConfigurationCacheBuildTreeLifecycleControllerFactory::class.java) registration.add(ConfigurationCacheStartParameter::class.java) registration.add(ConfigurationCacheClassLoaderScopeRegistryListener::class.java) diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/InstrumentedInputAccessListener.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/InstrumentedInputAccessListener.kt index 8c4ce4343487..378c9e411f7a 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/InstrumentedInputAccessListener.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/InstrumentedInputAccessListener.kt @@ -19,6 +19,7 @@ package org.gradle.configurationcache import org.gradle.api.file.FileCollection import org.gradle.configurationcache.initialization.ConfigurationCacheProblemsListener import org.gradle.configurationcache.serialization.Workarounds +import org.gradle.configurationcache.services.EnvironmentChangeTracker import org.gradle.internal.classpath.Instrumented import org.gradle.internal.event.ListenerManager import org.gradle.internal.service.scopes.Scopes @@ -67,26 +68,39 @@ val allowedProperties = setOf( class InstrumentedInputAccessListener( listenerManager: ListenerManager, configurationCacheProblemsListener: ConfigurationCacheProblemsListener, + private val environmentChangeTracker: EnvironmentChangeTracker, ) : Instrumented.Listener { private - val broadcast = listenerManager.getBroadcaster(UndeclaredBuildInputListener::class.java) + val undeclaredInputBroadcast = listenerManager.getBroadcaster(UndeclaredBuildInputListener::class.java) private val externalProcessListener = configurationCacheProblemsListener override fun systemPropertyQueried(key: String, value: Any?, consumer: String) { - if (allowedProperties.contains(key) || Workarounds.canReadSystemProperty(consumer)) { + if (allowedProperties.contains(key) || environmentChangeTracker.isSystemPropertyMutated(key) || Workarounds.canReadSystemProperty(consumer)) { return } - broadcast.systemPropertyRead(key, value, consumer) + undeclaredInputBroadcast.systemPropertyRead(key, value, consumer) + } + + override fun systemPropertyChanged(key: Any, value: Any?, consumer: String) { + environmentChangeTracker.systemPropertyChanged(key, value, consumer) + } + + override fun systemPropertyRemoved(key: Any, consumer: String) { + environmentChangeTracker.systemPropertyRemoved(key) + } + + override fun systemPropertiesCleared(consumer: String) { + environmentChangeTracker.systemPropertiesCleared() } override fun envVariableQueried(key: String, value: String?, consumer: String) { if (Workarounds.canReadEnvironmentVariable(consumer)) { return } - broadcast.envVariableRead(key, value, consumer) + undeclaredInputBroadcast.envVariableRead(key, value, consumer) } override fun externalProcessStarted(command: String, consumer: String) { @@ -100,10 +114,10 @@ class InstrumentedInputAccessListener( if (Workarounds.canReadFiles(consumer)) { return } - broadcast.fileOpened(file, consumer) + undeclaredInputBroadcast.fileOpened(file, consumer) } override fun fileCollectionObserved(fileCollection: FileCollection, consumer: String) { - broadcast.fileCollectionObserved(fileCollection, consumer) + undeclaredInputBroadcast.fileCollectionObserved(fileCollection, consumer) } } diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/JsonModelWriter.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/JsonModelWriter.kt index 38b72c8bf590..0ac2d776d9c2 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/JsonModelWriter.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/JsonModelWriter.kt @@ -121,6 +121,11 @@ class JsonModelWriter(val writer: Writer) { } } } + is PropertyTrace.SystemProperty -> { + property("kind", "SystemProperty") + comma() + property("name", trace.name) + } is PropertyTrace.Task -> { property("kind", "Task") comma() diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/PropertyProblem.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/PropertyProblem.kt index 5e9531336851..c275d53abcc8 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/PropertyProblem.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/problems/PropertyProblem.kt @@ -132,6 +132,14 @@ sealed class PropertyTrace { get() = trace.containingUserCode } + class SystemProperty( + val name: String, + val trace: PropertyTrace + ) : PropertyTrace() { + override val containingUserCode: String + get() = trace.containingUserCode + } + override fun toString(): String = StringBuilder().apply { sequence.forEach { @@ -159,6 +167,11 @@ sealed class PropertyTrace { quoted(trace.name) append(" of ") } + is SystemProperty -> { + append("system property ") + quoted(trace.name) + append(" set at ") + } is Bean -> { quoted(trace.type.name) append(" bean found in ") @@ -203,6 +216,7 @@ sealed class PropertyTrace { get() = when (this) { is Bean -> trace is Property -> trace + is SystemProperty -> trace else -> null } } diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/CachedEnvironmentStateCodec.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/CachedEnvironmentStateCodec.kt new file mode 100644 index 000000000000..a31d52bf0df9 --- /dev/null +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/CachedEnvironmentStateCodec.kt @@ -0,0 +1,77 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.serialization.codecs + +import org.gradle.configurationcache.ConfigurationCacheError +import org.gradle.configurationcache.ConfigurationCacheProblemsException +import org.gradle.configurationcache.extensions.maybeUnwrapInvocationTargetException +import org.gradle.configurationcache.problems.PropertyTrace +import org.gradle.configurationcache.problems.propertyDescriptionFor +import org.gradle.configurationcache.serialization.Codec +import org.gradle.configurationcache.serialization.ReadContext +import org.gradle.configurationcache.serialization.WriteContext +import org.gradle.configurationcache.serialization.readList +import org.gradle.configurationcache.serialization.withPropertyTrace +import org.gradle.configurationcache.serialization.writeCollection +import org.gradle.configurationcache.services.EnvironmentChangeTracker +import java.io.IOException + + +internal +object CachedEnvironmentStateCodec : Codec { + override suspend fun WriteContext.encode(value: EnvironmentChangeTracker.CachedEnvironmentState) { + writeBoolean(value.cleared) + + writeCollection(value.updates) { update -> + withPropertyTrace(PropertyTrace.SystemProperty(update.key.toString(), update.location ?: PropertyTrace.Unknown)) { + try { + write(update.key) + write(update.value) + } catch (passThrough: IOException) { + throw passThrough + } catch (passThrough: ConfigurationCacheProblemsException) { + throw passThrough + } catch (error: Exception) { + throw ConfigurationCacheError( + propertyDescriptionFor(trace), + error.maybeUnwrapInvocationTargetException() + ) + } + } + } + + writeCollection(value.removals) { removal -> + writeString(removal.key) + } + } + + override suspend fun ReadContext.decode(): EnvironmentChangeTracker.CachedEnvironmentState { + val cleared = readBoolean() + val updates = readList { + val key = read() as Any + val value = read() + EnvironmentChangeTracker.SystemPropertySet(key, value, null) + } + + val removals = readList { + val key = readString() + EnvironmentChangeTracker.SystemPropertyRemove(key) + } + + return EnvironmentChangeTracker.CachedEnvironmentState(cleared, updates, removals) + } +} diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/Codecs.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/Codecs.kt index 60cd8ea667dd..f444107c259d 100644 --- a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/Codecs.kt +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/serialization/codecs/Codecs.kt @@ -170,6 +170,8 @@ class Codecs( bind(TaskReferenceCodec) + bind(CachedEnvironmentStateCodec) + bind(IsolatedManagedValueCodec(managedFactoryRegistry)) bind(IsolatedImmutableManagedValueCodec(managedFactoryRegistry)) bind(IsolatedJavaSerializedValueSnapshotCodec) diff --git a/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTracker.kt b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTracker.kt new file mode 100644 index 000000000000..e731061962df --- /dev/null +++ b/subprojects/configuration-cache/src/main/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTracker.kt @@ -0,0 +1,181 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.services + +import org.gradle.configuration.internal.UserCodeApplicationContext +import org.gradle.configurationcache.problems.PropertyTrace +import org.gradle.configurationcache.problems.location +import org.gradle.internal.service.scopes.Scopes +import org.gradle.internal.service.scopes.ServiceScope +import java.util.concurrent.ConcurrentHashMap + + +/** + * The environment state that was mutated at the configuration phase and has to be restored before running a build from the cache. + * + * The class can operate in two modes. First is the tracking mode when all environment-modifying operations are stored + * and the list of the operations can be retrieved as the CachedEnvironmentState object. This mode is intended for the + * builds with the configuration phase. The second mode applies the restored state to the environment and doesn't + * track anything. This mode is used when the configuration is restored from the configuration cache. + * Mode selection happens upon the first use of the class. Calling an operation that isn't supported in the current + * mode results in the IllegalStateException. + */ +@ServiceScope(Scopes.BuildTree::class) +class EnvironmentChangeTracker(private val userCodeApplicationContext: UserCodeApplicationContext) { + private + val mode = ModeHolder() + + fun isSystemPropertyMutated(key: String) = mode.toTrackingMode().isSystemPropertyMutated(key) + + fun loadFrom(storedState: CachedEnvironmentState) = mode.toRestoringMode().loadFrom(storedState) + + fun getCachedState() = mode.toTrackingMode().getCachedState() + + fun systemPropertyChanged(key: Any, value: Any?, consumer: String) = mode.toTrackingMode().systemPropertyChanged(key, value, consumer) + + fun systemPropertyRemoved(key: Any) = mode.toTrackingMode().systemPropertyRemoved(key) + + fun systemPropertiesCleared() = mode.toTrackingMode().systemPropertiesCleared() + + private + inner class ModeHolder { + // ModeHolder encapsulates concurrent mode updates. + private + var mode: TrackerMode = Initial() + + private + inline fun setMode(transition: (TrackerMode) -> T): T { + synchronized(this) { + val newMode = transition(mode) + mode = newMode + return newMode + } + } + + fun toTrackingMode(): Tracking = setMode(TrackerMode::toTracking) + + fun toRestoringMode(): Restoring = setMode(TrackerMode::toRestoring) + } + + private + sealed interface TrackerMode { + fun toTracking(): Tracking + fun toRestoring(): Restoring + } + + private + inner class Initial : TrackerMode { + override fun toTracking(): Tracking { + return Tracking() + } + + override fun toRestoring(): Restoring { + return Restoring() + } + } + + private + inner class Tracking : TrackerMode { + private + val mutatedSystemProperties = ConcurrentHashMap() + + @Volatile + private + var systemPropertiesCleared = false + + override fun toTracking(): Tracking { + return this + } + + override fun toRestoring(): Restoring { + throw IllegalStateException("Cannot restore state because change tracking is already in progress") + } + + fun isSystemPropertyMutated(key: String): Boolean { + return systemPropertiesCleared || mutatedSystemProperties.containsKey(key) + } + + fun getCachedState(): CachedEnvironmentState { + return CachedEnvironmentState( + cleared = systemPropertiesCleared, + updates = mutatedSystemProperties.values.filterIsInstance(), + removals = mutatedSystemProperties.values.filterIsInstance() + ) + } + + fun systemPropertyChanged(key: Any, value: Any?, consumer: String) { + mutatedSystemProperties[key] = SystemPropertySet(key, value, userCodeApplicationContext.location(consumer)) + } + + fun systemPropertyRemoved(key: Any) { + if (key is String) { + // Externally set system properties can only use Strings as keys. If the removal argument is a string + // then it can affect externally set property and has to be persisted. Then removal will be applied on + // the next run from cache. + mutatedSystemProperties[key] = SystemPropertyRemove(key) + } else { + // If the key is not a string, it can only affect properties that were set by the build logic. There is + // no need to persist the removal, but the set value should not be persisted too. A placeholder value + // will keep the key mutated to avoid recording it as an input. + mutatedSystemProperties[key] = SystemPropertyIgnored + } + } + + fun systemPropertiesCleared() { + systemPropertiesCleared = true + mutatedSystemProperties.clear() + } + } + + private + class Restoring : TrackerMode { + override fun toTracking(): Tracking { + throw IllegalStateException("Cannot track state because it was restored") + } + + override fun toRestoring(): Restoring { + throw IllegalStateException("Cannot restore state because it was already restored") + } + + fun loadFrom(storedState: CachedEnvironmentState) { + if (storedState.cleared) { + System.getProperties().clear() + } + + storedState.updates.forEach { update -> + System.getProperties()[update.key] = update.value + } + + storedState.removals.forEach { removal -> + System.clearProperty(removal.key) + } + } + } + + class CachedEnvironmentState(val cleared: Boolean, val updates: List, val removals: List) + + sealed class SystemPropertyChange + + class SystemPropertySet(val key: Any, val value: Any?, val location: PropertyTrace?) : SystemPropertyChange() + class SystemPropertyRemove(val key: String) : SystemPropertyChange() + + /** + * This is a placeholder for system properties that were set but then removed. Having this in the map marks + * the property as mutated for the rest of the configuration phase but doesn't store the key in cache. + */ + object SystemPropertyIgnored : SystemPropertyChange() +} diff --git a/subprojects/configuration-cache/src/test/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTrackerTest.kt b/subprojects/configuration-cache/src/test/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTrackerTest.kt new file mode 100644 index 000000000000..583f96ac04a6 --- /dev/null +++ b/subprojects/configuration-cache/src/test/kotlin/org/gradle/configurationcache/services/EnvironmentChangeTrackerTest.kt @@ -0,0 +1,52 @@ +/* + * Copyright 2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.configurationcache.services + +import org.gradle.configuration.internal.DefaultUserCodeApplicationContext +import org.junit.Test + + +class EnvironmentChangeTrackerTest { + @Test(expected = IllegalStateException::class) + fun `loading state after first update throws`() { + val tracker = EnvironmentChangeTracker(DefaultUserCodeApplicationContext()) + + tracker.systemPropertyRemoved("some.property") + + tracker.loadFrom(emptyEnvironmentState()) + } + + @Test(expected = IllegalStateException::class) + fun `updating state after loading throws`() { + val tracker = EnvironmentChangeTracker(DefaultUserCodeApplicationContext()) + + tracker.loadFrom(emptyEnvironmentState()) + + tracker.systemPropertyRemoved("some.property") + } + + @Test(expected = IllegalStateException::class) + fun `loading twice throws`() { + val tracker = EnvironmentChangeTracker(DefaultUserCodeApplicationContext()) + + tracker.loadFrom(emptyEnvironmentState()) + tracker.loadFrom(emptyEnvironmentState()) + } + + private + fun emptyEnvironmentState() = EnvironmentChangeTracker.CachedEnvironmentState(cleared = false, updates = listOf(), removals = listOf()) +} diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingEnvMap.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingEnvMap.java index f49d0781cf79..778e7f3ecf3b 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingEnvMap.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingEnvMap.java @@ -62,7 +62,7 @@ public boolean containsKey(@Nullable Object key) { @Override public Set keySet() { - return new AccessTrackingSet<>(super.keySet(), this::getAndReport, this::reportAggregatingAccess); + return new AccessTrackingSet<>(super.keySet(), trackingListener()); } private String getAndReport(@Nullable Object key) { @@ -74,7 +74,7 @@ private String getAndReport(@Nullable Object key) { @Override public Set> entrySet() { - return new AccessTrackingSet<>(delegate.entrySet(), this::onAccessEntrySetElement, this::reportAggregatingAccess); + return new AccessTrackingSet<>(delegate.entrySet(), entrySetTrackingListener()); } private void onAccessEntrySetElement(@Nullable Object potentialEntry) { @@ -124,5 +124,53 @@ private void reportAggregatingAccess() { // Mark all map contents as inputs if some aggregating access is used. delegate.forEach(onAccess); } + + private AccessTrackingSet.Listener trackingListener() { + return new AccessTrackingSet.Listener() { + @Override + public void onAccess(Object o) { + getAndReport(o); + } + + @Override + public void onAggregatingAccess() { + reportAggregatingAccess(); + } + + @Override + public void onRemove(Object object) { + // Environment variables are immutable. + } + + @Override + public void onClear() { + // Environment variables are immutable. + } + }; + } + + private AccessTrackingSet.Listener entrySetTrackingListener() { + return new AccessTrackingSet.Listener() { + @Override + public void onAccess(Object o) { + onAccessEntrySetElement(o); + } + + @Override + public void onAggregatingAccess() { + reportAggregatingAccess(); + } + + @Override + public void onRemove(Object object) { + // Environment variables are immutable. + } + + @Override + public void onClear() { + // Environment variables are immutable. + } + }; + } } diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingProperties.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingProperties.java index 023953eb4366..2dd8050bbd04 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingProperties.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingProperties.java @@ -16,6 +16,8 @@ package org.gradle.internal.classpath; +import com.google.common.primitives.Primitives; + import javax.annotation.Nullable; import java.io.IOException; import java.io.InputStream; @@ -27,6 +29,7 @@ import java.util.Collection; import java.util.Enumeration; import java.util.Map; +import java.util.Objects; import java.util.Properties; import java.util.Set; import java.util.function.BiConsumer; @@ -35,16 +38,69 @@ /** * A wrapper for {@link Properties} that notifies a listener about accesses. - * interface. */ class AccessTrackingProperties extends Properties { + /** + * A listener that is notified about reads and modifications of the Properties instance. + * Note that there's no guarantee about the state of the Properties object when the + * listener's method is called because of modifying operation: it may happen before or after modification. + */ + public interface Listener { + /** + * Called when the property with the name {@code key} is read. The {@code value} is the value of the property observed by the caller. + * The Properties object may not contain the property with this name, the value is {@code null} then. Note that most modifying methods + * like {@link Properties#setProperty(String, String)} provide information about the previous value and trigger this method. + * All modifying operations call this method prior to {@link #onChange(Object, Object)}, {@link #onRemove(Object)} or {@link #onClear()}. + *

+ * When this method is called because of the modifying operation, the state of the observed Properties object is undefined for the duration of the + * call: it may be already completely or partially modified to reflect the result of the operation. + * + * @param key the key used by the caller to access the property + * @param value the value observed by the caller or {@code null} if there is no value for the given key + */ + void onAccess(Object key, @Nullable Object value); + + /** + * Called when the property with the name {@code key} is updated or added. The {@code newValue} is the new value of the property provided by + * the caller. If the modifying method provides a way for the caller to observe a previous value of the key then + * {@link #onAccess(Object, Object)} method is called prior to this method. + *

+ * The state of the observed Properties object is undefined for the duration of the call: it may be already completely or partially + * modified to reflect the result of the operation. + * + * @param key the key used by the caller to access the property + * @param newValue the value provided by the caller + */ + void onChange(Object key, Object newValue); + + /** + * Called when the property with the name {@code key} is removed. The Properties object may not contain the property prior to the modification. + * If the modifying method provides a way for the caller to observe a previous value of the key then {@link #onAccess(Object, Object)} method is + * called prior to this method. + *

+ * The state of the observed Properties object is undefined for the duration of the call: it may be already completely or partially + * modified to reflect the result of the operation. + * + * @param key the key used by the caller to access the property + */ + void onRemove(Object key); + + /** + * Called when the caller unconditionally removes all properties in this Properties object, for example by calling {@link Properties#clear()}. + *

+ * The state of the observed Properties object is undefined for the duration of the call: it may be already completely or partially + * modified to reflect the result of the operation. + */ + void onClear(); + } + // TODO(https://github.com/gradle/configuration-cache/issues/337) Only a limited subset of method is tracked currently. private final Properties delegate; - private final BiConsumer onAccess; + private final Listener listener; - public AccessTrackingProperties(Properties delegate, BiConsumer onAccess) { + public AccessTrackingProperties(Properties delegate, Listener listener) { this.delegate = delegate; - this.onAccess = onAccess; + this.listener = listener; } @Override @@ -55,7 +111,7 @@ public Enumeration propertyNames() { @Override public Set stringPropertyNames() { - return new AccessTrackingSet<>(delegate.stringPropertyNames(), this::getAndReport, this::reportAggregatingAccess); + return new AccessTrackingSet<>(delegate.stringPropertyNames(), trackingListener()); } @Override @@ -84,7 +140,7 @@ public Enumeration elements() { @Override public Set keySet() { - return new AccessTrackingSet<>(delegate.keySet(), this::getAndReport, this::reportAggregatingAccess); + return new AccessTrackingSet<>(delegate.keySet(), trackingListener()); } @Override @@ -94,13 +150,13 @@ public Collection values() { @Override public Set> entrySet() { - return new AccessTrackingSet<>(delegate.entrySet(), this::onAccessEntrySetElement, this::reportAggregatingAccess); + return new AccessTrackingSet<>(delegate.entrySet(), entrySetTrackingListener(), TrackingEntry::new); } private void onAccessEntrySetElement(@Nullable Object potentialEntry) { - Map.Entry entry = AccessTrackingUtils.tryConvertingToTrackableEntry(potentialEntry); + Map.Entry entry = AccessTrackingUtils.tryConvertingToEntry(potentialEntry); if (entry != null) { - getAndReport(entry.getKey()); + getAndReportAccess(entry.getKey()); } } @@ -113,47 +169,161 @@ public void forEach(BiConsumer action) { @Override public void replaceAll(BiFunction function) { reportAggregatingAccess(); - delegate.replaceAll(function); + synchronized (delegate) { + delegate.replaceAll((k, v) -> { + Object newValue = function.apply(k, v); + // It is a bit of optimization to avoid storing unnecessary stores when the value doesn't change. + // Strings and primitive wrappers are tested with "equals", user types are tested for reference + // equality to avoid problems with poorly-defined user-provided equality. + if (!simpleOrRefEquals(newValue, v)) { + reportChange(k, newValue); + } + return newValue; + }); + } } @Override public Object putIfAbsent(Object key, Object value) { - return delegate.putIfAbsent(key, value); + Object oldValue; + synchronized (delegate) { + oldValue = delegate.putIfAbsent(key, value); + } + reportAccess(key, oldValue); + if (oldValue == null) { + // Properties disallow null values, so it is safe to assume that the map was changed. + reportChange(key, value); + } + return oldValue; + } @Override public boolean remove(Object key, Object value) { - return delegate.remove(key, value); + Object oldValue; + boolean hadValue; + synchronized (delegate) { + oldValue = delegate.get(key); + hadValue = delegate.remove(key, value); + } + reportAccess(key, oldValue); + if (hadValue) { + // The configuration cache uses onRemove callback to remember that the property has to be removed. + // Of course, the property has to be removed in the cached run only if it was removed in the + // non-cached run first. Changing the value of the property would invalidate the cache and recalculate the removal. + reportRemoval(key); + } + return hadValue; + } @Override - public boolean replace(Object key, Object oldValue, Object newValue) { - return delegate.replace(key, oldValue, newValue); + public boolean replace(Object key, Object expectedOldValue, Object newValue) { + Object oldValue; + boolean changed; + synchronized (delegate) { + oldValue = delegate.get(key); + changed = delegate.replace(key, expectedOldValue, newValue); + } + reportAccess(key, oldValue); + if (changed) { + // The configuration cache uses onChange callback to remember that the property has to be changed. + // Of course, the property has to be changed in the cached run only if it was changed in the + // non-cached run first. Changing the value of the property externally would invalidate the cache and recalculate the replacement. + reportChange(key, newValue); + } + return changed; + } @Override public Object replace(Object key, Object value) { - return delegate.replace(key, value); + Object oldValue; + synchronized (delegate) { + oldValue = delegate.replace(key, value); + } + reportAccess(key, oldValue); + if (oldValue != null) { + reportChange(key, value); + } + return oldValue; + } @Override public Object computeIfAbsent(Object key, Function mappingFunction) { - return delegate.computeIfAbsent(key, mappingFunction); + Object oldValue; + Object computedValue = null; + synchronized (delegate) { + oldValue = delegate.get(key); + if (oldValue == null) { + computedValue = delegate.computeIfAbsent(key, mappingFunction); + } + } + reportAccess(key, oldValue); + if (computedValue != null) { + reportChange(key, computedValue); + return computedValue; + } + return oldValue; } @Override public Object computeIfPresent(Object key, BiFunction remappingFunction) { - return delegate.computeIfPresent(key, remappingFunction); + Object oldValue; + Object computedValue = null; + synchronized (delegate) { + oldValue = delegate.get(key); + if (oldValue != null) { + computedValue = delegate.computeIfPresent(key, remappingFunction); + } + } + reportAccess(key, oldValue); + if (oldValue != null) { + if (computedValue != null) { + reportChange(key, computedValue); + } else { + reportRemoval(key); + } + } + return computedValue; } + @Override public Object compute(Object key, BiFunction remappingFunction) { - return delegate.compute(key, remappingFunction); + Object oldValue; + Object newValue; + synchronized (delegate) { + oldValue = delegate.get(key); + newValue = delegate.compute(key, remappingFunction); + } + reportAccess(key, oldValue); + if (newValue != null) { + reportChange(key, newValue); + } else if (oldValue != null) { + reportRemoval(key); + } + return newValue; } + @Override public Object merge(Object key, Object value, BiFunction remappingFunction) { - return delegate.merge(key, value, remappingFunction); + Object oldValue; + Object newValue; + synchronized (delegate) { + oldValue = delegate.get(key); + newValue = delegate.merge(key, value, remappingFunction); + } + reportAccess(key, oldValue); + if (newValue != null) { + reportChange(key, newValue); + } else if (oldValue != null) { + reportRemoval(key); + } + return newValue; + } @Override @@ -168,34 +338,53 @@ public boolean containsValue(Object value) { @Override public boolean containsKey(Object key) { - return getAndReport(key) != null; + return getAndReportAccess(key) != null; } @Override public Object put(Object key, Object value) { - return delegate.put(key, value); + Object oldValue; + synchronized (delegate) { + oldValue = delegate.put(key, value); + } + reportAccess(key, oldValue); + reportChange(key, value); + return oldValue; + } @Override public Object setProperty(String key, String value) { - return delegate.setProperty(key, value); + return put(key, value); } @Override public Object remove(Object key) { - Object result = delegate.remove(key); - reportKeyAndValue(key, result); + Object result; + synchronized (delegate) { + result = delegate.remove(key); + } + reportAccess(key, result); + reportRemoval(key); return result; + } @Override public void putAll(Map t) { - delegate.putAll(t); + synchronized (delegate) { + delegate.putAll(t); + } + // putAll has no return value so keys do not become inputs. + t.forEach(listener::onChange); } @Override public void clear() { - delegate.clear(); + synchronized (delegate) { + delegate.clear(); + } + reportClear(); } @Override @@ -205,20 +394,20 @@ public String getProperty(String key) { @Override public String getProperty(String key, String defaultValue) { - Object oValue = getAndReport(key); + Object oValue = getAndReportAccess(key); String value = oValue instanceof String ? (String) oValue : null; return value != null ? value : defaultValue; } @Override public Object getOrDefault(Object key, Object defaultValue) { - Object value = getAndReport(key); + Object value = getAndReportAccess(key); return value != null ? value : defaultValue; } @Override public Object get(Object key) { - return getAndReport(key); + return getAndReportAccess(key); } @Override @@ -298,20 +487,149 @@ public int hashCode() { return delegate.hashCode(); } - private Object getAndReport(Object key) { + private Object getAndReportAccess(Object key) { Object value = delegate.get(key); - reportKeyAndValue(key, value); + reportAccess(key, value); return value; } - private void reportKeyAndValue(Object key, Object value) { - if (key instanceof String && (value == null || value instanceof String)) { - onAccess.accept((String) key, (String) value); - } + private void reportAccess(Object key, Object value) { + listener.onAccess(key, value); } private void reportAggregatingAccess() { // Mark all map contents as inputs if some aggregating access is used. - delegate.forEach(this::reportKeyAndValue); + delegate.forEach(this::reportAccess); + } + + private void reportChange(Object key, Object value) { + listener.onChange(key, value); + } + + private void reportRemoval(Object key) { + listener.onRemove(key); + } + + private void reportClear() { + listener.onClear(); + } + + /** + * Tests equality two objects with {@code equals} if the objects are Strings or primitive wrappers. Otherwise, the equality of references is tested (i.e. {@code lhs == rhs}). + * + * @param lhs the first object (can be {@code null}) + * @param rhs the second object (can be {@code null}) + * @return {@code true} if the objects are equal in the sense described above + */ + private static boolean simpleOrRefEquals(@Nullable Object lhs, @Nullable Object rhs) { + if (lhs == rhs) { + return true; + } + if (lhs == null || rhs == null) { + return false; + } + Class lhsClass = lhs.getClass(); + if (lhsClass == rhs.getClass() && isSimpleType(lhsClass)) { + return Objects.equals(lhs, rhs); + } + return false; + } + + private static boolean isSimpleType(Class clazz) { + return clazz == String.class || Primitives.isWrapperType(clazz); + } + + private AccessTrackingSet.Listener trackingListener() { + return new AccessTrackingSet.Listener() { + @Override + public void onAccess(Object o) { + getAndReportAccess(o); + } + + @Override + public void onAggregatingAccess() { + reportAggregatingAccess(); + } + + @Override + public void onRemove(Object object) { + reportRemoval(object); + } + + @Override + public void onClear() { + reportClear(); + } + }; + } + + private AccessTrackingSet.Listener entrySetTrackingListener() { + return new AccessTrackingSet.Listener() { + @Override + public void onAccess(Object o) { + onAccessEntrySetElement(o); + } + + @Override + public void onAggregatingAccess() { + reportAggregatingAccess(); + } + + @Override + public void onRemove(Object potentialEntry) { + Map.Entry entry = AccessTrackingUtils.tryConvertingToEntry(potentialEntry); + if (entry != null) { + reportRemoval(entry.getKey()); + } + } + + @Override + public void onClear() { + reportClear(); + } + }; + } + + private class TrackingEntry implements Map.Entry { + private final Map.Entry delegate; + + TrackingEntry(Map.Entry delegate) { + this.delegate = delegate; + } + + @Override + public Object getKey() { + return delegate.getKey(); + } + + @Override + public Object getValue() { + return delegate.getValue(); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof Map.Entry)) { + return false; + } + Map.Entry that = (Map.Entry) o; + return Objects.equals(delegate.getKey(), that.getKey()) && Objects.equals(delegate.getValue(), that.getValue()); + } + + @Override + public int hashCode() { + return delegate.hashCode(); + } + + @Override + public Object setValue(Object value) { + Object oldValue = delegate.setValue(value); + listener.onAccess(getKey(), oldValue); + reportChange(getKey(), value); + return oldValue; + } } } diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingSet.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingSet.java index eb1e128303bb..e1fa7bbe7ad8 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingSet.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingSet.java @@ -17,34 +17,50 @@ package org.gradle.internal.classpath; import com.google.common.collect.ForwardingSet; +import com.google.common.collect.Iterators; import javax.annotation.Nonnull; import javax.annotation.Nullable; import java.util.Collection; import java.util.Iterator; import java.util.Set; -import java.util.function.Consumer; +import java.util.function.Function; /** * The special-cased implementation of {@link Set} that tracks all accesses to its elements. + * * @param the type of elements */ class AccessTrackingSet extends ForwardingSet { + public interface Listener { + void onAccess(Object o); + + void onAggregatingAccess(); + + void onRemove(Object object); + + void onClear(); + } + // TODO(https://github.com/gradle/configuration-cache/issues/337) Only a limited subset of entrySet/keySet methods are currently tracked. private final Set delegate; - private final Consumer onAccess; - private final Runnable onAggregatingAccess; + private final Listener listener; + private final Function factory; + + public AccessTrackingSet(Set delegate, Listener listener) { + this(delegate, listener, Function.identity()); + } - public AccessTrackingSet(Set delegate, Consumer onAccess, Runnable onAggregatingAccess) { + public AccessTrackingSet(Set delegate, Listener listener, Function factory) { this.delegate = delegate; - this.onAccess = onAccess; - this.onAggregatingAccess = onAggregatingAccess; + this.listener = listener; + this.factory = factory; } @Override public boolean contains(@Nullable Object o) { boolean result = delegate.contains(o); - onAccess.accept(o); + listener.onAccess(o); return result; } @@ -52,7 +68,7 @@ public boolean contains(@Nullable Object o) { public boolean containsAll(@Nonnull Collection collection) { boolean result = delegate.containsAll(collection); for (Object o : collection) { - onAccess.accept(o); + listener.onAccess(o); } return result; } @@ -60,7 +76,8 @@ public boolean containsAll(@Nonnull Collection collection) { @Override public boolean remove(Object o) { // We cannot perform modification before notifying because the listener may want to query the state of the delegate prior to that. - onAccess.accept(o); + listener.onAccess(o); + listener.onRemove(o); return delegate.remove(o); } @@ -68,15 +85,22 @@ public boolean remove(Object o) { public boolean removeAll(Collection collection) { // We cannot perform modification before notifying because the listener may want to query the state of the delegate prior to that. for (Object o : collection) { - onAccess.accept(o); + listener.onAccess(o); + listener.onRemove(o); } return delegate.removeAll(collection); } + @Override + public void clear() { + delegate.clear(); + listener.onClear(); + } + @Override public Iterator iterator() { reportAggregatingAccess(); - return delegate().iterator(); + return Iterators.transform(delegate().iterator(), factory::apply); } @Override @@ -105,14 +129,25 @@ public int hashCode() { @Override public Object[] toArray() { - reportAggregatingAccess(); - return delegate.toArray(); + // this is basically a reimplementation of the standardToArray that doesn't call this.size() + // and avoids double-reporting of the aggregating access. + return toArray(new Object[0]); } @Override + @SuppressWarnings({"unchecked", "SuspiciousToArrayCall"}) public T[] toArray(T[] array) { reportAggregatingAccess(); - return delegate.toArray(array); + T[] result = delegate().toArray(array); + for (int i = 0; i < result.length; ++i) { + // The elements of result have to be of some subtype of E because of Set's invariant, + // so the inner cast is safe. The outer cast might be problematic if T is a some subtype + // of E and the factory function returns some other subtype. However, this is unlikely + // to happen in our use cases. Only System.getProperties().entrySet implementation uses + // this conversion. + result[i] = (T) factory.apply((E) result[i]); + } + return result; } @Override @@ -123,6 +158,6 @@ protected Set delegate() { } private void reportAggregatingAccess() { - onAggregatingAccess.run(); + listener.onAggregatingAccess(); } } diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingUtils.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingUtils.java index 4f41ca45a4d8..a0b21cfdcd55 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingUtils.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/AccessTrackingUtils.java @@ -44,10 +44,10 @@ private AccessTrackingUtils() { */ @Nullable public static Map.Entry tryConvertingToTrackableEntry(Object o) { - if (!(o instanceof Map.Entry)) { + Map.Entry entry = tryConvertingToEntry(o); + if (entry == null) { return null; } - Map.Entry entry = (Map.Entry) o; // Return a copy to make sure that the results of getKey() and getValue() do not change. Object key = entry.getKey(); Object value = entry.getValue(); @@ -57,4 +57,11 @@ public static Map.Entry tryConvertingToTrackableEntry(Object o) return null; } + @Nullable + public static Map.Entry tryConvertingToEntry(Object o) { + if (!(o instanceof Map.Entry)) { + return null; + } + return (Map.Entry) o; + } } diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java index f2ec8ef2159d..6b24a761f4ee 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/Instrumented.java @@ -42,6 +42,18 @@ public class Instrumented { public void systemPropertyQueried(String key, @Nullable Object value, String consumer) { } + @Override + public void systemPropertyChanged(Object key, @Nullable Object value, String consumer) { + } + + @Override + public void systemPropertyRemoved(Object key, String consumer) { + } + + @Override + public void systemPropertiesCleared(String consumer) { + } + @Override public void envVariableQueried(String key, @Nullable String value, String consumer) { } @@ -77,7 +89,17 @@ public static void groovyCallSites(CallSiteArray array) { case "getProperty": array.array[callSite.getIndex()] = new SystemPropertyCallSite(callSite); break; + case "setProperty": + array.array[callSite.getIndex()] = new SetSystemPropertyCallSite(callSite); + break; + case "setProperties": + array.array[callSite.getIndex()] = new SetSystemPropertiesCallSite(callSite); + break; + case "clearProperty": + array.array[callSite.getIndex()] = new ClearSystemPropertyCallSite(callSite); + break; case "properties": + case "getProperties": array.array[callSite.getIndex()] = new SystemPropertiesCallSite(callSite); break; case "getInteger": @@ -125,11 +147,54 @@ public static String systemProperty(String key, @Nullable String defaultValue, S // Called by generated code. public static Properties systemProperties(String consumer) { - return new AccessTrackingProperties(System.getProperties(), (k, v) -> { - systemPropertyQueried(convertToString(k), convertToString(v), consumer); + return new AccessTrackingProperties(System.getProperties(), new AccessTrackingProperties.Listener() { + // Do not track accesses to non-String properties. Only String properties can be set externally, so they cannot affect the cached configuration. + @Override + public void onAccess(Object key, @Nullable Object value) { + if (key instanceof String && (value == null || value instanceof String)) { + systemPropertyQueried(convertToString(key), convertToString(value), consumer); + } + } + + @Override + public void onChange(Object key, Object newValue) { + listener().systemPropertyChanged(key, newValue, consumer); + } + + @Override + public void onRemove(Object key) { + listener().systemPropertyRemoved(key, consumer); + } + + @Override + public void onClear() { + listener().systemPropertiesCleared(consumer); + } }); } + // Called by generated code. + public static String setSystemProperty(String key, String value, String consumer) { + String oldValue = System.setProperty(key, value); + systemPropertyQueried(key, oldValue, consumer); + listener().systemPropertyChanged(key, value, consumer); + return oldValue; + } + + // Called by generated code. + public static String clearSystemProperty(String key, String consumer) { + String oldValue = System.clearProperty(key); + systemPropertyQueried(key, oldValue, consumer); + listener().systemPropertyRemoved(key, consumer); + return oldValue; + } + + public static void setSystemProperties(Properties properties, String consumer) { + listener().systemPropertiesCleared(consumer); + properties.forEach((k, v) -> listener().systemPropertyChanged(k, v, consumer)); + System.setProperties(properties); + } + // Called by generated code. public static Integer getInteger(String key, String consumer) { systemPropertyQueried(key, consumer); @@ -360,10 +425,38 @@ private static String joinCommand(List command) { public interface Listener { /** - * @param consumer The name of the class that is reading the property value + * Invoked when the code reads the system property with the String key. + * + * @param key the name of the property + * @param value the value of the property at the time of reading or {@code null} if the property is not present + * @param consumer the name of the class that is reading the property value */ void systemPropertyQueried(String key, @Nullable Object value, String consumer); + /** + * Invoked when the code updates or adds the system property. + * + * @param key the name of the property, can be non-string + * @param value the new value of the property, can be {@code null} or non-string + * @param consumer the name of the class that is updating the property value + */ + void systemPropertyChanged(Object key, @Nullable Object value, String consumer); + + /** + * Invoked when the code removes the system property. The property may not be present. + * + * @param key the name of the property, can be non-string + * @param consumer the name of the class that is removing the property value + */ + void systemPropertyRemoved(Object key, String consumer); + + /** + * Invoked when all system properties are removed. + * + * @param consumer the name of the class that is removing the system properties + */ + void systemPropertiesCleared(String consumer); + /** * Invoked when the code reads the environment variable. * @@ -473,6 +566,15 @@ public Object call(Object receiver, Object arg) throws Throwable { } } + @Override + public Object callStatic(Class receiver, Object arg1) throws Throwable { + if (receiver.equals(System.class)) { + return systemProperty(arg1.toString(), array.owner.getName()); + } else { + return super.callStatic(receiver, arg1); + } + } + @Override public Object call(Object receiver, Object arg1, Object arg2) throws Throwable { if (receiver.equals(System.class)) { @@ -481,6 +583,89 @@ public Object call(Object receiver, Object arg1, Object arg2) throws Throwable { return super.call(receiver, arg1, arg2); } } + + @Override + public Object callStatic(Class receiver, Object arg1, Object arg2) throws Throwable { + if (receiver.equals(System.class)) { + return systemProperty(arg1.toString(), convertToString(arg2), array.owner.getName()); + } else { + return super.callStatic(receiver, arg1, arg2); + } + } + } + + private static class SetSystemPropertyCallSite extends AbstractCallSite { + public SetSystemPropertyCallSite(CallSite callSite) { + super(callSite); + } + + @Override + public Object call(Object receiver, Object arg1, Object arg2) throws Throwable { + if (receiver.equals(System.class)) { + return setSystemProperty(convertToString(arg1), convertToString(arg2), array.owner.getName()); + } else { + return super.call(receiver, arg1, arg2); + } + } + + @Override + public Object callStatic(Class receiver, Object arg1, Object arg2) throws Throwable { + if (receiver.equals(System.class)) { + return setSystemProperty(convertToString(arg1), convertToString(arg2), array.owner.getName()); + } else { + return super.callStatic(receiver, arg1, arg2); + } + } + } + + private static class SetSystemPropertiesCallSite extends AbstractCallSite { + public SetSystemPropertiesCallSite(CallSite callSite) { + super(callSite); + } + + @Override + public Object call(Object receiver, Object arg1) throws Throwable { + if (receiver.equals(System.class) && arg1 instanceof Properties) { + setSystemProperties((Properties) arg1, array.owner.getName()); + return null; + } else { + return super.call(receiver, arg1); + } + } + + @Override + public Object callStatic(Class receiver, Object arg1) throws Throwable { + if (receiver.equals(System.class) && arg1 instanceof Properties) { + setSystemProperties((Properties) arg1, array.owner.getName()); + return null; + } else { + return super.callStatic(receiver, arg1); + } + } + } + + private static class ClearSystemPropertyCallSite extends AbstractCallSite { + public ClearSystemPropertyCallSite(CallSite callSite) { + super(callSite); + } + + @Override + public Object call(Object receiver, Object arg1) throws Throwable { + if (receiver.equals(System.class)) { + return clearSystemProperty(convertToString(arg1), array.owner.getName()); + } else { + return super.call(receiver, arg1); + } + } + + @Override + public Object callStatic(Class receiver, Object arg1) throws Throwable { + if (receiver.equals(System.class)) { + return clearSystemProperty(convertToString(arg1), array.owner.getName()); + } else { + return super.callStatic(receiver, arg1); + } + } } private static class SystemPropertiesCallSite extends AbstractCallSite { @@ -496,6 +681,24 @@ public Object callGetProperty(Object receiver) throws Throwable { return super.callGetProperty(receiver); } } + + @Override + public Object call(Object receiver) throws Throwable { + if (receiver.equals(System.class)) { + return systemProperties(array.owner.getName()); + } else { + return super.call(receiver); + } + } + + @Override + public Object callStatic(Class receiver) throws Throwable { + if (receiver.equals(System.class)) { + return systemProperties(array.owner.getName()); + } else { + return super.callStatic(receiver); + } + } } private static class GetEnvCallSite extends AbstractCallSite { diff --git a/subprojects/core/src/main/java/org/gradle/internal/classpath/InstrumentingTransformer.java b/subprojects/core/src/main/java/org/gradle/internal/classpath/InstrumentingTransformer.java index 9780c31be9b0..81bf57a54360 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/classpath/InstrumentingTransformer.java +++ b/subprojects/core/src/main/java/org/gradle/internal/classpath/InstrumentingTransformer.java @@ -62,7 +62,7 @@ class InstrumentingTransformer implements CachedClasspathTransformer.Transform { /** * Decoration format. Increment this when making changes. */ - private static final int DECORATION_FORMAT = 18; + private static final int DECORATION_FORMAT = 19; private static final Type SYSTEM_TYPE = getType(System.class); private static final Type STRING_TYPE = getType(String.class); @@ -72,6 +72,7 @@ class InstrumentingTransformer implements CachedClasspathTransformer.Transform { private static final Type SERIALIZED_LAMBDA_TYPE = getType(SerializedLambda.class); private static final Type LONG_TYPE = getType(Long.class); private static final Type BOOLEAN_TYPE = getType(Boolean.class); + public static final Type PROPERTIES_TYPE = getType(Properties.class); private static final String RETURN_STRING = getMethodDescriptor(STRING_TYPE); private static final String RETURN_STRING_FROM_STRING = getMethodDescriptor(STRING_TYPE, STRING_TYPE); @@ -93,8 +94,10 @@ class InstrumentingTransformer implements CachedClasspathTransformer.Transform { private static final String RETURN_PRIMITIVE_BOOLEAN_FROM_STRING_STRING = getMethodDescriptor(Type.BOOLEAN_TYPE, STRING_TYPE, STRING_TYPE); private static final String RETURN_OBJECT_FROM_INT = getMethodDescriptor(OBJECT_TYPE, Type.INT_TYPE); private static final String RETURN_BOOLEAN_FROM_OBJECT = getMethodDescriptor(Type.BOOLEAN_TYPE, OBJECT_TYPE); - private static final String RETURN_PROPERTIES = getMethodDescriptor(getType(Properties.class)); - private static final String RETURN_PROPERTIES_FROM_STRING = getMethodDescriptor(getType(Properties.class), STRING_TYPE); + private static final String RETURN_PROPERTIES = getMethodDescriptor(PROPERTIES_TYPE); + private static final String RETURN_PROPERTIES_FROM_STRING = getMethodDescriptor(PROPERTIES_TYPE, STRING_TYPE); + private static final String RETURN_VOID_FROM_PROPERTIES = getMethodDescriptor(Type.VOID_TYPE, PROPERTIES_TYPE); + private static final String RETURN_VOID_FROM_PROPERTIES_STRING = getMethodDescriptor(Type.VOID_TYPE, PROPERTIES_TYPE, STRING_TYPE); private static final String RETURN_CALL_SITE_ARRAY = getMethodDescriptor(getType(CallSiteArray.class)); private static final String RETURN_VOID_FROM_CALL_SITE_ARRAY = getMethodDescriptor(Type.VOID_TYPE, getType(CallSiteArray.class)); private static final String RETURN_OBJECT_FROM_SERIALIZED_LAMBDA = getMethodDescriptor(OBJECT_TYPE, SERIALIZED_LAMBDA_TYPE); @@ -330,6 +333,18 @@ private boolean visitINVOKESTATIC(String owner, String name, String descriptor) _LDC(binaryClassNameOf(className)); _INVOKESTATIC(INSTRUMENTED_TYPE, "systemProperties", RETURN_PROPERTIES_FROM_STRING); return true; + } else if (name.equals("setProperties") && descriptor.equals(RETURN_VOID_FROM_PROPERTIES)) { + _LDC(binaryClassNameOf(className)); + _INVOKESTATIC(INSTRUMENTED_TYPE, "setSystemProperties", RETURN_VOID_FROM_PROPERTIES_STRING); + return true; + } else if (name.equals("setProperty") && descriptor.equals(RETURN_STRING_FROM_STRING_STRING)) { + _LDC(binaryClassNameOf(className)); + _INVOKESTATIC(INSTRUMENTED_TYPE, "setSystemProperty", RETURN_STRING_FROM_STRING_STRING_STRING); + return true; + } else if (name.equals("clearProperty") && descriptor.equals(RETURN_STRING_FROM_STRING)) { + _LDC(binaryClassNameOf(className)); + _INVOKESTATIC(INSTRUMENTED_TYPE, "clearSystemProperty", RETURN_STRING_FROM_STRING_STRING); + return true; } else if (name.equals("getenv")) { if (descriptor.equals(RETURN_STRING_FROM_STRING)) { // System.getenv(String) -> String diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AbstractAccessTrackingMapTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AbstractAccessTrackingMapTest.groovy index 9a797c631bff..dace0cda148b 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AbstractAccessTrackingMapTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AbstractAccessTrackingMapTest.groovy @@ -24,7 +24,7 @@ import java.util.function.Consumer abstract class AbstractAccessTrackingMapTest extends Specification { protected final Map innerMap = ['existing': 'existingValue', 'other': 'otherValue'] - protected final BiConsumer consumer = Mock() + protected final BiConsumer onAccess = Mock() protected abstract Map getMapUnderTestToRead() @@ -34,8 +34,8 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -49,8 +49,8 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -65,8 +65,8 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -80,8 +80,8 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: !result - 1 * consumer.accept('missing', null) - 0 * consumer._ + 1 * onAccess.accept('missing', null) + 0 * onAccess._ } def "aggregating method #methodName reports all map contents as inputs"() { @@ -89,9 +89,9 @@ abstract class AbstractAccessTrackingMapTest extends Specification { operation.accept(getMapUnderTestToRead()) then: - (1.._) * consumer.accept('existing', 'existingValue') - (1.._) * consumer.accept('other', 'otherValue') - 0 * consumer._ + (1.._) * onAccess.accept('existing', 'existingValue') + (1.._) * onAccess.accept('other', 'otherValue') + 0 * onAccess._ where: methodName | operation @@ -111,7 +111,7 @@ abstract class AbstractAccessTrackingMapTest extends Specification { getMapUnderTestToRead().toString() then: - 0 * consumer._ + 0 * onAccess._ } def "entrySet() contains(entry(#key, #requestedValue)) and containsAll(entry(#key, #requestedValue)) are tracked"() { @@ -120,16 +120,16 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: containsResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ when: def containsAllResult = getMapUnderTestToRead().entrySet().containsAll(Collections.singleton(entry(key, requestedValue))) then: containsAllResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | requestedValue | reportedValue | expectedResult @@ -145,9 +145,9 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: result == expectedResult - 1 * consumer.accept(key1, reportedValue1) - 1 * consumer.accept(key2, reportedValue2) - 0 * consumer._ + 1 * onAccess.accept(key1, reportedValue1) + 1 * onAccess.accept(key2, reportedValue2) + 0 * onAccess._ where: key1 | requestedValue1 | reportedValue1 | key2 | requestedValue2 | reportedValue2 | expectedResult @@ -163,16 +163,16 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: containsResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ when: def containsAllResult = getMapUnderTestToRead().keySet().containsAll(Collections.singleton(key)) then: containsAllResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -186,9 +186,9 @@ abstract class AbstractAccessTrackingMapTest extends Specification { then: result == expectedResult - 1 * consumer.accept(key1, reportedValue1) - 1 * consumer.accept(key2, reportedValue2) - 0 * consumer._ + 1 * onAccess.accept(key1, reportedValue1) + 1 * onAccess.accept(key2, reportedValue2) + 0 * onAccess._ where: key1 | reportedValue1 | key2 | reportedValue2 | expectedResult diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingEnvMapTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingEnvMapTest.groovy index 42091b4a19f4..553090d89625 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingEnvMapTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingEnvMapTest.groovy @@ -19,7 +19,7 @@ package org.gradle.internal.classpath class AccessTrackingEnvMapTest extends AbstractAccessTrackingMapTest { @Override protected Map getMapUnderTestToRead() { - return new AccessTrackingEnvMap(innerMap, consumer) + return new AccessTrackingEnvMap(innerMap, onAccess) } def "access to non-string element with containsKey throws"() { @@ -28,6 +28,6 @@ class AccessTrackingEnvMapTest extends AbstractAccessTrackingMapTest { then: thrown(RuntimeException) - 0 * consumer._ + 0 * onAccess._ } } diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesNonStringTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesNonStringTest.groovy index e9255b80641f..a74b6caf3b55 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesNonStringTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesNonStringTest.groovy @@ -21,8 +21,6 @@ import com.google.common.collect.ImmutableSet import com.google.common.collect.Maps import spock.lang.Specification -import java.util.function.BiConsumer - import static org.gradle.internal.classpath.AccessTrackingPropertiesNonStringTest.TestData.EXISTING_KEY import static org.gradle.internal.classpath.AccessTrackingPropertiesNonStringTest.TestData.EXISTING_VALUE import static org.gradle.internal.classpath.AccessTrackingPropertiesNonStringTest.TestData.MISSING_KEY @@ -39,23 +37,27 @@ class AccessTrackingPropertiesNonStringTest extends Specification { 'existing', 'existingStringValue', 'keyWithNonStringValue', NON_STRING_VALUE ) - private final BiConsumer consumer = Mock() + private final AccessTrackingProperties.Listener listener = Mock() protected Properties getMapUnderTestToRead() { return getMapUnderTestToWrite() } protected Properties getMapUnderTestToWrite() { - return new AccessTrackingProperties(propertiesWithContent(innerMap), consumer) + return getMapUnderTestToWrite(innerMap) + } + + protected Properties getMapUnderTestToWrite(Map contents) { + return new AccessTrackingProperties(propertiesWithContent(contents), listener) } - def "get(#key) is not tracked for non-strings"() { + def "get(#key) is tracked for non-strings"() { when: def result = getMapUnderTestToRead().get(key) then: result == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedResult) where: key | expectedResult @@ -64,103 +66,107 @@ class AccessTrackingPropertiesNonStringTest extends Specification { 'keyWithNonStringValue' | NON_STRING_VALUE } - def "getOrDefault(#key) is not tracked for non-strings"() { + def "getOrDefault(#key) is tracked for non-strings"() { when: def result = getMapUnderTestToRead().getOrDefault(key, 'defaultValue') then: result == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, trackedValue) where: - key | expectedResult - EXISTING_KEY | EXISTING_VALUE - MISSING_KEY | 'defaultValue' - 'keyWithNonStringValue' | NON_STRING_VALUE + key | trackedValue | expectedResult + EXISTING_KEY | EXISTING_VALUE | EXISTING_VALUE + MISSING_KEY | null | 'defaultValue' + 'keyWithNonStringValue' | NON_STRING_VALUE | NON_STRING_VALUE } - def "containsKey(#key) is not tracked for non-strings"() { + def "containsKey(#key) is tracked for non-strings"() { when: def result = getMapUnderTestToRead().containsKey(key) then: result == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, trackedValue) where: - key | expectedResult - EXISTING_KEY | true - MISSING_KEY | false - 'keyWithNonStringValue' | true + key | trackedValue | expectedResult + EXISTING_KEY | EXISTING_VALUE | true + MISSING_KEY | null | false + 'keyWithNonStringValue' | NON_STRING_VALUE | true } - def "getProperty(String) is not tracked for non-string values"() { + def "getProperty(String) is tracked for non-string values"() { when: def result = getMapUnderTestToRead().getProperty('keyWithNonStringValue') then: result == null - 0 * consumer._ + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) } - def "getProperty(String, String) is not tracked for non-string values"() { + def "getProperty(String, String) is tracked for non-string values"() { when: def result = getMapUnderTestToRead().getProperty('keyWithNonStringValue', 'defaultValue') then: result == 'defaultValue' - 0 * consumer._ + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) } - def "forEach is tracked for strings only"() { + def "forEach is tracked for non-strings"() { when: HashMap iterated = new HashMap<>() getMapUnderTestToRead().forEach(iterated::put) then: iterated.keySet() == innerMap.keySet() - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } - def "entrySet() enumeration is tracked for strings only"() { + def "entrySet() enumeration is tracked for non-strings"() { when: def result = ImmutableSet.copyOf(getMapUnderTestToRead().entrySet()) then: result == innerMap.entrySet() - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } - def "entrySet() contains(entry(#key, #requestedValue)) and containsAll(entry(#key, #requestedValue)) are not tracked for non-strings"() { + def "entrySet() contains(entry(#key, #requestedValue)) and containsAll(entry(#key, #requestedValue)) are tracked for non-strings"() { when: def containsResult = getMapUnderTestToRead().entrySet().contains(entry(key, requestedValue)) then: containsResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) when: def containsAllResult = getMapUnderTestToRead().entrySet().containsAll(Collections.singleton(entry(key, requestedValue))) then: containsAllResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) where: - key | requestedValue | expectedResult - 'existing' | null | false - EXISTING_KEY | EXISTING_VALUE | true - EXISTING_KEY | OTHER_VALUE | false - EXISTING_KEY | null | false - MISSING_KEY | EXISTING_VALUE | false - 'keyWithNonStringValue' | NON_STRING_VALUE | true - 'keyWithNonStringValue' | OTHER_VALUE | false - 'keyWithNonStringValue' | null | false + key | expectedValue | requestedValue | expectedResult + 'existing' | 'existingStringValue' | null | false + EXISTING_KEY | EXISTING_VALUE | EXISTING_VALUE | true + EXISTING_KEY | EXISTING_VALUE | OTHER_VALUE | false + EXISTING_KEY | EXISTING_VALUE | null | false + MISSING_KEY | null | EXISTING_VALUE | false + 'keyWithNonStringValue' | NON_STRING_VALUE | NON_STRING_VALUE | true + 'keyWithNonStringValue' | NON_STRING_VALUE | OTHER_VALUE | false + 'keyWithNonStringValue' | NON_STRING_VALUE | null | false } - def "entrySet() containsAll() is tracked for strings only"() { + def "entrySet() containsAll() is tracked for all entries"() { when: def result = getMapUnderTestToRead().entrySet().containsAll(Arrays.asList( entry(EXISTING_KEY, EXISTING_VALUE), @@ -168,8 +174,10 @@ class AccessTrackingPropertiesNonStringTest extends Specification { entry('existing', 'existingStringValue'))) then: result - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } def "keySet() enumeration is tracked for strings only"() { @@ -178,79 +186,90 @@ class AccessTrackingPropertiesNonStringTest extends Specification { then: result == innerMap.keySet() - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } - def "keySet() contains(#key) and containsAll(#key) are not tracked for non-strings"() { + def "keySet() contains(#key) and containsAll(#key) are tracked for non-strings"() { when: def containsResult = getMapUnderTestToRead().keySet().contains(key) then: containsResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) when: def containsAllResult = getMapUnderTestToRead().keySet().containsAll(Collections. singleton(key)) then: containsAllResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) where: - key | expectedResult - EXISTING_KEY | true - MISSING_KEY | false - 'keyWithNonStringValue' | true + key | expectedValue | expectedResult + EXISTING_KEY | EXISTING_VALUE | true + MISSING_KEY | null | false + 'keyWithNonStringValue' | NON_STRING_VALUE | true } - def "keySet() containsAll() is tracked for strings only"() { + def "keySet() containsAll() is tracked for non-strings"() { when: def result = getMapUnderTestToRead().keySet().containsAll(Arrays.asList(EXISTING_KEY, 'keyWithNonStringValue', 'existing')) then: result - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } - def "stringPropertyNames() contains(#key) and containsAll(#key) are not tracked for non-strings"() { + def "stringPropertyNames() contains(#key) and containsAll(#key) are tracked for non-strings"() { when: def containsResult = getMapUnderTestToRead().stringPropertyNames().contains(key) then: containsResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) when: def containsAllResult = getMapUnderTestToRead().stringPropertyNames().containsAll(Collections. singleton(key)) then: containsAllResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) where: - key | expectedResult - EXISTING_KEY | false - MISSING_KEY | false - 'keyWithNonStringValue' | false + key | expectedValue | expectedResult + EXISTING_KEY | EXISTING_VALUE | false + MISSING_KEY | null | false + 'keyWithNonStringValue' | NON_STRING_VALUE | false } - def "stringPropertyNames() containsAll() is tracked for strings only"() { + def "stringPropertyNames() containsAll() is tracked for non-stringsy"() { when: def result = getMapUnderTestToRead().stringPropertyNames().containsAll(Arrays.asList(EXISTING_KEY, 'keyWithNonStringValue', 'existing')) then: !result - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 0 * listener._ } - def "remove(#key) is not tracked for non-strings"() { + def "remove(#key) is tracked for non-strings"() { when: def result = getMapUnderTestToWrite().remove(key) then: result == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedResult) + then: + 1 * listener.onRemove(key) + then: + 0 * listener._ + where: key | expectedResult EXISTING_KEY | EXISTING_VALUE @@ -258,62 +277,109 @@ class AccessTrackingPropertiesNonStringTest extends Specification { 'keyWithNonStringValue' | NON_STRING_VALUE } - def "keySet() remove(#key) and removeAll(#key) are not tracked for non-strings"() { + def "keySet() remove(#key) and removeAll(#key) are tracked for non-strings"() { when: def removeResult = getMapUnderTestToWrite().keySet().remove(key) then: removeResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) + then: + 1 * listener.onRemove(key) + then: + 0 * listener._ when: def removeAllResult = getMapUnderTestToWrite().keySet().removeAll(Collections. singleton(key)) then: removeAllResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) + then: + 1 * listener.onRemove(key) + then: + 0 * listener._ where: - key | expectedResult - EXISTING_KEY | true - MISSING_KEY | false - 'keyWithNonStringValue' | true + key | expectedValue | expectedResult + EXISTING_KEY | EXISTING_VALUE | true + MISSING_KEY | null | false + 'keyWithNonStringValue' | NON_STRING_VALUE | true } - def "keySet() removeAll() is tracked for strings only"() { + def "keySet() removeAll() is tracked for non-strings"() { when: - def result = getMapUnderTestToRead().keySet().removeAll(Arrays.asList(EXISTING_KEY, 'keyWithNonStringValue', 'existing')) + def result = getMapUnderTestToWrite().keySet().removeAll(Arrays.asList(EXISTING_KEY, 'keyWithNonStringValue', 'existing')) then: result - 1 * consumer.accept('existing', 'existingStringValue') - 0 * consumer._ + 1 * listener.onAccess(EXISTING_KEY, EXISTING_VALUE) + 1 * listener.onAccess('existing', 'existingStringValue') + 1 * listener.onAccess('keyWithNonStringValue', NON_STRING_VALUE) + 1 * listener.onRemove(EXISTING_KEY) + 1 * listener.onRemove('existing') + 1 * listener.onRemove('keyWithNonStringValue') + 0 * listener._ } - def "entrySet() remove(#key) and removeAll(#key) are not tracked for non-strings"() { + def "entrySet() remove(#key) and removeAll(#key) are tracked for non-strings"() { when: def removeResult = getMapUnderTestToWrite().entrySet().remove(entry(key, requestedValue)) then: removeResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) + 1 * listener.onRemove(key) + 0 * listener._ when: def removeAllResult = getMapUnderTestToWrite().entrySet().removeAll(Collections.singleton(entry(key, requestedValue))) then: removeAllResult == expectedResult - 0 * consumer._ + 1 * listener.onAccess(key, expectedValue) + then: + 1 * listener.onRemove(key) + then: + 0 * listener._ where: - key | requestedValue | expectedResult - 'existing' | null | false - EXISTING_KEY | EXISTING_VALUE | true - EXISTING_KEY | OTHER_VALUE | false - EXISTING_KEY | null | false - MISSING_KEY | EXISTING_VALUE | false - 'keyWithNonStringValue' | NON_STRING_VALUE | true - 'keyWithNonStringValue' | OTHER_VALUE | false - 'keyWithNonStringValue' | null | false + key | expectedValue | requestedValue | expectedResult + 'existing' | 'existingStringValue' | null | false + EXISTING_KEY | EXISTING_VALUE | EXISTING_VALUE | true + EXISTING_KEY | EXISTING_VALUE | OTHER_VALUE | false + EXISTING_KEY | EXISTING_VALUE | null | false + MISSING_KEY | null | EXISTING_VALUE | false + 'keyWithNonStringValue' | NON_STRING_VALUE | NON_STRING_VALUE | true + 'keyWithNonStringValue' | NON_STRING_VALUE | OTHER_VALUE | false + 'keyWithNonStringValue' | NON_STRING_VALUE | null | false + } + + def "replaceAll() uses equals for primitive wrappers and Strings"() { + def map = getMapUnderTestToWrite(intValue: 100500, stringValue: "value") + when: + map.replaceAll { k, v -> + switch (k) { + case "intValue": return Integer.valueOf((int) v) + case "stringValue": return new String((String) v) + } + } + + then: + 0 * listener.onChange(_, _) + } + + def "replaceAll() uses reference equality for non-primitives"() { + def value1 = [1] + def value2 = [1] + + def map = getMapUnderTestToWrite(value: value1) + when: + map.replaceAll { k, v -> + return value2 + } + + then: + 1 * listener.onChange("value", value2) } private static Properties propertiesWithContent(Map contents) { diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesTest.groovy index 4be54e797aba..89129503ac40 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingPropertiesTest.groovy @@ -19,16 +19,45 @@ package org.gradle.internal.classpath import com.google.common.io.ByteStreams import com.google.common.io.CharStreams +import javax.annotation.Nullable +import java.util.function.BiConsumer +import java.util.function.BiFunction import java.util.function.Consumer class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { + private BiConsumer onChange = Mock() + private Consumer onRemove = Mock() + private Runnable onClear = Mock() + + private AccessTrackingProperties.Listener listener = new AccessTrackingProperties.Listener() { + @Override + void onAccess(Object key, @Nullable Object value) { + onAccess.accept(key, value) + } + + @Override + void onChange(Object key, @Nullable Object newValue) { + onChange.accept(key, newValue) + } + + @Override + void onRemove(Object key) { + onRemove.accept(key) + } + + @Override + void onClear() { + onClear.run() + } + } + @Override protected Properties getMapUnderTestToRead() { return getMapUnderTestToWrite() } protected Properties getMapUnderTestToWrite() { - return new AccessTrackingProperties(propertiesWithContent(innerMap), consumer) + return new AccessTrackingProperties(propertiesWithContent(innerMap), listener) } def "getProperty(#key) is tracked"() { @@ -37,8 +66,8 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -52,8 +81,8 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -67,16 +96,16 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: containsResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ when: def containsAllResult = getMapUnderTestToRead().stringPropertyNames().containsAll(Collections.singleton(key)) then: containsAllResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + 0 * onAccess._ where: key | expectedResult | reportedValue @@ -90,9 +119,9 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: result == expectedResult - 1 * consumer.accept(key1, reportedValue1) - 1 * consumer.accept(key2, reportedValue2) - 0 * consumer._ + 1 * onAccess.accept(key1, reportedValue1) + 1 * onAccess.accept(key2, reportedValue2) + 0 * onAccess._ where: key1 | reportedValue1 | key2 | reportedValue2 | expectedResult @@ -107,30 +136,66 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: result == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + then: + 1 * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ + where: key | reportedValue | expectedResult 'existing' | 'existingValue' | 'existingValue' 'missing' | null | null } + def "remove(#key, #removedValue) is tracked"() { + when: + def result = getMapUnderTestToWrite().remove(key, removedValue) + + then: + result == expectedResult + 1 * onAccess.accept(key, reportedValue) + then: + expectedReportedRemovalsCount * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ + + where: + key | removedValue | reportedValue | expectedResult + 'existing' | 'existingValue' | 'existingValue' | true + 'existing' | 'missingValue' | 'existingValue' | false + 'missing' | 'missingValue' | null | false + + // onRemove is only called if the value is actually removed + expectedReportedRemovalsCount = expectedResult ? 1 : 0 + } + def "keySet() remove(#key) and removeAll(#key) are tracked"() { when: def removeResult = getMapUnderTestToWrite().keySet().remove(key) then: removeResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + then: + 1 * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ when: def removeAllResult = getMapUnderTestToWrite().keySet().removeAll(Collections.singleton(key)) then: removeAllResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + then: + 1 * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ where: key | reportedValue | expectedResult @@ -144,16 +209,24 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { then: removeResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + then: + 1 * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ when: def removeAllResult = getMapUnderTestToWrite().entrySet().removeAll(Collections.singleton(entry(key, requestedValue))) then: removeAllResult == expectedResult - 1 * consumer.accept(key, reportedValue) - 0 * consumer._ + 1 * onAccess.accept(key, reportedValue) + then: + 1 * onRemove.accept(key) + then: + 0 * onAccess._ + 0 * onRemove._ where: key | requestedValue | reportedValue | expectedResult @@ -167,9 +240,9 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { operation.accept(getMapUnderTestToRead()) then: - (1.._) * consumer.accept('existing', 'existingValue') - (1.._) * consumer.accept('other', 'otherValue') - 0 * consumer._ + (1.._) * onAccess.accept('existing', 'existingValue') + (1.._) * onAccess.accept('other', 'otherValue') + 0 * onAccess._ where: methodName | operation @@ -191,10 +264,11 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { def "method #methodName does not report properties as inputs"() { when: - operation.accept(getMapUnderTestToRead()) + operation.accept(getMapUnderTestToWrite()) then: - 0 * consumer._ + 0 * onAccess._ + where: methodName | operation "toString()" | call(p -> p.toString()) @@ -203,6 +277,384 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { "load(InputStream)" | call(p -> p.load(new ByteArrayInputStream(new byte[0]))) } + def "method put(#key, #newValue) reports argument as input and change"() { + when: + def result = getMapUnderTestToWrite().put(key, newValue) + + then: + result == oldValue + 1 * onAccess.accept(key, oldValue) + then: + 1 * onChange.accept(key, newValue) + + where: + key | oldValue | newValue + 'existing' | 'existingValue' | 'changed' + 'missingKey' | null | 'changed' + } + + def "method put(#key, #newValue) updates map"() { + given: + def map = getMapUnderTestToWrite() + + when: + map.put(key, newValue) + + then: + map.get(key) == newValue + + where: + key | oldValue | newValue + 'existing' | 'existingValue' | 'changed' + 'missingKey' | null | 'changed' + } + + def "method putAll() reports argument as change only"() { + when: + def update = [existing: 'changedExisting', missing: 'changedMissing'] + getMapUnderTestToWrite().putAll(update) + + then: + 1 * onChange.accept('existing', 'changedExisting') + 1 * onChange.accept('missing', 'changedMissing') + 0 * onAccess._ + } + + def "method putIfAbsent(#key, newValue) reports change"() { + when: + def result = getMapUnderTestToWrite().putIfAbsent(key, 'newValue') + + then: + result == oldValue + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, 'newValue') + then: + 0 * onChange._ + 0 * onAccess._ + + where: + key | oldValue | changeCount + 'existing' | 'existingValue' | 0 + 'missing' | null | 1 + } + + def "method replaceAll changes map"() { + when: + def map = getMapUnderTestToWrite() + map.replaceAll((k, v) -> { + if (k == 'existing') { + return 'newValue' + } + return v + }) + + then: + (Map) map == [existing: 'newValue', other: 'otherValue'] + } + + def "method replaceAll reports access to every item"() { + when: + getMapUnderTestToWrite().replaceAll((k, v) -> { + if (k == 'existing') { + return 'newValue' + } + return v + }) + + then: + 1 * onAccess.accept('existing', 'existingValue') + 1 * onAccess.accept('other', 'otherValue') + then: + 0 * onAccess._ + } + + def "method replaceAll reports access before change"() { + when: + getMapUnderTestToWrite().replaceAll((k, v) -> { + if (k == 'existing') { + return 'newValue' + } + return v + }) + + then: + 1 * onAccess.accept('existing', 'existingValue') + then: + 1 * onChange.accept('existing', 'newValue') + then: + 0 * onChange._ + } + + def "method replace(#key, #oldValue, newValue) changes map"() { + when: + def map = getMapUnderTestToWrite() + def result = map.replace(key, oldValue, 'newValue') + + then: + result == expectedResult + map[key] == expectedValue + + where: + key | oldValue | expectedValue | expectedResult + 'existing' | 'existingValue' | 'newValue' | true + 'existing' | 'otherValue' | 'existingValue' | false + 'missing' | 'otherValue' | null | false + + } + + def "method replace(#key, #oldValue, newValue) reports change"() { + when: + getMapUnderTestToWrite().replace(key, oldValue, 'newValue') + + then: + 1 * onAccess.accept(key, expectedOldValue) + then: + changeCount * onChange.accept(key, 'newValue') + then: + 0 * onChange._ + 0 * onAccess._ + + where: + key | expectedOldValue | oldValue | changeCount + 'existing' | 'existingValue' | expectedOldValue | 1 + 'existing' | 'existingValue' | 'otherValue' | 0 + 'missing' | null | 'otherValue' | 0 + } + + def "method replace(#key, newValue) changes map"() { + when: + def map = getMapUnderTestToWrite() + def result = map.replace(key, 'newValue') + + then: + result == oldValue + map[key] == expectedValue + + where: + key | oldValue | expectedValue + 'existing' | 'existingValue' | 'newValue' + 'missing' | null | null + } + + def "method replace(#key, newValue) reports change"() { + when: + getMapUnderTestToWrite().replace(key, 'newValue') + + then: + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, 'newValue') + then: + 0 * onChange._ + 0 * onAccess._ + + where: + key | oldValue + 'existing' | 'existingValue' + 'missing' | null + + changeCount = oldValue != null ? 1 : 0 + } + + def "method computeIfAbsent(#key, #newValue) changes map"() { + when: + def map = getMapUnderTestToWrite() + def result = map.computeIfAbsent(key, k -> newValue) + + then: + result == expectedResult + map[key] == expectedResult + + where: + key | newValue | expectedResult + 'existing' | 'newValue' | 'existingValue' + 'existing' | null | 'existingValue' + 'missing' | 'newValue' | 'newValue' + 'missing' | null | null + } + + def "method computeIfAbsent(#key, #newValue) reports change"() { + when: + getMapUnderTestToWrite().computeIfAbsent(key, k -> newValue) + + then: + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, newValue) + then: + 0 * onAccess._ + 0 * onChange._ + 0 * onRemove._ + + where: + key | newValue | oldValue | changeCount + 'existing' | 'newValue' | 'existingValue' | 0 + 'existing' | null | 'existingValue' | 0 + 'missing' | 'newValue' | null | 1 + 'missing' | null | null | 0 + } + + def "method computeIfPresent(#key, #newValue) changes map"() { + when: + def map = getMapUnderTestToWrite() + def result = map.computeIfPresent(key, (k, v) -> newValue) + + then: + result == expectedResult + map[key] == expectedResult + + where: + key | newValue | expectedResult + 'existing' | 'newValue' | 'newValue' + 'existing' | null | null + 'missing' | 'newValue' | null + 'missing' | null | null + } + + def "method computeIfPresent(#key, #newValue) reports change"() { + when: + getMapUnderTestToWrite().computeIfPresent(key, (k, v) -> newValue) + + then: + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, newValue) + removeCount * onRemove.accept(key) + then: + 0 * onChange._ + 0 * onAccess._ + 0 * onRemove._ + + where: + key | newValue | oldValue | changeCount | removeCount + 'existing' | 'newValue' | 'existingValue' | 1 | 0 + 'existing' | null | 'existingValue' | 0 | 1 + 'missing' | 'newValue' | null | 0 | 0 + 'missing' | null | null | 0 | 0 + } + + def "method compute(#key, #newValue) changes map"() { + when: + def map = getMapUnderTestToWrite() + def result = map.compute(key, (k, v) -> newValue) + + then: + result == expectedResult + map[key] == expectedResult + + where: + key | newValue | expectedResult + 'existing' | 'newValue' | 'newValue' + 'existing' | null | null + 'missing' | 'newValue' | 'newValue' + 'missing' | null | null + } + + def "method compute(#key, #newValue) reports change"() { + when: + getMapUnderTestToWrite().compute(key, (k, v) -> newValue) + + then: + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, newValue) + removeCount * onRemove.accept(key) + then: + 0 * onChange._ + 0 * onAccess._ + 0 * onRemove._ + + where: + key | newValue | oldValue | changeCount | removeCount + 'existing' | 'newValue' | 'existingValue' | 1 | 0 + 'existing' | null | 'existingValue' | 0 | 1 + 'missing' | 'newValue' | null | 1 | 0 + 'missing' | null | null | 0 | 0 + } + + def "method merge(#key, #newValue) changes map"(String key, String newValue, BiFunction func, String expectedResult) { + when: + def map = getMapUnderTestToWrite() + def result = map.merge(key, newValue, func) + + then: + result == expectedResult + map[key] == expectedResult + + where: + key | newValue | func | expectedResult + 'existing' | 'Concat' | String::concat | 'existingValueConcat' + 'existing' | '' | AccessTrackingPropertiesTest::removeMapping | null + 'missing' | 'newValue' | String::concat | 'newValue' + } + + def "method merge(#key, #newValue) reports change"(String key, String newValue, String oldValue, BiFunction func) { + when: + getMapUnderTestToWrite().merge(key, newValue, func) + + then: + 1 * onAccess.accept(key, oldValue) + then: + changeCount * onChange.accept(key, changed) + removeCount * onRemove.accept(key) + + then: + 0 * onAccess._ + 0 * onChange._ + 0 * onRemove._ + + where: + key | newValue | oldValue | func | changed | removeCount + 'existing' | 'Concat' | 'existingValue' | String::concat | "$oldValue$newValue" | 0 + 'existing' | '' | 'existingValue' | AccessTrackingPropertiesTest::removeMapping | null | 1 + 'missing' | 'newValue' | null | String::concat | newValue | 0 + + changeCount = changed != null ? 1 : 0 + } + + def "entry method setValue changes map"() { + when: + def map = getMapUnderTestToWrite() + def entry = map.entrySet().find { entry -> entry.getKey() == 'existing' } + + def result = entry.setValue('newValue') + + then: + result == 'existingValue' + map['existing'] == 'newValue' + } + + def "entry method setValue reports change"() { + when: + def entry = getMapUnderTestToWrite().entrySet().find { entry -> entry.getKey() == 'existing' } + + // reset a mock + entry.setValue('newValue') + + then: + // 1st invocation comes from "find()" call, 2nd is for the setValue result. + // TODO(mlopatkin) replace 1st call with a proper aggregating access handling. + 2 * onAccess.accept('existing', 'existingValue') + then: + 1 * onChange.accept('existing', 'newValue') + then: + 0 * onChange._ + } + + def "method clear() notifies listener and changes map"() { + def map = getMapUnderTestToWrite() + when: + map.clear() + + then: + map.isEmpty() + 1 * onClear.run() + 0 * onAccess._ + 0 * onRemove._ + 0 * onChange._ + } + private static Properties propertiesWithContent(Map contents) { Properties props = new Properties() props.putAll(contents) @@ -213,4 +665,8 @@ class AccessTrackingPropertiesTest extends AbstractAccessTrackingMapTest { private static Consumer call(Consumer consumer) { return consumer } + + private static Object removeMapping(Object key, Object value) { + return null + } } diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingSetTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingSetTest.groovy index 8289a28ece09..ed3bf9e4c16c 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingSetTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/AccessTrackingSetTest.groovy @@ -21,10 +21,9 @@ import spock.lang.Specification import java.util.function.Consumer class AccessTrackingSetTest extends Specification { - private final Consumer consumer = Mock() - private final Runnable aggregatingAccess = Mock() + private final AccessTrackingSet.Listener listener = Mock() private final Set inner = new HashSet<>(Arrays.asList('existing', 'other')) - private final AccessTrackingSet set = new AccessTrackingSet<>(inner, consumer, aggregatingAccess) + private final AccessTrackingSet set = new AccessTrackingSet<>(inner, listener) def "contains of existing element is tracked"() { when: @@ -32,9 +31,8 @@ class AccessTrackingSetTest extends Specification { then: result - 1 * consumer.accept('existing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('existing') + 0 * listener._ } def "contains of null is tracked"() { @@ -43,9 +41,8 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept(null) - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess(null) + 0 * listener._ } def "contains of missing element is tracked"() { @@ -54,9 +51,8 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept('missing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('missing') + 0 * listener._ } def "contains of inconvertible element is tracked"() { @@ -65,9 +61,8 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept(123) - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess(123) + 0 * listener._ } def "containsAll of existing elements is tracked"() { @@ -76,10 +71,9 @@ class AccessTrackingSetTest extends Specification { then: result - 1 * consumer.accept('existing') - 1 * consumer.accept('other') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('existing') + 1 * listener.onAccess('other') + 0 * listener._ } def "containsAll of missing elements is tracked"() { @@ -88,10 +82,9 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept('missing') - 1 * consumer.accept('alsoMissing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('missing') + 1 * listener.onAccess('alsoMissing') + 0 * listener._ } def "containsAll of missing and existing elements is tracked"() { @@ -100,10 +93,9 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept('missing') - 1 * consumer.accept('existing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('missing') + 1 * listener.onAccess('existing') + 0 * listener._ } def "remove of existing element is tracked"() { @@ -112,9 +104,9 @@ class AccessTrackingSetTest extends Specification { then: result - 1 * consumer.accept('existing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('existing') + 1 * listener.onRemove('existing') + 0 * listener._ } def "remove of missing element is tracked"() { @@ -123,45 +115,62 @@ class AccessTrackingSetTest extends Specification { then: !result - 1 * consumer.accept('missing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('missing') + then: + 1 * listener.onRemove('missing') + then: + 0 * listener._ } def "removeAll of existing elements is tracked"() { when: - def result = set.removeAll('existing', 'other') + def result = set.removeAll(['existing', 'other']) then: result - 1 * consumer.accept('existing') - 1 * consumer.accept('other') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('existing') + then: + 1 * listener.onRemove('existing') + then: + 1 * listener.onAccess('other') + then: + 1 * listener.onRemove('other') + then: + 0 * listener._ } def "removeAll of missing elements is tracked"() { when: - def result = set.removeAll('missing', 'alsoMissing') + def result = set.removeAll(['missing', 'alsoMissing']) then: !result - 1 * consumer.accept('missing') - 1 * consumer.accept('alsoMissing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('missing') + then: + 1 * listener.onRemove('missing') + then: + 1 * listener.onAccess('alsoMissing') + then: + 1 * listener.onRemove('alsoMissing') + then: + 0 * listener._ } def "removeAll of existing and missing elements is tracked"() { when: - def result = set.removeAll('existing', 'missing') + def result = set.removeAll(['existing', 'missing']) then: result - 1 * consumer.accept('existing') - 1 * consumer.accept('missing') - 0 * consumer._ - 0 * aggregatingAccess._ + 1 * listener.onAccess('existing') + then: + 1 * listener.onRemove('existing') + then: + 1 * listener.onAccess('missing') + then: + 1 * listener.onRemove('missing') + then: + 0 * listener._ } def "method #methodName is reported as aggregating"() { @@ -169,8 +178,8 @@ class AccessTrackingSetTest extends Specification { operation.accept(set) then: - 0 * consumer._ - (1.._) * aggregatingAccess.run() + (1.._) * listener.onAggregatingAccess() + 0 * listener._ where: methodName | operation @@ -186,18 +195,26 @@ class AccessTrackingSetTest extends Specification { "removeIf(Predicate)" | call(s -> s.removeIf(e -> false)) } + def "method clear is reported"() { + when: + set.clear() + + then: + inner.isEmpty() + 1 * listener.onClear() + 0 * listener._ + } + def "method #methodName is not reported as aggregating"() { when: operation.accept(set) then: - 0 * consumer._ - 0 * aggregatingAccess.run() + 0 * listener._ where: methodName | operation "retainAll()" | call(s -> s.retainAll(Collections.emptySet())) - "clear()" | call(s -> s.clear()) "toString()" | call(s -> s.toString()) } diff --git a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/DefaultCachedClasspathTransformerTest.groovy b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/DefaultCachedClasspathTransformerTest.groovy index 2c8dc3f349cc..0bef3a199a02 100644 --- a/subprojects/core/src/test/groovy/org/gradle/internal/classpath/DefaultCachedClasspathTransformerTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/internal/classpath/DefaultCachedClasspathTransformerTest.groovy @@ -235,7 +235,7 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec { def file = testDir.file("thing.jar") jar(file) def classpath = DefaultClassPath.of(file) - def cachedFile = testDir.file("cached/edd5a925756f9e184e86c343f144191a/thing.jar") + def cachedFile = testDir.file("cached/a0ff919f7115600becaeee69464d82cf/thing.jar") when: def cachedClasspath = transformer.transform(classpath, BuildLogic) @@ -263,7 +263,7 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec { def dir = testDir.file("thing.dir") classesDir(dir) def classpath = DefaultClassPath.of(dir) - def cachedFile = testDir.file("cached/a4cdf563229efdb0dd6757a7e5469f9b/thing.dir.jar") + def cachedFile = testDir.file("cached/2a394a2c80ba9bd4e62f7bfe48f4367c/thing.dir.jar") when: def cachedClasspath = transformer.transform(classpath, BuildLogic) @@ -293,8 +293,8 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec { def file = testDir.file("thing.jar") jar(file) def classpath = DefaultClassPath.of(dir, file) - def cachedDir = testDir.file("cached/a4cdf563229efdb0dd6757a7e5469f9b/thing.dir.jar") - def cachedFile = testDir.file("cached/edd5a925756f9e184e86c343f144191a/thing.jar") + def cachedDir = testDir.file("cached/2a394a2c80ba9bd4e62f7bfe48f4367c/thing.dir.jar") + def cachedFile = testDir.file("cached/a0ff919f7115600becaeee69464d82cf/thing.jar") when: def cachedClasspath = transformer.transform(classpath, BuildLogic) @@ -351,8 +351,8 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec { def file3 = testDir.file("thing3.jar") jar(file3) def classpath = DefaultClassPath.of(dir, file, dir2, file2, dir3, file3) - def cachedDir = testDir.file("cached/a4cdf563229efdb0dd6757a7e5469f9b/thing.dir.jar") - def cachedFile = testDir.file("cached/edd5a925756f9e184e86c343f144191a/thing.jar") + def cachedDir = testDir.file("cached/2a394a2c80ba9bd4e62f7bfe48f4367c/thing.dir.jar") + def cachedFile = testDir.file("cached/a0ff919f7115600becaeee69464d82cf/thing.jar") when: def cachedClasspath = transformer.transform(classpath, BuildLogic) @@ -372,7 +372,7 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec { def file = testDir.file("thing.jar") jar(file) def classpath = DefaultClassPath.of(file) - def cachedFile = testDir.file("cached/2b088b2faf4c2cbf2d23a43d06e2dea2/thing.jar") + def cachedFile = testDir.file("cached/f5f441108338e7d1bebb5460a1fce0d9/thing.jar") when: def cachedClasspath = transformer.transform(classpath, BuildLogic, transform)