From a4b8590d56e9a5f82037a0b963c5e1ad7d06cd38 Mon Sep 17 00:00:00 2001 From: Gary Hale Date: Sat, 13 Feb 2021 11:12:04 -0500 Subject: [PATCH] Fixes issue with file hashing cache Normalization rules were not being properly captured when calculating the cache key for file hashes during runtime classpath normalization. This was causing incorrect build cache misses under the right sequence of events. Fixes #16144 --- ...asspathNormalizationIntegrationTest.groovy | 118 ++++++++++++++++-- .../state/IgnoringResourceHasher.java | 2 + .../MetaInfAwareClasspathResourceHasher.java | 1 + ...rtiesFileAwareClasspathResourceHasher.java | 2 + .../state/UnionResourceEntryFilter.java | 1 + .../state/IgnoringResourceHasherTest.groovy | 11 ++ ...InfAwareClasspathResourceHasherTest.groovy | 13 ++ ...ileAwareClasspathResourceHasherTest.groovy | 12 ++ 8 files changed, 150 insertions(+), 10 deletions(-) diff --git a/subprojects/core/src/integTest/groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy b/subprojects/core/src/integTest/groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy index 4487faf8420b..83f44d7b07df 100644 --- a/subprojects/core/src/integTest/groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy +++ b/subprojects/core/src/integTest/groovy/org/gradle/normalization/ConfigureRuntimeClasspathNormalizationIntegrationTest.groovy @@ -20,8 +20,13 @@ import org.gradle.integtests.fixtures.AbstractIntegrationSpec import org.gradle.integtests.fixtures.ToBeFixedForConfigurationCache import org.gradle.integtests.fixtures.UnsupportedWithConfigurationCache import org.gradle.test.fixtures.file.TestFile +import org.gradle.util.TextUtil +import spock.lang.Issue import spock.lang.Unroll +import java.util.jar.Attributes +import java.util.jar.Manifest + @Unroll class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractIntegrationSpec { @@ -80,6 +85,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte 'nested in dir jars' | 'ignoredResourceInNestedInDirJar' | 'notIgnoredResourceInNestedInDirJar' | Api.ANNOTATION } + @ToBeFixedForConfigurationCache(because = "classpath normalization") @Unroll def "can ignore manifest attributes in #tree on runtime classpath"() { def project = new ProjectWithRuntimeClasspathNormalization(Api.RUNTIME).withManifestAttributesIgnored() @@ -96,7 +102,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte skipped(project.customTask) when: - manifestResource.replaceContents("Manifest-Version: 1.0\nImplementation-Version: 1.0.1") + manifestResource.changeAttributes((IMPLEMENTATION_VERSION): "1.0.1") succeeds project.customTask then: skipped(project.customTask) @@ -124,7 +130,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte skipped(project.customTask) when: - manifestResource.replaceContents("Manifest-Version: 1.0\nImplementation-Version: 1.0.1") + manifestResource.changeAttributes((IMPLEMENTATION_VERSION): "1.0.1") succeeds project.customTask then: skipped(project.customTask) @@ -152,7 +158,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte skipped(project.customTask) when: - manifestResource.replaceContents("Manifest-Version: 1.0\nImplementation-Version: 1.0.1") + manifestResource.changeAttributes((IMPLEMENTATION_VERSION): "1.0.1") project.jarManifestProperties.replaceContents("implementation-version=1.0.1") succeeds project.customTask then: @@ -235,8 +241,8 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte change | config | api 'ignore file' | "ignore '**/some-other-file.txt'" | Api.RUNTIME 'ignore file' | "ignore '**/some-other-file.txt'" | Api.ANNOTATION - 'ignore manifest attribute' | "metaInf { ignoreAttribute 'Implementation-version' }" | Api.RUNTIME - 'ignore manifest attribute' | "metaInf { ignoreAttribute 'Implementation-version' }" | Api.ANNOTATION + 'ignore manifest attribute' | "metaInf { ignoreAttribute '${IMPLEMENTATION_VERSION}' }" | Api.RUNTIME + 'ignore manifest attribute' | "metaInf { ignoreAttribute '${IMPLEMENTATION_VERSION}' }" | Api.ANNOTATION 'ignore property' | "properties { ignoreProperty 'timestamp' }" | Api.RUNTIME 'ignore property' | "properties { ignoreProperty 'timestamp' }" | Api.ANNOTATION } @@ -466,9 +472,48 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte executedAndNotSkipped(project.customTask) } + @ToBeFixedForConfigurationCache(because = "classpath normalization") + @Issue('https://github.com/gradle/gradle/issues/16144') + def "changing normalization configuration rules changes build cache key (#description)"() { + def project = new ProjectWithRuntimeClasspathNormalization(Api.RUNTIME) + project.propertiesFileInJar.changeProperty(ALSO_IGNORE_ME, 'some value') + project.buildFile << """ + normalization { + runtimeClasspath { + if (project.hasProperty('${enableFilterFlag}')) { + ${normalizationRule} + } + } + } + """ + + when: + args('--build-cache') + succeeds 'clean', project.customTask + then: + executedAndNotSkipped(project.customTask) + + when: + args("-P${enableFilterFlag}", '--build-cache') + succeeds 'clean', project.customTask + then: + executedAndNotSkipped(project.customTask) + + where: + enableFilterFlag | normalizationRule | description + PROPERTIES_FILTER_FLAG | "properties('**/foo.properties') { ignoreProperty '${ALSO_IGNORE_ME}' }" | 'properties rule' + META_INF_FILTER_FLAG | "metaInf { ignoreAttribute '${IMPLEMENTATION_VERSION}' }" | 'meta-inf rule' + FILE_FILTER_FLAG | "ignore '**/ignored.txt'" | 'ignore rule' + } + static final String IGNORE_ME = 'ignore-me' + static final String ALSO_IGNORE_ME = 'also-ignore-me' static final String IGNORE_ME_TOO = 'ignore-me-too' static final String DONT_IGNORE_ME = 'dont-ignore-me' + static final String IMPLEMENTATION_VERSION = Attributes.Name.IMPLEMENTATION_VERSION.toString() + static final String PROPERTIES_FILTER_FLAG = "filterProperties" + static final String META_INF_FILTER_FLAG = "filterMetaInf" + static final String FILE_FILTER_FLAG = "filterFile" enum Api { RUNTIME, ANNOTATION @@ -476,6 +521,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte class ProjectWithRuntimeClasspathNormalization { final TestFile root + final TestFile buildCacheDir TestResource ignoredResourceInDirectory TestResource notIgnoredResourceInDirectory TestResource ignoredResourceInJar @@ -484,9 +530,9 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte TestResource notIgnoredResourceInJar TestResource notIgnoredResourceInNestedJar TestResource notIgnoredResourceInNestedInDirJar - TestResource jarManifest + ManifestResource jarManifest TestResource jarManifestProperties - TestResource manifestInDirectory + ManifestResource manifestInDirectory PropertiesResource propertiesFileInDir PropertiesResource propertiesFileInJar PropertiesResource propertiesFileInNestedJar @@ -499,10 +545,22 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte private TestFile nestedInDirJarContents private final String projectName final TestFile buildFile + final TestFile settingsFile ProjectWithRuntimeClasspathNormalization(String projectName = null, Api api) { this.projectName = projectName this.root = projectName ? file(projectName) : temporaryFolder.testDirectory + this.buildCacheDir = testDirectory.file("build-cache") + + def buildCachePath = TextUtil.normaliseFileSeparators(buildCacheDir.absolutePath) + + settingsFile = root.file('settings.gradle') << """ + buildCache { + local { + directory = file('${buildCachePath}') + } + } + """ buildFile = root.file('build.gradle') << """ apply plugin: 'base' @@ -520,7 +578,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte notIgnoredResourceInDirectory = new TestResource(file("not-ignored.txt") << "This should not be ignored") nestedInDirJar = file('nestedInDir.jar') propertiesFileInDir = new PropertiesResource(file('some/path/to/foo.properties'), [(IGNORE_ME): 'this should be ignored', (DONT_IGNORE_ME): 'this should not be ignored']) - manifestInDirectory = new TestResource(file('META-INF/MANIFEST.MF') << "Manifest-Version: 1.0\nImplementation-Version: 1.0.0") + manifestInDirectory = new ManifestResource(file('META-INF/MANIFEST.MF')).withAttributes((IMPLEMENTATION_VERSION): "1.0.0") } nestedJarContents = root.file('libraryContents').create { ignoredResourceInNestedJar = new TestResource(file('some/package/ignored.txt') << "This should be ignored", this.&createJar) @@ -528,7 +586,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte propertiesFileInNestedJar = new PropertiesResource(file('some/path/to/foo.properties'), [(IGNORE_ME): 'this should be ignored', (DONT_IGNORE_ME): 'this should not be ignored'], this.&createJar) } libraryJarContents = root.file('libraryContents').create { - jarManifest = new TestResource(file('META-INF/MANIFEST.MF') << "Manifest-Version: 1.0\nImplementation-Version: 1.0.0", this.&createJar) + jarManifest = new ManifestResource(file('META-INF/MANIFEST.MF'), this.&createJar).withAttributes((IMPLEMENTATION_VERSION): "1.0.0") jarManifestProperties = new TestResource(file('META-INF/build-info.properties') << "implementation-version=1.0.0", this.&createJar) ignoredResourceInJar = new TestResource(file('some/package/ignored.txt') << "This should be ignored", this.&createJar) notIgnoredResourceInJar = new TestResource(file('some/package/not-ignored.txt') << "This should not be ignored", this.&createJar) @@ -550,6 +608,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte .withNormalizer(ClasspathNormalizer) outputs.file(outputFile) .withPropertyName("outputFile") + outputs.cacheIf { true } doLast { outputFile.text = "done" @@ -558,6 +617,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte """ } else { return """ + @CacheableTask class CustomTask extends DefaultTask { @OutputFile File outputFile = new File(temporaryDir, "output.txt") @Classpath FileCollection classpath = project.layout.files("classpath/dirEntry", "library.jar") @@ -632,7 +692,7 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte normalization { runtimeClasspath { metaInf { - ignoreAttribute "Implementation-Version" + ignoreAttribute "${IMPLEMENTATION_VERSION}" } } } @@ -707,6 +767,44 @@ class ConfigureRuntimeClasspathNormalizationIntegrationTest extends AbstractInte } } + class ManifestResource extends TestResource { + Map attributes + + ManifestResource(TestFile backingFile, Closure onChange = {}) { + super(backingFile, onChange) + } + + ManifestResource withAttributes(Map attributes) { + this.attributes = attributes + def manifest = new Manifest() + def mainAttributes = manifest.getMainAttributes() + mainAttributes.put(Attributes.Name.MANIFEST_VERSION, "1.0") + attributes.each {name, value -> + mainAttributes.put(new Attributes.Name(name), value) + } + backingFile.withOutputStream {os -> + manifest.write(os) + } + return this + } + + ManifestResource changeAttributes(Map attributes) { + withAttributes(attributes) + changed() + return this + } + + @Override + void changeContents() { + throw new UnsupportedOperationException() + } + + @Override + void add() { + throw new UnsupportedOperationException() + } + } + class PropertiesResource extends TestResource { String comment = "" diff --git a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/IgnoringResourceHasher.java b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/IgnoringResourceHasher.java index 118437915de6..79ddc324402e 100644 --- a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/IgnoringResourceHasher.java +++ b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/IgnoringResourceHasher.java @@ -33,6 +33,8 @@ public IgnoringResourceHasher(ResourceHasher delegate, ResourceFilter resourceFi @Override public void appendConfigurationToHasher(Hasher hasher) { + delegate.appendConfigurationToHasher(hasher); + hasher.putString(getClass().getName()); resourceFilter.appendConfigurationToHasher(hasher); } diff --git a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasher.java b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasher.java index a27b3a9e76c0..4f3c2c1cc1d3 100644 --- a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasher.java +++ b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasher.java @@ -52,6 +52,7 @@ public MetaInfAwareClasspathResourceHasher(ResourceHasher delegate, ResourceEntr @Override public void appendConfigurationToHasher(Hasher hasher) { delegate.appendConfigurationToHasher(hasher); + hasher.putString(getClass().getName()); attributeResourceFilter.appendConfigurationToHasher(hasher); } diff --git a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasher.java b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasher.java index b5927139f092..3f9edec5c1c7 100644 --- a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasher.java +++ b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasher.java @@ -58,6 +58,8 @@ public PropertiesFileAwareClasspathResourceHasher(ResourceHasher delegate, Map resourceEntryFilter.appendConfigurationToHasher(hasher)); } diff --git a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/UnionResourceEntryFilter.java b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/UnionResourceEntryFilter.java index af01e808f83f..519c38300ceb 100644 --- a/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/UnionResourceEntryFilter.java +++ b/subprojects/normalization-java/src/main/java/org/gradle/api/internal/changedetection/state/UnionResourceEntryFilter.java @@ -37,6 +37,7 @@ public boolean shouldBeIgnored(String entry) { @Override public void appendConfigurationToHasher(Hasher hasher) { + hasher.putString(getClass().getName()); filters.forEach(resourceEntryFilter -> resourceEntryFilter.appendConfigurationToHasher(hasher)); } } diff --git a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/IgnoringResourceHasherTest.groovy b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/IgnoringResourceHasherTest.groovy index e90d6e324475..9a656334702a 100644 --- a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/IgnoringResourceHasherTest.groovy +++ b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/IgnoringResourceHasherTest.groovy @@ -19,6 +19,7 @@ package org.gradle.api.internal.changedetection.state import org.gradle.internal.file.FileMetadata import org.gradle.internal.file.impl.DefaultFileMetadata import org.gradle.internal.hash.HashCode +import org.gradle.internal.hash.Hasher import org.gradle.internal.snapshot.RegularFileSnapshot import spock.lang.Specification @@ -50,4 +51,14 @@ class IgnoringResourceHasherTest extends Specification { and: hash == null } + + def "delegate configuration is added to hasher"() { + def configurationHasher = Mock(Hasher) + + when: + hasher.appendConfigurationToHasher(configurationHasher) + + then: + 1 * delegate.appendConfigurationToHasher(configurationHasher) + } } diff --git a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasherTest.groovy b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasherTest.groovy index 53fdfae14768..1cbd5393ce6e 100644 --- a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasherTest.groovy +++ b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/MetaInfAwareClasspathResourceHasherTest.groovy @@ -21,6 +21,7 @@ import org.gradle.api.internal.file.archive.ZipEntry import org.gradle.internal.file.FileMetadata import org.gradle.internal.file.impl.DefaultFileMetadata import org.gradle.internal.hash.HashCode +import org.gradle.internal.hash.Hasher import org.gradle.internal.snapshot.RegularFileSnapshot import org.junit.Rule import org.junit.rules.TemporaryFolder @@ -389,6 +390,18 @@ class MetaInfAwareClasspathResourceHasherTest extends Specification { hash3 == hash4 } + def "delegate configuration is added to hasher"() { + def configurationHasher = Mock(Hasher) + def delegate = Mock(ResourceHasher) + useDelegate(delegate) + + when: + hasher.appendConfigurationToHasher(configurationHasher) + + then: + 1 * delegate.appendConfigurationToHasher(configurationHasher) + } + void populateAttributes(Attributes attributes, Map attributesMap) { attributesMap.each { String name, Object value -> if (value instanceof String) { diff --git a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasherTest.groovy b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasherTest.groovy index 12014152fabf..99b7e08caf13 100644 --- a/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasherTest.groovy +++ b/subprojects/normalization-java/src/test/groovy/org/gradle/api/internal/changedetection/state/PropertiesFileAwareClasspathResourceHasherTest.groovy @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet import com.google.common.collect.Maps import com.google.common.io.ByteStreams import org.gradle.api.internal.file.archive.ZipEntry +import org.gradle.internal.hash.Hasher import org.gradle.internal.hash.Hashing import org.gradle.internal.hash.HashingOutputStream import org.gradle.internal.snapshot.RegularFileSnapshot @@ -257,6 +258,17 @@ class PropertiesFileAwareClasspathResourceHasherTest extends Specification { SnapshotContext.FILE_SNAPSHOT | "key" | 'keyWithBadEscapeSequence\\uxxxx=some value' } + def "delegate configuration is added to hasher"() { + def configurationHasher = Mock(Hasher) + delegate = Mock(ResourceHasher) + + when: + filteredHasher.appendConfigurationToHasher(configurationHasher) + + then: + 1 * delegate.appendConfigurationToHasher(configurationHasher) + } + enum SnapshotContext { ZIP_ENTRY, FILE_SNAPSHOT }