Fix support with Gradle 5.x/Android 2.5+; add Gradle 5.1 and drop Gra…

…dle 3.0 in tests. (google#287) - `gradle/wrapper/gradle-wrapper.properties`: the Protobuf plugin is now built with Gradle 5.1 - `ProtobufPlugin.groovy`: fixed a bug where GenerateProtoTask may "include" the wrong directory when running with Android plugin >= 2.5. - The bug: `addGenerateProtoTask()` always use the directory per *sourceSet* as input (original line 328), but with Android plugin >= 2.5, the extract task writes to the directory per *variant* (original line 272), thus they don't match. The GenerateProtoTask ends up using a nonexistent directory as an input. We didn't catch it in tests because the integration tests were only run with Android plugin up to 2.3. However, Gradle produced a warning about missing input directory, which becomes an error on Gradle 5.0 (google#253). Adding Gradle 5.1 and Android 3.1.0 to the test reproduces this bug. - The fix refactored the file and moved the assigning of inputs closer to where the extract task is created. This makes sure the input of the generate task matches the output of the extract task. - Updated tests - Added Gradle 5.1/Android 3.1.0 to tests, and removed Gradle 2.14.1 the oldest version on the tested list, raising the minimum supported version to Gradle 3.0. Removed code paths dedicated to Gradle <3.0. - Gradle 3.0/Android 2.2.0 is commented out from the list because of gradle/gradle#8158. I have verified this pair passes the tests if it's the only one in the list, presumably bypassing the Gradle issue.
jdneo · Jan 10, 2019 · 8d0b8db · 8d0b8db
1 parent 87b3cae
commit 8d0b8db
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 77 deletions.
diff --git a/build.gradle b/build.gradle
@@ -88,7 +88,7 @@ dependencies {
 
     // This kotlin version needs to be compatible with the 
     // android plugin being used in the test runtime.
-    testProjectRuntime "org.jetbrains.kotlin:kotlin-gradle-plugin:1.2.0"
+    testProjectRuntime "org.jetbrains.kotlin:kotlin-gradle-plugin:1.3.11"
 
     // Add android plugin to the test classpath, so that we can jump into class definitions,
     // read their sources, set break points, etc while debugging android unit tests.

diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties
@@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-5.0-bin.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-5.1-bin.zip
diff --git a/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy b/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy
@@ -77,6 +77,11 @@ class ProtobufPlugin implements Plugin<Project> {
     }
 
     void apply(final Project project) {
+      if (Utils.compareGradleVersion(project, "3.0") < 0) {
+        throw new GradleException(
+          "Gradle version is ${project.gradle.gradleVersion}. Minimum supported version is 3.0")
+      }
+
         this.project = project
         // At least one of the prerequisite plugins must by applied before this plugin can be applied, so
         // we will use the PluginManager.withPlugin() callback mechanism to delay applying this plugin until
@@ -224,8 +229,7 @@ class ProtobufPlugin implements Plugin<Project> {
       Task extractProtosTask = maybeAddExtractProtosTask(sourceSet.name)
       generateProtoTask.dependsOn(extractProtosTask)
 
-      Task extractIncludeProtosTask = maybeAddExtractIncludeProtosTask(sourceSet.name)
-      generateProtoTask.dependsOn(extractIncludeProtosTask)
+      setupExtractIncludeProtosTask(generateProtoTask, sourceSet.name)
 
       // Include source proto files in the compiled archive, so that proto files from
       // dependent projects can import them.
@@ -269,15 +273,11 @@ class ProtobufPlugin implements Plugin<Project> {
                   it.attribute(artifactType, "jar")
                 }
               }.files : null
-        Task extractIncludeProtosTask = maybeAddExtractIncludeProtosTask(
-            name, classPathConfig, testClassPathConfig)
-        generateProtoTask.dependsOn(extractIncludeProtosTask)
+        setupExtractIncludeProtosTask(generateProtoTask, name, classPathConfig, testClassPathConfig)
       } else {
         // For Android Gradle plugin < 2.5
         variant.sourceSets.each {
-          Task extractIncludeProtosTask =
-              maybeAddExtractIncludeProtosTask(it.name)
-          generateProtoTask.dependsOn(extractIncludeProtosTask)
+          setupExtractIncludeProtosTask(generateProtoTask, it.name)
         }
       }
 
@@ -304,7 +304,7 @@ class ProtobufPlugin implements Plugin<Project> {
         it.fileResolver = this.fileResolver
         sourceSets.each { sourceSet ->
           // Include sources
-          Utils.addFilesToTaskInputs(project, inputs, sourceSet.proto)
+          Utils.addFilesToTaskInputs(inputs, sourceSet.proto)
           ProtobufSourceDirectorySet protoSrcDirSet = sourceSet.proto
           protoSrcDirSet.srcDirs.each { srcDir ->
             include srcDir
@@ -315,19 +315,8 @@ class ProtobufPlugin implements Plugin<Project> {
               project.fileTree(getExtractedProtosDir(sourceSet.name)) {
                 include "**/*.proto"
               }
-          Utils.addFilesToTaskInputs(project, inputs, extractedProtoSources)
+          Utils.addFilesToTaskInputs(inputs, extractedProtoSources)
           include extractedProtoSources.dir
-
-          // Register extracted include protos
-          ConfigurableFileTree extractedIncludeProtoSources =
-              project.fileTree(getExtractedIncludeProtosDir(sourceSet.name)) {
-                include "**/*.proto"
-              }
-          // Register them as input, but not as "source".
-          // Inputs are checked in incremental builds, but only "source" files are compiled.
-          inputs.dir extractedIncludeProtoSources
-          // Add the extracted include dir to the --proto_path include paths.
-          include extractedIncludeProtoSources.dir
         }
       }
     }
@@ -356,50 +345,59 @@ class ProtobufPlugin implements Plugin<Project> {
     }
 
     /**
-     * Adds a task to extract protos from compile dependencies of a sourceSet,
-     * if there isn't one. Those are needed for imports in proto files, but
-     * they won't be compiled since they have already been compiled in their
-     * own projects or artifacts.
+     * Sets up a task to extract protos from compile dependencies of a sourceSet, Those are needed
+     * for imports in proto files, but they won't be compiled since they have already been compiled
+     * in their own projects or artifacts.
      *
      * <p>This task is per-sourceSet for both Java and per variant for Android.
      */
-    private Task maybeAddExtractIncludeProtosTask(
+    private void setupExtractIncludeProtosTask(
+        GenerateProtoTask generateProtoTask,
         String sourceSetOrVariantName,
         FileCollection compileClasspathConfiguration = null,
         FileCollection testedCompileClasspathConfiguration = null) {
       String extractIncludeProtosTaskName = 'extractInclude' +
           Utils.getSourceSetSubstringForTaskNames(sourceSetOrVariantName) + 'Proto'
-      Task existingTask = project.tasks.findByName(extractIncludeProtosTaskName)
-      if (existingTask != null) {
-        return existingTask
-      }
-      return project.tasks.create(extractIncludeProtosTaskName, ProtobufExtract) {
-        description = "Extracts proto files from compile dependencies for includes"
-        destDir = getExtractedIncludeProtosDir(sourceSetOrVariantName) as File
-        inputs.files (compileClasspathConfiguration
-          ?: project.configurations[Utils.getConfigName(sourceSetOrVariantName, 'compile')])
-
-        // TL; DR: Make protos in 'test' sourceSet able to import protos from the 'main' sourceSet.
-        // Sub-configurations, e.g., 'testCompile' that extends 'compile', don't depend on the
-        // their super configurations. As a result, 'testCompile' doesn't depend on 'compile' and
-        // it cannot get the proto files from 'main' sourceSet through the configuration. However,
-        if (Utils.isAndroidProject(project)) {
-          // TODO(zhangkun83): Android sourceSet doesn't have compileClasspath. If it did, we
-          // haven't figured out a way to put source protos in 'resources'. For now we use an ad-hoc
-          // solution that manually includes the source protos of 'main' and its dependencies.
-          if (Utils.isTest(sourceSetOrVariantName)) {
-            inputs.files getSourceSets()['main'].proto
-            inputs.files testedCompileClasspathConfiguration ?: project.configurations['compile']
+      Task task = project.tasks.findByName(extractIncludeProtosTaskName)
+      if (task == null) {
+        task = project.tasks.create(extractIncludeProtosTaskName, ProtobufExtract) {
+          description = "Extracts proto files from compile dependencies for includes"
+          destDir = getExtractedIncludeProtosDir(sourceSetOrVariantName) as File
+          inputs.files (compileClasspathConfiguration
+            ?: project.configurations[Utils.getConfigName(sourceSetOrVariantName, 'compile')])
+
+          // TL; DR: Make protos in 'test' sourceSet able to import protos from the 'main'
+          // sourceSet.  Sub-configurations, e.g., 'testCompile' that extends 'compile', don't
+          // depend on the their super configurations. As a result, 'testCompile' doesn't depend on
+          // 'compile' and it cannot get the proto files from 'main' sourceSet through the
+          // configuration. However,
+          if (Utils.isAndroidProject(project)) {
+            // TODO(zhangkun83): Android sourceSet doesn't have compileClasspath. If it did, we
+            // haven't figured out a way to put source protos in 'resources'. For now we use an
+            // ad-hoc solution that manually includes the source protos of 'main' and its
+            // dependencies.
+            if (Utils.isTest(sourceSetOrVariantName)) {
+              inputs.files getSourceSets()['main'].proto
+              inputs.files testedCompileClasspathConfiguration ?: project.configurations['compile']
+            }
+          } else {
+            // In Java projects, the compileClasspath of the 'test' sourceSet includes all the
+            // 'resources' of the output of 'main', in which the source protos are placed.  This is
+            // nicer than the ad-hoc solution that Android has, because it works for any extended
+            // configuration, not just 'testCompile'.
+            inputs.files getSourceSets()[sourceSetOrVariantName].compileClasspath
           }
-        } else {
-          // In Java projects, the compileClasspath of the 'test' sourceSet includes all the
-          // 'resources' of the output of 'main', in which the source protos are placed.
-          // This is nicer than the ad-hoc solution that Android has, because it works for any
-          // extended configuration, not just 'testCompile'.
-          inputs.files getSourceSets()[sourceSetOrVariantName].compileClasspath
+          isTest = Utils.isTest(sourceSetOrVariantName)
         }
-        isTest = Utils.isTest(sourceSetOrVariantName)
       }
+
+      generateProtoTask.dependsOn task
+
+      File extractedIncludeProtosDir = task.destDir
+      generateProtoTask.include extractedIncludeProtosDir
+      // Register the include dir as input, but not as "source".
+      // Inputs are checked in incremental builds, but only "source" files are compiled.
+      generateProtoTask.inputs.dir extractedIncludeProtosDir
     }
 
     private void linkGenerateProtoTasksToTaskName(String compileTaskName, GenerateProtoTask genProtoTask) {

diff --git a/src/main/groovy/com/google/protobuf/gradle/Utils.groovy b/src/main/groovy/com/google/protobuf/gradle/Utils.groovy
@@ -95,13 +95,8 @@ class Utils {
     return "compile" + GUtil.toCamelCase(variantName) + "Kotlin"
   }
 
-  static void addFilesToTaskInputs(Project project, TaskInputs inputs, Object files) {
-    if (compareGradleVersion(project, "3.0") >= 0) {
-      inputs.files(files).skipWhenEmpty()
-    } else {
-      // source() is deprecated since Gradle 3.0
-      inputs.source(files)
-    }
+  static void addFilesToTaskInputs(TaskInputs inputs, Object files) {
+    inputs.files(files).skipWhenEmpty()
   }
 
   /**

diff --git a/src/test/groovy/com/google/protobuf/gradle/AndroidProjectDetectionTest.groovy b/src/test/groovy/com/google/protobuf/gradle/AndroidProjectDetectionTest.groovy
@@ -10,8 +10,8 @@ import spock.lang.Specification
  * Verify android projects are identified correctly
  */
 class AndroidProjectDetectionTest extends Specification {
-  private static final List<String> GRADLE_VERSION = ["2.14.1", "3.0", "4.2", "4.3", "4.8"]
-  private static final List<String> ANDROID_PLUGIN_VERSION = ["2.2.0", "2.2.0", "2.3.0", "2.3.0", "3.1.2"]
+  private static final List<String> GRADLE_VERSION = [/* "3.0", */ "4.2", "4.3", "5.1"]
+  private static final List<String> ANDROID_PLUGIN_VERSION = [/* "2.2.0", */ "2.3.0", "2.3.0", "3.1.0"]
 
   static void appendUtilIsAndroidProjectCheckTask(File buildFile, boolean assertResult) {
     buildFile << """

diff --git a/src/test/groovy/com/google/protobuf/gradle/ProtobufAndroidPluginTest.groovy b/src/test/groovy/com/google/protobuf/gradle/ProtobufAndroidPluginTest.groovy
@@ -10,8 +10,9 @@ import spock.lang.Specification
  * Unit tests for android related functionality.
  */
 class ProtobufAndroidPluginTest extends Specification {
-  private static final List<String> GRADLE_VERSION = ["2.14.1", "3.0", "4.2", "4.3"]
-  private static final List<String> ANDROID_PLUGIN_VERSION = ["2.2.0", "2.2.0", "2.3.0", "2.3.0"]
+  // TODO(zhangkun83): restore 3.0/2.2.0 once https://github.com/gradle/gradle/issues/8158 is resolved
+  private static final List<String> GRADLE_VERSION = [/* "3.0", */ "4.2", "4.3", "5.1"]
+  private static final List<String> ANDROID_PLUGIN_VERSION = [/* "2.2.0", */ "2.3.0", "2.3.0", "3.1.0"]
 
   void "testProjectAndroid should be successfully executed (java only)"() {
     given: "project from testProject, testProjectLite & testProjectAndroid"

diff --git a/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy b/src/test/groovy/com/google/protobuf/gradle/ProtobufJavaPluginTest.groovy
@@ -12,7 +12,8 @@ import spock.lang.Specification
  * Unit tests for normal java and kotlin functionality.
  */
 class ProtobufJavaPluginTest extends Specification {
-  private static final List<String> GRADLE_VERSIONS = ["2.12", "3.0", "4.0", "4.3"]
+  // TODO(zhangkun83): restore 3.0 once https://github.com/gradle/gradle/issues/8158 is resolved
+  private static final List<String> GRADLE_VERSIONS = [/* "3.0",*/ "4.0", "4.3", "5.1"]
 
   void "testApplying java and com.google.protobuf adds corresponding task to project"() {
     given: "a basic project with java and com.google.protobuf"
@@ -81,7 +82,7 @@ class ProtobufJavaPluginTest extends Specification {
     when: "build is invoked"
     BuildResult result = GradleRunner.create()
       .withProjectDir(projectDir)
-      .withArguments('build')
+      .withArguments('build', '--stacktrace')
       .withDebug(true)
       .withGradleVersion(gradleVersion)
       .build()

diff --git a/testProjectAndroid/settings.gradle b/testProjectAndroid/settings.gradle
@@ -0,0 +1 @@
+rootProject.name = 'testProjectAndroid'
diff --git a/testProjectAndroidBase/build_base.gradle b/testProjectAndroidBase/build_base.gradle
@@ -1,7 +1,15 @@
 repositories {
+    maven { url 'https://maven.google.com' }
     maven { url "https://plugins.gradle.org/m2/" }
 }
 
+buildscript {
+  repositories {
+    maven { url 'https://maven.google.com' }
+    maven { url "https://plugins.gradle.org/m2/" }
+  }
+}
+
 android {
     compileSdkVersion 26
     buildToolsVersion "26.0.1"

diff --git a/testProjectCustomProtoDir/build.gradle b/testProjectCustomProtoDir/build.gradle
@@ -40,12 +40,14 @@ protobuf.protoc {
   artifact = 'com.google.protobuf:protoc:3.0.0'
 }
 
-task printDeps(dependsOn: build) << {
-  configurations.each { println it }
-  sourceSets.each { println it.getCompileTaskName("proto") }
-  sourceSets.each { println it.getCompileTaskName("java") }
-  sourceSets.each { println it }
-  println tasks['compileJava'].source
-  println project.buildDir
-  println project.buildDir.path
+task printDeps(dependsOn: build) {
+  doLast {
+    configurations.each { println it }
+    sourceSets.each { println it.getCompileTaskName("proto") }
+    sourceSets.each { println it.getCompileTaskName("java") }
+    sourceSets.each { println it }
+    println tasks['compileJava'].source
+    println project.buildDir
+    println project.buildDir.path
+  }
 }