Refactor top-level build.gradle to avoid enabling unhelpful plugins #6131

ejona86 · 2019-09-07T19:31:23Z

This is a long patch series that looks intimidating. But it is split up into atomic comics that apply to one piece at a time. I spent effort to avoid changing existing behavior or to further simplify, so most large code movements have no differences within their body. The exception is cases where we had ifs checking which project we were processing; those were mostly removed in favor of triggering the behavior only if a project uses specific plugins.

This is in no way "complete." There's still plenty of other improvements available.

There were several objectives: 1) get the build file into stronger shape where related code is together, 2) allow more independence in projects (e.g., we shouldn't be forced to migrate all projects to java-library at once) and 3) allow Android to be part of our main build. Actually adding Android projects was not included (see #6132), since that is a riskier addition.

Parts of this may be more conservative than you may hope, like I did not update the examples to the new plugin syntax. That should not be seen as a problem; we can do further changes later.

When merging, I will leave the commits separate.

Examples and android projects were left unchanged. They can be changed later. No plugin versions were changed, to make this as non-functional of a change as possible. Upgrading Gradle to 5.6 was necessary for pluginManagement in settings.gradle.

dapengzhang0 · 2019-09-12T17:51:43Z

all/build.gradle

-buildscript {
-    repositories {
-        maven { // The google mirror is less flaky than mavenCentral()
-            url "https://maven-central.storage-download.googleapis.com/repos/central/data/" }


To keep the old behavior, you might add this in settings.gradle

pluginManagement { repositories { maven { // The google mirror is less flaky than mavenCentral() url "https://maven-central.storage-download.googleapis.com/repos/central/data/" } gradlePluginPortal() } }

We aren't using mavenCentral any more, so using the Google mirror isn't all that useful. But even more, it can't load the plugins from the google mirror, because the search will be based on the name com.github.kt3k.coveralls, not org.kt3k.gradle.plugin:coveralls-gradle-plugin. While there are ways to make that work, they require some effort and it doesn't seem all that helpful given most of the plugins are not available on the google mirror.

dapengzhang0 · 2019-09-12T17:59:45Z

gae-interop-testing/gae-jdk8/build.gradle

@@ -34,8 +40,6 @@ repositories {
    jcenter()
 }

-apply plugin: 'java'                              // standard Java tasks
-apply plugin: 'war'                               // standard Web Archive plugin
 apply plugin: 'com.google.cloud.tools.appengine'  // App Engine tasks


Any reason 'com.google.cloud.tools.appengine' is not moved to plugins {}?

It isn't on the plugins.gradle.org so the new plugin syntax doesn't work. I later figured out a way to solve that for Android, and I could apply it here. But I also didn't care too much about this file since it is already out-of-the-way and not causing trouble. Also also, I would still need the okhttp dependency in the buildscript {} section since it is used by the testing code below, so I can't clean up this file completely anyway.

dapengzhang0 · 2019-09-12T18:00:11Z

gae-interop-testing/gae-jdk8/build.gradle

@@ -26,6 +25,13 @@ buildscript {
    }
 }

+plugins {
+    id "java"


It seems all projects explicitly apply java plugin, why not apply it in rootDir/build.gradle?
It is for migration to java-library?

1 ) Android should not include Java, which is a follow-up PR and 2) make it easier to swap to java-library, since java-library will only apply to a subset of projects. "java" plugin has historically been the main problem, so it was the most important one to move into individual projects. Things like maven-publish were more icing on the cake.

dapengzhang0 · 2019-09-12T18:41:13Z

settings.gradle

@@ -1,3 +1,16 @@
+pluginManagement {
+    plugins {


This method (PluginManagementSpec.plugins) is incubating, why not just specify versions in build.gradle?

You can't use variable names in plugins {} blocks; there can't be any logic. So that would require having a plugins section in the root build.gradle with apply false for each. But I think that is still functionally different from this, in the way transitive dependencies will be loaded for each project. A bit uncertain. In any case, this seemed much nicer and since it is only in one location didn't seem too much worry if they change the API some in the future.

Also, I plan to move all of our library versions into this file, as Gradle allows specifying dependencies without a version and having this file choose the version to use. That would be much nicer than figuring out which variable to use. But this also relates to some pinning support I want to use, lock files, and more which I've not had time to integrate and verify that they don't regress our Maven Central pom.xml output.

dapengzhang0 · 2019-09-12T21:02:19Z

grpclb/build.gradle

@@ -7,6 +8,8 @@ plugins {

 description = "gRPC: GRPCLB LoadBalancer plugin"

+evaluationDependsOn(project(':grpc-core').path)


Why this change?

I had trouble with project(':grpc-core').sourceSets.test.output throwing, because the sourceSets weren't loaded yet because the java plugin wasn't loaded yet. Or some such. It was unfortunate, but the hack wasn't needed very many places and it wasn't at risk of forming a circular dependency.

dapengzhang0 · 2019-09-12T21:21:49Z

build.gradle

+            source = "8"
+        }
+
+        checkstyleMain {


Shouldn't this be in plugins.withId('checkstyle') {} ?

Yes and no. checkstyle is always enabled so there's no need to include it in a block. The other things I moved to plugins.withId() must be moved into a conditional section to allow applying the plugins optional.

dapengzhang0 · 2019-09-12T21:24:28Z

alts/build.gradle

@@ -1,4 +1,5 @@
 plugins {
+    id "java"
    id "maven-publish"


Why not move 'maven' plugin as well? If mavne-publish is a super set of maven plugin, we can remove maven plugin.

I'd just assume delete the maven plugin. I couldn't find anything that failed when I removed the maven plugin outright. That seemed suspicious to me, so I just left it in place for now to reduce the risk.

ejona86

Good eye for noticing all those parts you commented on.

ejona86 · 2019-09-12T21:35:30Z

all/build.gradle

-buildscript {
-    repositories {
-        maven { // The google mirror is less flaky than mavenCentral()
-            url "https://maven-central.storage-download.googleapis.com/repos/central/data/" }


We aren't using mavenCentral any more, so using the Google mirror isn't all that useful. But even more, it can't load the plugins from the google mirror, because the search will be based on the name com.github.kt3k.coveralls, not org.kt3k.gradle.plugin:coveralls-gradle-plugin. While there are ways to make that work, they require some effort and it doesn't seem all that helpful given most of the plugins are not available on the google mirror.

ejona86 · 2019-09-12T21:37:14Z

alts/build.gradle

@@ -1,4 +1,5 @@
 plugins {
+    id "java"
    id "maven-publish"


I'd just assume delete the maven plugin. I couldn't find anything that failed when I removed the maven plugin outright. That seemed suspicious to me, so I just left it in place for now to reduce the risk.

ejona86 · 2019-09-12T21:40:41Z

gae-interop-testing/gae-jdk8/build.gradle

@@ -26,6 +25,13 @@ buildscript {
    }
 }

+plugins {
+    id "java"


1 ) Android should not include Java, which is a follow-up PR and 2) make it easier to swap to java-library, since java-library will only apply to a subset of projects. "java" plugin has historically been the main problem, so it was the most important one to move into individual projects. Things like maven-publish were more icing on the cake.

ejona86 · 2019-09-12T21:42:32Z

gae-interop-testing/gae-jdk8/build.gradle

@@ -34,8 +40,6 @@ repositories {
    jcenter()
 }

-apply plugin: 'java'                              // standard Java tasks
-apply plugin: 'war'                               // standard Web Archive plugin
 apply plugin: 'com.google.cloud.tools.appengine'  // App Engine tasks


It isn't on the plugins.gradle.org so the new plugin syntax doesn't work. I later figured out a way to solve that for Android, and I could apply it here. But I also didn't care too much about this file since it is already out-of-the-way and not causing trouble. Also also, I would still need the okhttp dependency in the buildscript {} section since it is used by the testing code below, so I can't clean up this file completely anyway.

ejona86 · 2019-09-12T21:46:40Z

grpclb/build.gradle

@@ -7,6 +8,8 @@ plugins {

 description = "gRPC: GRPCLB LoadBalancer plugin"

+evaluationDependsOn(project(':grpc-core').path)


I had trouble with project(':grpc-core').sourceSets.test.output throwing, because the sourceSets weren't loaded yet because the java plugin wasn't loaded yet. Or some such. It was unfortunate, but the hack wasn't needed very many places and it wasn't at risk of forming a circular dependency.

ejona86 · 2019-09-12T21:51:59Z

settings.gradle

@@ -1,3 +1,16 @@
+pluginManagement {
+    plugins {


You can't use variable names in plugins {} blocks; there can't be any logic. So that would require having a plugins section in the root build.gradle with apply false for each. But I think that is still functionally different from this, in the way transitive dependencies will be loaded for each project. A bit uncertain. In any case, this seemed much nicer and since it is only in one location didn't seem too much worry if they change the API some in the future.

Also, I plan to move all of our library versions into this file, as Gradle allows specifying dependencies without a version and having this file choose the version to use. That would be much nicer than figuring out which variable to use. But this also relates to some pinning support I want to use, lock files, and more which I've not had time to integrate and verify that they don't regress our Maven Central pom.xml output.

ejona86 · 2019-09-12T21:54:34Z

build.gradle

+            source = "8"
+        }
+
+        checkstyleMain {


Yes and no. checkstyle is always enabled so there's no need to include it in a block. The other things I moved to plugins.withId() must be moved into a conditional section to allow applying the plugins optional.

dapengzhang0

LGTM. The answers to my concerns are acceptable to me. I reviewed one commit after another, as the PR is separated very well in atomic commits, the reviewing is not as intimidating as I/you would expect.

ejona86 added 7 commits September 7, 2019 08:18

Apply JMH plugin explicitly when needed

19fbaf6

Apply maven-publish plugin explicitly when needed

fb2301e

Move ALPN Agent configuration to each project needing it

af77556

Apply japicmp plugin explicitly when needed

b1bb108

Apply java plugin explicitly when needed

84d9109

Apply animalsniffer plugin explicitly when needed

ee79444

ejona86 mentioned this pull request Sep 7, 2019

Add Cronet to main build #6132

Merged

ejona86 requested a review from dapengzhang0 September 7, 2019 19:35

dapengzhang0 reviewed Sep 12, 2019

View reviewed changes

ejona86 commented Sep 12, 2019

View reviewed changes

dapengzhang0 approved these changes Sep 13, 2019

View reviewed changes

ejona86 merged commit 2b94577 into grpc:master Sep 13, 2019

ejona86 deleted the gradle-refactor branch September 13, 2019 16:42

ejona86 mentioned this pull request Sep 18, 2019

Android kokoro build is very flaky #6146

Closed

lock bot locked as resolved and limited conversation to collaborators Dec 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor top-level build.gradle to avoid enabling unhelpful plugins #6131

Refactor top-level build.gradle to avoid enabling unhelpful plugins #6131

ejona86 commented Sep 7, 2019 •

edited

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 left a comment

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

ejona86 Sep 12, 2019

dapengzhang0 left a comment •

edited

		@@ -7,6 +8,8 @@ plugins {

		description = "gRPC: GRPCLB LoadBalancer plugin"

		evaluationDependsOn(project(':grpc-core').path)

Refactor top-level build.gradle to avoid enabling unhelpful plugins #6131

Refactor top-level build.gradle to avoid enabling unhelpful plugins #6131

Conversation

ejona86 commented Sep 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapengzhang0 left a comment • edited

Choose a reason for hiding this comment

ejona86 commented Sep 7, 2019 •

edited

dapengzhang0 left a comment •

edited