Improve Gradle Build Arguments #699

erisu · 2019-03-27T09:43:27Z

Platforms affected

android

Motivation and Context

Fix Gradle Fails to sync after upgrading to Cordova Android 8.0 #675
Improve ability to set build arguments.

Description

Remove <uses-sdk> from AndroidManifest.xml (minSdkVersion, maxSdkVersion, and targetSdkVersion)
Remove AndroidManifest setter and getters for minSdkVersion, maxSdkVersion, and targetSdkVersion
Updated compileSdkVersion
Remove spec tests.
Improve ways of setting build SDK values:
- minSdkVersion
- maxSdkVersion
  - Added build flag (will set command-line argument and take priority)
  - Added config.xml <preference> name android-maxSdkVersion (Updates gradle.properties)
- targetSdkVersion
  - Added build flag (will set command-line argument and take priority)
  - Added config.xml <preference> name android-targetSdkVersion (Updates gradle.properties)

Testing

npm t

Checklist

I've run the tests to see all new and existing tests pass
I added automated test coverage as appropriate for this change
Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
I've updated the documentation if necessary

dpogue

This looks pretty good to me, a few minor comments.
I think we should review what we do with compileSdkVersion and how that interacts with targetSdkVersion.

dpogue · 2019-03-29T00:10:09Z

bin/templates/cordova/lib/build.js

@@ -266,6 +270,8 @@ module.exports.help = function () {
    console.log('    \'--prepenv\': don\'t build, but copy in build scripts where necessary');
    console.log('    \'--versionCode=#\': Override versionCode for this build. Useful for uploading multiple APKs.');
    console.log('    \'--minSdkVersion=#\': Override minSdkVersion for this build. Useful for uploading multiple APKs.');
+    console.log('    \'--maxSdkVersion=#\': Override maxSdkVersion for this build. Useful for uploading multiple APKs.');


Remove "Useful for uploading multiple APKs." from minSdkVersion, maxSdkVersion, andtargetSdkVersion.

There's also probably no value in supporting maxSdkVersion since they really really strongly discourage using it (your app will not be compatible with newer versions of Android)

At best, we might only be able to add to the printout (Not Recommended).

We already supported the setting of the maxSdkVersion property through config.xml and in which sets it in AndroidManifest.xml.

This PR moves the maxSdkVersion definition into the Gradle's workflow and removes from AndroidManifest.xml.

This does add a new command line option to exposes alternative ways for defining this value other than config.xml. I thought it was a missed option because we supported min and believe there was no harm to add.

If we want to remove maxSdkVersion altogether, it then becomes a major change. As for future planning, it would be best if I don't add the new command-line option then. We probably should just add warnings that the maxSdkVersion setting will be deprecated in the next major.

Thoughts and Opinions?

dpogue · 2019-03-29T00:11:25Z

bin/templates/project/app/build.gradle

-    ext.cdvCompileSdkVersion = privateHelpers.getProjectTarget()
-    //ext.cdvCompileSdkVersion = project.ext.defaultCompileSdkVersion
-}
+ext.cdvCompileSdkVersion = cdvCompileSdkVersion == null ? (


compileSdkVersion should probably default to targetSdkVersion, unless it is explicitly set

codecov-io · 2019-03-30T05:34:35Z

Codecov Report

Merging #699 into master will decrease coverage by 0.4%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
- Coverage   64.38%   63.98%   -0.41%     
==========================================
  Files          18       18              
  Lines        1828     1824       -4     
==========================================
- Hits         1177     1167      -10     
- Misses        651      657       +6

Impacted Files	Coverage Δ
bin/templates/cordova/lib/AndroidManifest.js	`100% <ø> (ø)`	⬆️
bin/lib/create.js	`92.72% <ø> (ø)`	⬆️
bin/templates/cordova/lib/prepare.js	`35.81% <0%> (-0.5%)`	⬇️
...lates/cordova/lib/config/GradlePropertiesParser.js	`76.66% <100%> (+2.59%)`	⬆️
bin/templates/cordova/lib/build.js	`26.77% <40%> (+0.75%)`	⬆️

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 9531dbb...9f67d1f. Read the comment docs.

erisu · 2019-04-01T04:56:35Z

Test Case

`config.xml`

`minSdkVersion`

<preference name="android-minSdkVersion" value="20" />

$ cordova create test1 com.foobar.test1 test1 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test1' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'20'
targetSdkVersion:'28'

<preference name="android-minSdkVersion" value="21" />

$ cordova build android

BUILD SUCCESSFUL in 1s
44 actionable tasks: 9 executed, 35 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test1' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'21'
targetSdkVersion:'28'

`maxSdkVersion`

<preference name="android-maxSdkVersion" value="20" />

$ cordova create test2 com.foobar.test2 test2 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test2' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
maxSdkVersion:'20'
targetSdkVersion:'28'

<preference name="android-maxSdkVersion" value="21" />

$ cordova build android

BUILD SUCCESSFUL in 1s
44 actionable tasks: 4 executed, 40 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test2' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
maxSdkVersion:'21'
targetSdkVersion:'28'

`targetSdkVersion`

<preference name="android-targetSdkVersion" value="20" />

$ cordova create test3 com.foobar.test3 test3 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test3' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'20'

<preference name="android-targetSdkVersion" value="21" />

$ cordova build android

BUILD SUCCESSFUL in 0s
44 actionable tasks: 4 executed, 40 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test3' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'21'

Command-Line Argument

`--minSdkVersion`

$ cordova create test4 com.foobar.test4 test4 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android -- --minSdkVersion=20

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test4' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'20'
targetSdkVersion:'28'

$ cordova build android -- --minSdkVersion=21

BUILD SUCCESSFUL in 1s
44 actionable tasks: 9 executed, 35 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test4' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'21'
targetSdkVersion:'28'

`--maxSdkVersion`

$ cordova create test5 com.foobar.test5 test5 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android -- --maxSdkVersion=20

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test5' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
maxSdkVersion:'20'
targetSdkVersion:'28'

$ cordova build android -- --maxSdkVersion=21

BUILD SUCCESSFUL in 0s
44 actionable tasks: 3 executed, 41 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test5' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
maxSdkVersion:'21'
targetSdkVersion:'28'

`--targetSdkVersion`

$ cordova create test6 com.foobar.test6 test6 && $_
$ cordova platform add github:erisu/cordova-android\#sync-fix
$ cordova build android -- --targetSdkVersion=20

BUILD SUCCESSFUL in 2s
44 actionable tasks: 44 executed

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test6' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'20'

$ cordova build android -- --targetSdkVersion=21

BUILD SUCCESSFUL in 0s
44 actionable tasks: 3 executed, 41 up-to-date

$ aapt dump badging platforms/android/app/build/outputs/apk/debug/app-debug.apk

package: name='com.foobar.test6' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'21'

erisu · 2019-04-01T05:48:58Z

@dpogue This should now be ready for a final review.

bin/templates/cordova/lib/build.js

For flexibility, `maxSdkVersion` is an available option to users but not recommended to set.

knight9999

No any problem.
All tests are green.

drastik · 2019-04-06T01:35:13Z

Heck yes!

This solves an annoying few extra steps each compile. May be worth noting, Google play console just forced me to use min SDK 26 earlier today, rejecting .apk uploads with min target = 19.

This PR also closes:
#680
#675

dpogue · 2019-04-06T04:15:39Z

@drastik I'm pretty sure Google's now requiring that the compileSdkVersion is set to a minimum of 26 for submission to the Play Store, but the minSdkVersion should be able to point to any previous version of Android.

erisu requested a review from dpogue March 27, 2019 09:43

brodycj mentioned this pull request Mar 27, 2019

fixed bad target+mini sdk version #680

Closed

5 tasks

erisu added 2 commits March 27, 2019 23:50

Remove uses-sdk from AndroidManifest

c95b0a1

Remove dependency elementtree

42d3b4f

erisu force-pushed the sync-fix branch from 23cdc23 to 42d3b4f Compare March 27, 2019 14:50

dpogue reviewed Mar 29, 2019

View reviewed changes

erisu marked this pull request as ready for review March 30, 2019 04:14

Updated Build Command Help Menu Printout

ac1bc60

erisu force-pushed the sync-fix branch from 6a2f324 to ac1bc60 Compare March 30, 2019 05:25

erisu added 3 commits March 31, 2019 18:41

Update Gradle Properties Parser to Always Set Overriding Changes

ccf648b

Add Update Exisiting Gradle Property Use Case

842851c

Update Gradle Properties Configure Method

7f074b9

Update GradlePropertiesParser Test Spec

fcc2ce6

erisu commented Apr 4, 2019

View reviewed changes

bin/templates/cordova/lib/build.js Outdated Show resolved Hide resolved

Update Build Help Printout for maxSdkVersion Flag

9f67d1f

For flexibility, `maxSdkVersion` is an available option to users but not recommended to set.

erisu requested a review from knight9999 April 4, 2019 23:40

erisu added this to the 8.0.1 milestone Apr 4, 2019

knight9999 approved these changes Apr 5, 2019

View reviewed changes

dpogue approved these changes Apr 6, 2019

View reviewed changes

erisu merged commit 485e6e0 into apache:master Apr 6, 2019

erisu deleted the sync-fix branch April 6, 2019 04:28

erisu mentioned this pull request Apr 6, 2019

I have a question about development with android stuido 3.2 and higher #629

Closed

This was referenced Apr 6, 2019

Compile SDK / Target SDK version not consistent with defaults in root build.gradle #631

Closed

Inconsistent handling of min/target SDK values #508

Closed

breautek mentioned this pull request Aug 26, 2019

Manifest merger failed Cordova-android 8.0.0 in Android Studio #817

Closed

3 tasks

erisu mentioned this pull request Oct 18, 2019

Fix: Cordova requirements consider the android-targetSdkVersion #849

Merged

5 tasks

breautek mentioned this pull request Apr 13, 2020

cdvMinSdkVersion seems to not work in 7.1.4 #599

Closed

breautek mentioned this pull request Jul 20, 2020

<uses-sdk> tag is not added to AndroidManifest.xml as expected #923

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Gradle Build Arguments #699

Improve Gradle Build Arguments #699

erisu commented Mar 27, 2019 •

edited

Loading

dpogue left a comment

dpogue Mar 29, 2019

erisu Mar 29, 2019 •

edited

Loading

dpogue Mar 29, 2019

codecov-io commented Mar 30, 2019 •

edited

Loading

erisu commented Apr 1, 2019

erisu commented Apr 1, 2019

knight9999 left a comment

drastik commented Apr 6, 2019

dpogue commented Apr 6, 2019

Improve Gradle Build Arguments #699

Improve Gradle Build Arguments #699

Conversation

erisu commented Mar 27, 2019 • edited Loading

Platforms affected

Motivation and Context

Description

Testing

Checklist

dpogue left a comment

Choose a reason for hiding this comment

dpogue Mar 29, 2019

Choose a reason for hiding this comment

erisu Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

dpogue Mar 29, 2019

Choose a reason for hiding this comment

codecov-io commented Mar 30, 2019 • edited Loading

Codecov Report

erisu commented Apr 1, 2019

Test Case

config.xml

minSdkVersion

maxSdkVersion

targetSdkVersion

Command-Line Argument

--minSdkVersion

--maxSdkVersion

--targetSdkVersion

erisu commented Apr 1, 2019

knight9999 left a comment

Choose a reason for hiding this comment

drastik commented Apr 6, 2019

dpogue commented Apr 6, 2019

erisu commented Mar 27, 2019 •

edited

Loading

erisu Mar 29, 2019 •

edited

Loading

codecov-io commented Mar 30, 2019 •

edited

Loading

`config.xml`

`minSdkVersion`

`maxSdkVersion`

`targetSdkVersion`

`--minSdkVersion`

`--maxSdkVersion`

`--targetSdkVersion`