Gradle updates #507

dpogue · 2018-09-26T07:39:19Z

Platforms affected

Android

What does this PR do?

Updates to latest version of the Android gradle plugin (3.2.0)
Updates gradle to the minimum version required by that plugin (4.6)
Fixes a gradle warning about LeftShift Closure being deprecated
Fixes a gradle warning about publishNonDefault being deprecated
Uses the google() repo syntax to reference the Google maven repository
Updates the Github/project URLs for maven artifacts
Fixes a handful of deprecation warnings happening inside the CordovaLib classes

In theory, this should fix the NDK issues reported in #504 due to outdated gradle versions.

What testing has been done on this change?

Nothing yet 🙃

Checklist

Reported an issue: NDK 18 toolchains #504
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

raphinesse · 2018-09-26T14:17:52Z

appveyor.yml

@@ -5,7 +5,7 @@ environment:
    GRADLE_OPTS: -Dorg.gradle.daemon=false

    # URL for SDK Tools, Revision 26.1.1 (September 2017)


I guess the version here is no longer correct with this change

Supposedly 26.1.1 is still the latest, but the download link doesn't match what was here before 😕

raphinesse · 2018-09-26T14:20:31Z

appveyor.yml


-    - choco install gradle --version 3.4.1
+    - choco install gradle --version 4.6.0


Actually, this should install the minimum version of Gradle needed to generate the wrapper with our actually required version. At least that was true before this change.

codecov-io · 2018-09-26T17:07:31Z

Codecov Report

Merging #507 into master will decrease coverage by 0.24%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #507      +/-   ##
==========================================
- Coverage   62.11%   61.87%   -0.25%     
==========================================
  Files          17       17              
  Lines        1985     1975      -10     
  Branches      371      366       -5     
==========================================
- Hits         1233     1222      -11     
- Misses        752      753       +1

Impacted Files	Coverage Δ
...n/templates/cordova/lib/builders/ProjectBuilder.js	`44.55% <0%> (ø)`	⬆️
bin/templates/cordova/lib/pluginHandlers.js	`85.71% <0%> (-1.43%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1396c7...47bb518. Read the comment docs.

oliversalzburg · 2018-09-26T20:18:14Z

I think you missed to replace one location with google(): https://github.com/apache/cordova-android/blob/master/bin/templates/cordova/lib/plugin-build.gradle#L23

Apps builds, deploys and runs fine with that piece adjusted.

oliversalzburg · 2018-09-26T20:36:26Z

I'm also seeing this in the sync log:

Configuration 'compile' in project ':app' is deprecated. Use 'implementation' instead.

https://stackoverflow.com/questions/47300679/configuration-compile-in-is-deprecated-but-all-configurations-are-implementa says to upgrade gradle to 3.2.0, which resolves the warning for me.

oliversalzburg · 2018-09-26T20:39:57Z

I'm also seeing

publishNonDefault is deprecated and has no effect anymore. All variants are now published.

Removing https://github.com/apache/cordova-android/blob/master/bin/templates/cordova/lib/plugin-build.gradle#L49 resolves the warning.

oliversalzburg · 2018-09-26T20:51:46Z

The SDK version should be moved from https://github.com/apache/cordova-android/blob/master/bin/templates/project/AndroidManifest.xml#L48 to https://github.com/apache/cordova-android/blob/master/bin/templates/project/app/build.gradle#L165

dpogue · 2018-11-01T04:19:17Z

The SDK version should be moved from https://github.com/apache/cordova-android/blob/master/bin/templates/project/AndroidManifest.xml#L48 to https://github.com/apache/cordova-android/blob/master/bin/templates/project/app/build.gradle#L165

See #508 for additional discussion around that point.
Short answer is that you're right, longer answer is that it's complicated.

erisu

Other then the one question I posted below, I feel that the changes are OK.

test/wrapper.gradle

brodycj · 2018-11-12T17:31:06Z

bin/templates/project/app/build.gradle

 }

 // Configuration properties. Set these via environment variables, build-extras.gradle, or gradle.properties.
 // Refer to: http://www.gradle.org/docs/current/userguide/tutorial_this_and_that.html
 ext {
    apply from: '../CordovaLib/cordova.gradle'
+
    // The value for android.compileSdkVersion.
    if (!project.hasProperty('cdvCompileSdkVersion')) {
        cdvCompileSdkVersion = null;


Remove semicolon here (as proposed in PR #441)?

What does this have to do with this PR?

brodycj · 2018-11-12T17:34:20Z

bin/templates/project/app/build.gradle

    }
 }

 // Allow plugins to declare Maven dependencies via build-extras.gradle.
 allprojects {
    repositories {
-        mavenCentral();
+        mavenCentral()


Add google() or replace mavenCentral() with google() here, as suggested in https://github.com/apache/cordova-android/pull/441/files/478450fe4fe6a7b06e15d2b68facf0aecd63a27e#r228693626?

What has #441 to do with this?

Updates and suggestions in #441 may be relevant here. This PR is to update Gradle in general. #441 was raised to introduce Kotlin support but it did include some general updates that I think we should consider outside of adding the Kotlin support.

brodycj · 2018-11-12T17:35:51Z

I raised a few comments based on PR #441. Should we address these?

Is this ready to be merged?

dpogue · 2018-11-12T17:38:03Z

I believe this is ready to be merged, but I'm not sure what you think it has to do with #441, which is proposing the addition of an entirely different feature.

This PR is updating gradle versions for compatibility with the latest Android SDK.
Kotlin support is out of scope here.

brodycj · 2018-11-12T17:40:20Z

I understand that Kotlin is out of scope. But I think some suggestions in #441 should be considered since they seem to be outside the scope of adding Kotlin.

brodycj · 2018-11-12T17:41:33Z

Or should some of the suggestions be considered in yet another PR?

davidschreiber · 2018-11-15T13:13:27Z

bin/templates/project/app/build.gradle

        jcenter()
    }
 }

 task wrapper(type: Wrapper) {
-    gradleVersion = '4.4.0'
+    gradleVersion = '4.6.0'


Any reason for not using the latest 4.10.2? It should deliver better build speeds and other benefits.

Historically we've targeted the minimum required version of gradle

Is there some specific reason for targeting the minimum version? I don't think that targeting older versions has benefits over targeting newer versions. Also, Gradle development between 4.6 and 4.10 greatly focused on speed

(Might be worth opening a whole new issue about this thing so it doesn't get buried here)

Also, as I mentioned in #596 - Since Gradle 4.9 it is possible to build App Bundle - a better way to publish apps to the Play Store.
Currently, Cordova doesn't support this and I have to manually change the app manifests after each build.

Ref apacheGH-504.

raphinesse reviewed Sep 26, 2018

View reviewed changes

dpogue force-pushed the gradle-updates branch from 8760016 to a36c0f8 Compare September 27, 2018 05:47

anatoly-spb mentioned this pull request Sep 27, 2018

Ionic 3 + Android: Cannot build android apk with NDK r18 ionic-team/ionic-framework#15775

Closed

dpogue mentioned this pull request Oct 2, 2018

Inconsistent handling of min/target SDK values #508

Closed

dpogue force-pushed the gradle-updates branch 2 times, most recently from 07574ac to 8286353 Compare November 1, 2018 05:08

erisu approved these changes Nov 5, 2018

View reviewed changes

test/wrapper.gradle Show resolved Hide resolved

knight9999 approved these changes Nov 6, 2018

View reviewed changes

dpogue force-pushed the gradle-updates branch 2 times, most recently from 58f46de to baf09c0 Compare November 6, 2018 04:36

dpogue closed this Nov 6, 2018

dpogue reopened this Nov 6, 2018

brodycj mentioned this pull request Nov 12, 2018

CB-14089: (android) Add Kotlin support #441

Closed

3 tasks

brodycj reviewed Nov 12, 2018

View reviewed changes

davidschreiber reviewed Nov 15, 2018

View reviewed changes

davidschreiber mentioned this pull request Nov 15, 2018

Upgrade to PSPDFKit 5.0.1 for Android PSPDFKit/Cordova-Android#37

Merged

dpogue added 2 commits November 20, 2018 21:12

Updates for latest gradle/build tools

f1f1ac3

Ref apacheGH-504.

Silence some deprecation linter warnings

47bb518

dpogue force-pushed the gradle-updates branch from baf09c0 to 47bb518 Compare November 21, 2018 05:13

erisu merged commit 2163c8a into apache:master Nov 21, 2018

dpogue deleted the gradle-updates branch November 21, 2018 06:27

dpogue mentioned this pull request Dec 3, 2018

Support gradle 4.9+ #596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gradle updates #507

Gradle updates #507

dpogue commented Sep 26, 2018

raphinesse Sep 26, 2018

dpogue Sep 26, 2018

raphinesse Sep 26, 2018

codecov-io commented Sep 26, 2018 •

edited

Loading

oliversalzburg commented Sep 26, 2018 •

edited

Loading

oliversalzburg commented Sep 26, 2018

oliversalzburg commented Sep 26, 2018 •

edited

Loading

oliversalzburg commented Sep 26, 2018

dpogue commented Nov 1, 2018

erisu left a comment

brodycj Nov 12, 2018

janpio Nov 12, 2018

brodycj Nov 12, 2018

janpio Nov 12, 2018

brodycj Nov 12, 2018

brodycj commented Nov 12, 2018

dpogue commented Nov 12, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 12, 2018

davidschreiber Nov 15, 2018

dpogue Nov 21, 2018

davidschreiber Nov 21, 2018

janpio Nov 21, 2018

theunreal Dec 5, 2018

		@@ -5,7 +5,7 @@ environment:
		GRADLE_OPTS: -Dorg.gradle.daemon=false

		# URL for SDK Tools, Revision 26.1.1 (September 2017)


		- choco install gradle --version 3.4.1
		- choco install gradle --version 4.6.0

Gradle updates #507

Gradle updates #507

Conversation

dpogue commented Sep 26, 2018

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 26, 2018 • edited Loading

Codecov Report

oliversalzburg commented Sep 26, 2018 • edited Loading

oliversalzburg commented Sep 26, 2018

oliversalzburg commented Sep 26, 2018 • edited Loading

oliversalzburg commented Sep 26, 2018

dpogue commented Nov 1, 2018

erisu 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

brodycj commented Nov 12, 2018

dpogue commented Nov 12, 2018

brodycj commented Nov 12, 2018

brodycj commented Nov 12, 2018

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

codecov-io commented Sep 26, 2018 •

edited

Loading

oliversalzburg commented Sep 26, 2018 •

edited

Loading

oliversalzburg commented Sep 26, 2018 •

edited

Loading