Modernize build and fix deprecated gradle API usages #1476

ZacSweers · 2019-08-06T04:41:21Z

This fixes a number of deprecated API usages found in my own project, and also brings the project up to speed with modern gradle and AGP versions

DSteve595 · 2019-08-06T09:56:57Z

gradle/dependencies.gradle

@@ -19,7 +19,7 @@ ext.androidConfig = [
 ]

 ext.dep = [
-    androidPlugin           : 'com.android.tools.build:gradle:3.3.0',
+    androidPlugin           : 'com.android.tools.build:gradle:3.4.1',


Will fix 👍

tasomaniac · 2019-08-06T18:58:34Z

apollo-gradle-plugin/src/main/groovy/com/apollographql/apollo/gradle/ApolloExtension.groovy

@@ -46,7 +46,7 @@ class ApolloExtension {
    outputPackageName = project.objects.property(String.class)
    outputPackageName.set("")

-    customTypeMapping = project.objects.property(Map.class)
+    customTypeMapping = project.objects.mapProperty(String.class, String.class)


I believe these were included in Gradle 4.x. Readme says 4.3 is required. Would be good to double check if 4.3 is fine or need to update the requirement in the Readme.

Can check, but also feel like only supporting current major version should be fine too. Gradle is already on 5.5, and these APIs are also not stable yet

My Sample changes is merged. If you update your branch then you will have the sample. Sample now uses a separate project with composite builds. We can quickly try with different set of Gradle versions there. By changing the wrapper on the sample.

ZacSweers · 2019-08-07T06:40:04Z

So the minimum supported gradle version for current AGP (3.4.2) is 5.1.1. I think that should settle it?

tasomaniac · 2019-08-07T16:49:54Z

Hmm, think this as minSdk vs compileSdk on Android. This plugin depends on AGP 3.4.2 but works fine with older versions. In fact it work with java/Kotlin only projects where there is no AGP dependency at all.

It's just that we need to figure out which versions we support at minimum and document that in README

ZacSweers · 2019-08-07T19:59:44Z

So I'm personally a big proponent of just supporting the current major version. For AGP that's 3.4.0, and for gradle that's 5.x. We can even actually easily set up tests for this on travis (example), but it's much harder to test this regularly if it requires lowering both AGP and gradle versions every time

tasomaniac · 2019-08-07T20:05:27Z

Gradle testing APIs support integration testing the plugin across different versions. Testing against different version of AGP is also possible. Of course that is out of scope here.

tasomaniac · 2019-08-07T20:24:50Z

Looks like there are some changes which affects the integration module

Failed to apply plugin [id 'com.apollographql.android']
Could not get unknown property 'SourceDirectorySet' for source set android test of type com.android.build.gradle.internal.api.DefaultAndroidSourceSet.

ZacSweers · 2019-08-07T22:04:20Z

Fixed the source directory set bit, just accidentally lost an import in a rebase.

As for testing gradle 4.3, even if I lower back down to AGP 3.3.0, the min gradle version is still 4.10.1. This does fail on one new source directory set API

3.2.1 requires gradle 4.6 (feb 2018). This also fails on the new source directory set API

4.3 was October 2017. Not sure

So in short - this would require gradle 5.x effectively. I think this is reasonable as it matches the current "major" AGP version as well

ZacSweers · 2019-08-07T22:20:11Z

Note the test failures appear unrelated, and I think just a change in how the gradle test kit works now. Will investigate

ZacSweers · 2019-08-08T02:32:39Z

I've partially addressed the new test requirements, but now I don't know what to do to fix all the tasks seeing themselves as up to date every test. I suspect it's something wrong with the incremental tasks logic in apollo itself

tasomaniac · 2019-08-08T05:47:52Z

In another Gradle plugin, we've had similar issues while trying to update to Gradle 5.x. Another problem was that it would use much more memory which results in ~5x slower CI builds. novoda/gradle-static-analysis-plugin#169

What about reverting the Gradle update. Or update to latest 4.10 but just fix depreciations. Then, that can be merged fast and then a PR updating Gradle can be open.

ZacSweers · 2019-08-08T05:51:23Z

I don't quite understand, as 4.10 is what we started with and per my last comment, some of these deprecation fixes require 5.x. Would prefer to fix here rather than backtrack

tasomaniac · 2019-08-08T16:13:21Z

I was just suggesting to take smaller steps so that this can be merged faster. Its just a suggestion. If it can be fixed here, that's better

Also requires putting AGP in classpath for android subprojects Per recommendation from the plugin itself: The Kotlin Gradle plugin was loaded multiple times in different subprojects, which is not supported and may break the build. This might happen in subprojects that apply the Kotlin plugins with the Gradle 'plugins { ... }' DSL if they specify explicit versions, even if the versions are equal. Please add the Kotlin plugin to the common parent project or the root project, then remove the versions in the subprojects. If the parent project does not need the plugin, add 'apply false' to the plugin line.

Using DefaultSourceDirectorySet directly is deprecated

Compiler doesn't like this, dunno why it passes master

ZacSweers · 2019-08-14T06:07:46Z

Finally figured out the issue and resolved with 57f829c. Should be good to go now 🤞

Would like to get resolution for the gradle versioning bit. I think supporting the latest gradle (5.x) and AGP (3.4.x, which requires gradle 5.1+) is a very sane baseline support strategy.

tasomaniac · 2019-08-14T12:39:19Z

...e-plugin/src/test/groovy/com/apollographql/apollo/gradle/unit/ApolloAndroidPluginSpec.groovy

@@ -73,8 +73,7 @@ class ApolloAndroidPluginSpec extends Specification {

    then:
    project.android.sourceSets.all { sourceSet ->
-      assert (sourceSet.extensions.findByName("graphql")) != null


Really small comment. I find having hard coded graphql in tests is better in terms of quality assurance.

Imagine a case where somebody changes GraphQLSourceDirectorySet.NAME parameter, that will be a breaking change. The previous version of these tests will catch it whereas the new one will just accept them.

tasomaniac

Looks good to me. Amazing maintenance work. Thanks @ZacSweers

tasomaniac · 2019-08-14T12:50:34Z

@ZacSweers can you also update the README mentioning the minimum Gradle requirement. I checked it with your branch, everything works as expected when a client uses Gradle 5.1.1 (which is also what is required by the latest stable Android plugin)

ZacSweers · 2019-08-15T05:50:02Z

@tasomaniac done and also opportunistically updated to gradle 5.6

sav007 · 2019-08-15T17:36:02Z

Is it ready to merge?

tasomaniac · 2019-08-15T17:36:37Z

I think so.

DSteve595 reviewed Aug 6, 2019

View reviewed changes

tasomaniac reviewed Aug 6, 2019

View reviewed changes

ZacSweers force-pushed the z/buildUpdates branch from 415ac34 to 5450e5c Compare August 6, 2019 23:27

ZacSweers force-pushed the z/buildUpdates branch from e92e4a3 to fe0dd4a Compare August 7, 2019 20:03

ZacSweers added 15 commits August 12, 2019 23:03

Gradle 5.5.1

e72f82f

Use proper mapProperty API

2f5751f

Use proper listProperty API

0d25949

Use conventional ObjectFactory.sourceDirectorySet()

fce7289

Using DefaultSourceDirectorySet directly is deprecated

Use conventional listProperty

c39140c

Use directoryProperty

60d4769

Fix deprecated property API usages in ApolloClassGenerationTask

9f401e5

AGP 3.4.2

17eb60e

Fix more usages and use proper Property types for collections

48604f1

Move args to separate lines to better deduce exception origin

8296209

Fix missing import

5338a44

Fix static imports

94cd0dc

Add missing settings.gradle to test runs

171a74c

Fix new deprecated API usages

36416a3

ZacSweers force-pushed the z/buildUpdates branch from 46e43ce to 36416a3 Compare August 13, 2019 03:06

ZacSweers added 5 commits August 13, 2019 18:59

Make outputDir not final

f3707d7

Compiler doesn't like this, dunno why it passes master

Minor cleanups to ApolloPluginTestHelper

2e4471c

Remove unused import

00f5f75

Fix source set name

57f829c

Add missing settings.gradle file to kotlinspec test

50d1f81

tasomaniac reviewed Aug 14, 2019

View reviewed changes

tasomaniac approved these changes Aug 14, 2019

View reviewed changes

sav007 approved these changes Aug 14, 2019

View reviewed changes

ZacSweers added 2 commits August 14, 2019 23:34

Update README

297738e

Gradle 5.6 final

b072073

Specify gradle 5.1.1

6e42944

sav007 merged commit 97ec9c5 into apollographql:master Aug 15, 2019

ZacSweers mentioned this pull request Aug 18, 2019

Fix deprecated mapproperty usage #1509

Merged

martinbonnin mentioned this pull request Aug 18, 2019

Gradle v5 deprecations #1271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize build and fix deprecated gradle API usages #1476

Modernize build and fix deprecated gradle API usages #1476

ZacSweers commented Aug 6, 2019 •

edited

Loading

DSteve595 Aug 6, 2019

ZacSweers Aug 6, 2019

tasomaniac Aug 6, 2019

ZacSweers Aug 6, 2019

tasomaniac Aug 6, 2019

ZacSweers commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

ZacSweers commented Aug 8, 2019

tasomaniac commented Aug 8, 2019

ZacSweers commented Aug 8, 2019

tasomaniac commented Aug 8, 2019

ZacSweers commented Aug 14, 2019

tasomaniac Aug 14, 2019

tasomaniac left a comment

tasomaniac commented Aug 14, 2019 •

edited

Loading

ZacSweers commented Aug 15, 2019

sav007 commented Aug 15, 2019

tasomaniac commented Aug 15, 2019

Modernize build and fix deprecated gradle API usages #1476

Modernize build and fix deprecated gradle API usages #1476

Conversation

ZacSweers commented Aug 6, 2019 • edited Loading

DSteve595 Aug 6, 2019

Choose a reason for hiding this comment

ZacSweers Aug 6, 2019

Choose a reason for hiding this comment

tasomaniac Aug 6, 2019

Choose a reason for hiding this comment

ZacSweers Aug 6, 2019

Choose a reason for hiding this comment

tasomaniac Aug 6, 2019

Choose a reason for hiding this comment

ZacSweers commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

tasomaniac commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

ZacSweers commented Aug 7, 2019

ZacSweers commented Aug 8, 2019

tasomaniac commented Aug 8, 2019

ZacSweers commented Aug 8, 2019

tasomaniac commented Aug 8, 2019

ZacSweers commented Aug 14, 2019

tasomaniac Aug 14, 2019

Choose a reason for hiding this comment

tasomaniac left a comment

Choose a reason for hiding this comment

tasomaniac commented Aug 14, 2019 • edited Loading

ZacSweers commented Aug 15, 2019

sav007 commented Aug 15, 2019

tasomaniac commented Aug 15, 2019

ZacSweers commented Aug 6, 2019 •

edited

Loading

tasomaniac commented Aug 14, 2019 •

edited

Loading