Skip to content

Commit

Permalink
Avoid eagerly resolving input files in ProtobufExtract (#713) (#719)
Browse files Browse the repository at this point in the history 
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 <gavra0@users.noreply.github.com>
  • Loading branch information
@YifeiZhuang @gavra0
YifeiZhuang and gavra0 authored Jul 13, 2023
1 parent 30fa504 commit 4502c7e
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 35 deletions.
73 changes: 38 additions & 35 deletions src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<File> files = inputFiles.files
PatternSet protoFilter = new PatternSet().include("**/*.proto")
Set<Object> 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<Object> 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
})
}
}
43 changes: 43 additions & 0 deletions src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ class ProtobufJavaPluginTest extends Specification {
// Current supported version is Gradle 5+.
private static final List<String> GRADLE_VERSIONS = ["5.6", "6.0", "6.7.1", "7.4.2", "7.6"]
private static final List<String> 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<String> 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"
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 4502c7e

Please sign in to comment.