From 4502c7eaec228520e757285e888668ae78c4e6dd Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Thu, 13 Jul 2023 11:57:15 -0700 Subject: [PATCH] Avoid eagerly resolving input files in ProtobufExtract (#713) (#719) Rely on FileCollection.getElements() (available in Gradle 5.6+) to ensure no files are accessed eagerly during configuration. This also ensures configuration cache key does not depend on them. Fixes issue #711. Test: Added "testProjectDependent proto extraction with configuration cache" with Gradle 8.1 Co-authored-by: Ivan Gavrilovic --- .../protobuf/gradle/ProtobufExtract.groovy | 73 ++++++++++--------- .../gradle/ProtobufJavaPluginTest.groovy | 43 +++++++++++ 2 files changed, 81 insertions(+), 35 deletions(-) diff --git a/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy b/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy index faafcae8..e27715e2 100644 --- a/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy +++ b/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy @@ -35,6 +35,7 @@ import org.gradle.api.file.ConfigurableFileCollection import org.gradle.api.file.DirectoryProperty import org.gradle.api.file.DuplicatesStrategy import org.gradle.api.file.FileCollection +import org.gradle.api.file.FileSystemLocation import org.gradle.api.file.FileTree import org.gradle.api.logging.Logger import org.gradle.api.model.ObjectFactory @@ -113,43 +114,45 @@ abstract class ProtobufExtract extends DefaultTask { // Provider.map seems broken for excluded tasks. Add inputFiles with all contents excluded for // the dependency it provides, but then provide the files we actually care about in our own // provider. https://github.com/google/protobuf-gradle-plugin/issues/550 + PatternSet protoFilter = new PatternSet().include("**/*.proto") return objectFactory.fileCollection() .from(inputFiles.filter { false }) - .from(providerFactory.provider { unused -> - Set files = inputFiles.files - PatternSet protoFilter = new PatternSet().include("**/*.proto") - Set protoInputs = [] as Set - for (File file : files) { - if (file.isDirectory()) { - protoInputs.add(file) - } else if (file.path.endsWith('.proto')) { - if (!warningLogged) { - warningLogged = true - logger.warn "proto file '${file.path}' directly specified in configuration. " + - "It's likely you specified files('path/to/foo.proto') or " + - "fileTree('path/to/directory') in protobuf or compile configuration. " + - "This makes you vulnerable to " + - "https://github.com/google/protobuf-gradle-plugin/issues/248. " + - "Please use files('path/to/directory') instead." - } - protoInputs.add(file) - } else if (file.path.endsWith('.jar') || file.path.endsWith('.zip')) { - protoInputs.add(archiveFacade.zipTree(file.path).matching(protoFilter)) - } else if (file.path.endsWith('.aar')) { - FileCollection zipTree = archiveFacade.zipTree(file.path).filter { File it -> it.path.endsWith('.jar') } - zipTree.each { entry -> - protoInputs.add(archiveFacade.zipTree(entry).matching(protoFilter)) - } - } else if (file.path.endsWith('.tar') - || file.path.endsWith('.tar.gz') - || file.path.endsWith('.tar.bz2') - || file.path.endsWith('.tgz')) { - protoInputs.add(archiveFacade.tarTree(file.path).matching(protoFilter)) - } else { - logger.debug "Skipping unsupported file type (${file.path}); handles only jar, tar, tar.gz, tar.bz2 & tgz" - } - } - return protoInputs + .from( + inputFiles.getElements().map { elements -> + Set protoInputs = [] as Set + for (FileSystemLocation e : elements) { + File file = e.asFile + if (file.isDirectory()) { + protoInputs.add(file) + } else if (file.path.endsWith('.proto')) { + if (!warningLogged) { + warningLogged = true + logger.warn "proto file '${file.path}' directly specified in configuration. " + + "It's likely you specified files('path/to/foo.proto') or " + + "fileTree('path/to/directory') in protobuf or compile configuration. " + + "This makes you vulnerable to " + + "https://github.com/google/protobuf-gradle-plugin/issues/248. " + + "Please use files('path/to/directory') instead." + } + protoInputs.add(file) + } else if (file.path.endsWith('.jar') || file.path.endsWith('.zip')) { + protoInputs.add(archiveFacade.zipTree(file.path).matching(protoFilter)) + } else if (file.path.endsWith('.aar')) { + FileCollection zipTree = archiveFacade.zipTree(file.path).filter { File it -> it.path.endsWith('.jar') } + zipTree.each { entry -> + protoInputs.add(archiveFacade.zipTree(entry).matching(protoFilter)) + } + } else if (file.path.endsWith('.tar') + || file.path.endsWith('.tar.gz') + || file.path.endsWith('.tar.bz2') + || file.path.endsWith('.tgz')) { + protoInputs.add(archiveFacade.tarTree(file.path).matching(protoFilter)) + } else { + logger.debug "Skipping unsupported file type (${file.path});" + + "handles only jar, tar, tar.gz, tar.bz2 & tgz" + } + } + return protoInputs }) } } diff --git a/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy b/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy index ce58fabc..14fc1ea2 100644 --- a/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy +++ b/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy @@ -17,6 +17,8 @@ class ProtobufJavaPluginTest extends Specification { // Current supported version is Gradle 5+. private static final List GRADLE_VERSIONS = ["5.6", "6.0", "6.7.1", "7.4.2", "7.6"] private static final List KOTLIN_VERSIONS = ["1.3.72", "1.4.32", "1.5.32", "1.6.21", "1.7.21"] + // Currently this is separate as some test projects are incompatible with Gradle 8.1 + private static final List GRADLE_WITH_FILE_SYSTEM_SNAPSHOTTING_FOR_CC = ["8.1"] void "testApplying java and com.google.protobuf adds corresponding task to project"() { given: "a basic project with java and com.google.protobuf" @@ -285,6 +287,47 @@ class ProtobufJavaPluginTest extends Specification { gradleVersion << GRADLE_VERSIONS } + @Unroll + void "testProjectDependent proto extraction with configuration cache [gradle #gradleVersion]"() { + given: "project from testProject & testProjectDependent" + File testProjectStaging = ProtobufPluginTestHelper.projectBuilder('testProject') + .copyDirs('testProjectBase', 'testProject') + .build() + File testProjectDependentStaging = ProtobufPluginTestHelper.projectBuilder('testProjectDependent') + .copyDirs('testProjectDependent') + .build() + + File mainProjectDir = ProtobufPluginTestHelper.projectBuilder('testProjectDependentMain') + .copySubProjects(testProjectStaging, testProjectDependentStaging) + .build() + + when: "extractIncludeProto is invoked" + BuildResult result = ProtobufPluginTestHelper.getGradleRunner( + mainProjectDir, + gradleVersion, + ":testProjectDependent:extractIncludeProto", + "--configuration-cache", + ).build() + + then: "it succeed" + result.task(":testProjectDependent:extractIncludeProto").outcome == TaskOutcome.SUCCESS + + when: "dependency sources are modified and extractIncludeProto is invoked again" + new File(testProjectStaging, "src/main/java/Bar.java").write("public class Bar {}") + result = ProtobufPluginTestHelper.getGradleRunner( + mainProjectDir, + gradleVersion, + ":testProjectDependent:extractIncludeProto", + "--configuration-cache", + ).build() + + then: "there is a configuration cache hit" + result.output.contains("Reusing configuration cache") + + where: + gradleVersion << GRADLE_WITH_FILE_SYSTEM_SNAPSHOTTING_FOR_CC + } + @Unroll void "testProjectJavaLibrary should be successfully executed (java-only as a library) [gradle #gradleVersion]"() { given: "project from testProjectJavaLibrary"