From 81e1cb6c426953d2f014660ac863478a6710f514 Mon Sep 17 00:00:00 2001 From: Ivan Gavrilovic Date: Tue, 2 Jun 2020 23:08:49 +0100 Subject: [PATCH] Avoid using project.copy when possible and fix serialization (#406) Since Gradle 6.0 there is FileSystemOperations API which is compatible with configuration caching. This PR introduces CopyActionFacade which exposes the copy method, but implementation is using the new API whne possible, and reverts to project.copy when Gradle version is lower than 6.0. This PR also starts using Gradle 6.0 to build the plugin. It is needed in order to use FileSystemOperations API. In GenerateProtoTask, SourceSet is now not serialized as part of the task state, and a provider capturing what is needed is created instead. Integration test for the Java plugin is updated to Gradle 6.5-rc-1. --- .../protobuf/gradle/CopyActionFacade.java | 70 +++++++++++++++++++ .../protobuf/gradle/GenerateProtoTask.groovy | 40 +++++++---- .../protobuf/gradle/ProtobufExtract.groovy | 58 ++++++++++----- .../gradle/ProtobufJavaPluginTest.groovy | 13 ++-- 4 files changed, 142 insertions(+), 39 deletions(-) create mode 100644 src/main/groovy/com/google/protobuf/gradle/CopyActionFacade.java diff --git a/src/main/groovy/com/google/protobuf/gradle/CopyActionFacade.java b/src/main/groovy/com/google/protobuf/gradle/CopyActionFacade.java new file mode 100644 index 00000000..41e8e770 --- /dev/null +++ b/src/main/groovy/com/google/protobuf/gradle/CopyActionFacade.java @@ -0,0 +1,70 @@ +/* + * Original work copyright (c) 2015, Alex Antonov. All rights reserved. + * Modified work copyright (c) 2015, Google Inc. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this + * list of conditions and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors + * may be used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package com.google.protobuf.gradle; + +import org.gradle.api.Action; +import org.gradle.api.Project; +import org.gradle.api.file.CopySpec; +import org.gradle.api.file.FileSystemOperations; +import org.gradle.api.tasks.WorkResult; + +import javax.inject.Inject; + +/** + * Interface exposing the file copying feature. Actual implementations may use the + * {@link org.gradle.api.file.FileSystemOperations} if available (Gradle 6.0+) or {@link org.gradle.api.Project#copy} if + * the version of Gradle is below 6.0. + */ +interface CopyActionFacade { + WorkResult copy(Action var1); + + class ProjectBased implements CopyActionFacade { + private final Project project; + + public ProjectBased(Project project) { + this.project = project; + } + + @Override + public WorkResult copy(Action action) { + return project.copy(action); + } + } + + abstract class FileSystemOperationsBased implements CopyActionFacade { + @Inject + public abstract FileSystemOperations getFileSystemOperations(); + + @Override + public WorkResult copy(Action action) { + return getFileSystemOperations().copy(action); + } + } +} diff --git a/src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy b/src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy index 5f33061c..df6b541e 100644 --- a/src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy +++ b/src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy @@ -41,6 +41,9 @@ import org.gradle.api.file.FileCollection import org.gradle.api.file.SourceDirectorySet import org.gradle.api.internal.file.FileResolver import org.gradle.api.logging.LogLevel +import org.gradle.api.model.ObjectFactory +import org.gradle.api.provider.Provider +import org.gradle.api.provider.ProviderFactory import org.gradle.api.tasks.CacheableTask import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFiles @@ -56,6 +59,7 @@ import org.gradle.api.tasks.TaskAction import org.gradle.util.ConfigureUtil import javax.annotation.Nullable +import javax.inject.Inject /** * The task that compiles proto files into Java files. @@ -63,7 +67,7 @@ import javax.annotation.Nullable // TODO(zhangkun83): add per-plugin output dir reconfiguraiton. @CompileDynamic @CacheableTask -public class GenerateProtoTask extends DefaultTask { +public abstract class GenerateProtoTask extends DefaultTask { // Windows CreateProcess has command line limit of 32768: // https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx static final int WINDOWS_CMD_LENGTH_LIMIT = 32760 @@ -76,21 +80,28 @@ public class GenerateProtoTask extends DefaultTask { private final ConfigurableFileCollection includeDirs = project.files() // source files are proto files that will be compiled by protoc private final ConfigurableFileCollection sourceFiles = project.files() - private final NamedDomainObjectContainer builtins - private final NamedDomainObjectContainer plugins + private final NamedDomainObjectContainer builtins = objectFactory.domainObjectContainer(PluginOptions) + private final NamedDomainObjectContainer plugins = objectFactory.domainObjectContainer(PluginOptions) // These fields are set by the Protobuf plugin only when initializing the // task. Ideally they should be final fields, but Gradle task cannot have // constructor arguments. We use the initializing flag to prevent users from // accidentally modifying them. private String outputBaseDir - // Tags for selectors inside protobuf.generateProtoTasks - private SourceSet sourceSet + // Tags for selectors inside protobuf.generateProtoTasks; do not serialize with Gradle configuration caching + @SuppressWarnings("UnnecessaryTransientModifier") // It is not necessary for task to implement Serializable + transient private SourceSet sourceSet private Object variant private ImmutableList flavors private String buildType private boolean isTestVariant private FileResolver fileResolver + private final Provider isTestProvider = providerFactory.provider { + if (Utils.isAndroidProject(project)) { + return isTestVariant + } + return Utils.isTest(sourceSet.name) + } /** * If true, will set the protoc flag @@ -311,10 +322,11 @@ public class GenerateProtoTask extends DefaultTask { return descriptorSetOptions.path != null ? descriptorSetOptions.path : "${outputBaseDir}/descriptor_set.desc" } - public GenerateProtoTask() { - builtins = project.container(PluginOptions) - plugins = project.container(PluginOptions) - } + @Inject + abstract ProviderFactory getProviderFactory() + + @Inject + abstract ObjectFactory getObjectFactory() //=========================================================================== // Configuration methods @@ -384,10 +396,12 @@ public class GenerateProtoTask extends DefaultTask { */ @Input public boolean getIsTest() { - if (Utils.isAndroidProject(project)) { - return isTestVariant - } - return Utils.isTest(sourceSet.name) + return isTestProvider.get() + } + + @Internal("Already captured with getIsTest()") + Provider getIsTestProvider() { + return isTestProvider } /** diff --git a/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy b/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy index 25c948fd..79545206 100644 --- a/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy +++ b/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy @@ -33,25 +33,29 @@ import com.google.common.base.Preconditions import groovy.transform.CompileDynamic import org.gradle.api.DefaultTask import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.model.ObjectFactory import org.gradle.api.tasks.Input import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal import org.gradle.api.tasks.OutputDirectory import org.gradle.api.tasks.PathSensitive import org.gradle.api.tasks.PathSensitivity import org.gradle.api.tasks.TaskAction +import javax.inject.Inject /** * Extracts proto files from a dependency configuration. */ @CompileDynamic -class ProtobufExtract extends DefaultTask { +abstract class ProtobufExtract extends DefaultTask { /** * The directory for the extracted files. */ private File destDir private Boolean isTest = null - private final ConfigurableFileCollection inputFiles = project.files() + private final ConfigurableFileCollection inputFiles = objectFactory.fileCollection() + private final CopyActionFacade copyActionFacade = instantiateCopyActionFacade() public void setIsTest(boolean isTest) { this.isTest = isTest @@ -70,6 +74,14 @@ class ProtobufExtract extends DefaultTask { return inputFiles } + @Internal + CopyActionFacade getCopyActionFacade() { + return copyActionFacade + } + + @Inject + abstract ObjectFactory getObjectFactory() + @TaskAction void extract() { destDir.mkdir() @@ -77,46 +89,46 @@ class ProtobufExtract extends DefaultTask { inputFiles.each { file -> logger.debug "Extracting protos from ${file} to ${destDir}" if (file.isDirectory()) { - project.copy { - includeEmptyDirs(false) - from(file.path) { + copyActionFacade.copy { spec -> + spec.includeEmptyDirs = false + spec.from(file.path) { include '**/*.proto' } - into(destDir) + spec.into(destDir) } } else if (file.path.endsWith('.proto')) { if (!warningLogged) { warningLogged = true - project.logger.warn "proto file '${file.path}' directly specified in configuration. " + + 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." } - project.copy { - includeEmptyDirs(false) - from(file.path) - into(destDir) + copyActionFacade.copy { spec -> + spec.includeEmptyDirs = false + spec.from(file.path) + spec.into(destDir) } } else if (file.path.endsWith('.jar') || file.path.endsWith('.zip')) { - project.copy { - includeEmptyDirs(false) - from(project.zipTree(file.path)) { + copyActionFacade.copy { spec -> + spec.includeEmptyDirs = false + spec.from(project.zipTree(file.path)) { include '**/*.proto' } - into(destDir) + spec.into(destDir) } } else if (file.path.endsWith('.tar') || file.path.endsWith('.tar.gz') || file.path.endsWith('.tar.bz2') || file.path.endsWith('.tgz')) { - project.copy { - includeEmptyDirs(false) - from(project.tarTree(file.path)) { + copyActionFacade.copy { spec -> + spec.includeEmptyDirs = false + spec.from(project.tarTree(file.path)) { include '**/*.proto' } - into(destDir) + spec.into(destDir) } } else { logger.debug "Skipping unsupported file type (${file.path}); handles only jar, tar, tar.gz, tar.bz2 & tgz" @@ -134,4 +146,12 @@ class ProtobufExtract extends DefaultTask { protected File getDestDir() { return destDir } + + private CopyActionFacade instantiateCopyActionFacade() { + if (Utils.compareGradleVersion(project, "6.0") > 0) { + // Use object factory to instantiate as that will inject the necessary service. + return objectFactory.newInstance(CopyActionFacade.FileSystemOperationsBased) + } + return new CopyActionFacade.ProjectBased(project) + } } diff --git a/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy b/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy index 612f4bb5..cb59db14 100644 --- a/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy +++ b/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy @@ -16,7 +16,7 @@ import spock.lang.Unroll @CompileDynamic class ProtobufJavaPluginTest extends Specification { // Current supported version is Gradle 5+. - private static final List GRADLE_VERSIONS = ["5.6", "6.0", "6.1"] + private static final List GRADLE_VERSIONS = ["5.6", "6.0", "6.5-rc-1"] private static final List KOTLIN_VERSIONS = ["1.3.20", "1.3.30"] void "testApplying java and com.google.protobuf adds corresponding task to project"() { @@ -80,7 +80,7 @@ class ProtobufJavaPluginTest extends Specification { } @Unroll - void "testProject should be successfully executed (instant-execution) [gradle #gradleVersion]"() { + void "testProject should be successfully executed (configuration cache) [gradle #gradleVersion]"() { given: "project from testProject" File projectDir = ProtobufPluginTestHelper.projectBuilder('testProject') .copyDirs('testProjectBase', 'testProject') @@ -91,8 +91,7 @@ class ProtobufJavaPluginTest extends Specification { .withProjectDir(projectDir) .withArguments( 'build', '--stacktrace', - '-Dorg.gradle.unsafe.instant-execution=true', - '-Dorg.gradle.unsafe.instant-execution.fail-on-problems=true' + '--configuration-cache=warn' ) .withPluginClasspath() .withGradleVersion(gradleVersion) @@ -113,10 +112,10 @@ class ProtobufJavaPluginTest extends Specification { result = runner.build() then: "it reuses the task graph" - result.output.contains("Reusing instant execution cache") + result.output.contains("Reusing configuration cache") - and: "it succeeds" - result.task(":build").outcome == TaskOutcome.SUCCESS + and: "it is up to date" + result.task(":build").outcome == TaskOutcome.UP_TO_DATE ProtobufPluginTestHelper.verifyProjectDir(projectDir) where: