Use custom Gradle properties to read minSdkVersion value from config.xml

[brodybits]

Based on proposal by @erisu in #508 (comment), along with a few more changes:

• some minor fixes to GradlePropertiesParser configuration
• quick fix to handle minSdkVersion correctly
• updates from review
• now limited to minSdkVersion setting

I think some more improvements are needed to make the behavior easier to understand and maintain, hope I will get a chance to revisit after the new release is available.

FUTURE TODO:

• some cleanup & improvements by @dpogue in a branch referenced below are not included here

Resolves ref: #508

[erisu]

It might also be recommend to add our defaults to this._defaults in the constructor method.

Same as how @dpogue did it with his commit in his repo: dpogue@d18f8d1

[brodybits]

Marked as WIP for now. I would like to resolve #657 before continuing with rework on this one.

[brodybits]

I had some trouble using cdvCompileSdkVersion to configure the target SDK version, which is related to issue #657. My idea is to go ahead with the proposed changes for cordova-android 8 and then revisit and refine at some point in the future. If anyone has a better idea please come forward.

I would like to get PRs #656, #663, and #664 merged first, and test these changes with master before we merge this one.

[brodybits]

I resolved the review comments above, now limited the changes to handle minSdkVersion. I encountered more problems dealing with compile or target SDK value, would like to keep outside the scope of this PR.

P.S. I still did not get a chance to test my recent work, keeping it as a WIP PR for now.

[codecov-io]

Codecov Report

Merging #655 into master will decrease coverage by 0.21%.
The diff coverage is 38.46%.

@@            Coverage Diff            @@
##           master    #655      +/-   ##
=========================================
- Coverage   64.61%   64.4%   -0.22%     
=========================================
  Files          18      18              
  Lines        1820    1826       +6     
=========================================
  Hits         1176    1176              
- Misses        644     650       +6

Impacted Files	Coverage Δ
bin/templates/cordova/lib/prepare.js	36.3% <0%> (-0.38%)	⬇️
...lates/cordova/lib/config/GradlePropertiesParser.js	74.07% <55.55%> (-9.26%)	⬇️

---

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 719acd3...393b440. Read the comment docs.

[erisu]

I am dismissing the review block as the changes were applied. I will review it again when it is no longer a WIP PR.

As for the targetSdkVersion, I not sure what issues you saw. With my original changes, it worked but I didn't use the cdv prefix and is maybe why I didn't see what you saw.

Correction were applied.

[brodybits]

I will review it again when it is no longer a WIP PR.

Thanks. I cannot promise when I will be able to continue with testing and fixing this one right now. I wouldn't mind if you or anyone else wants to take it over.

As for the targetSdkVersion, I not sure what issues you saw.

#657 was the main one. I tried quite a few things, nothing worked right for me. The compileSdkVersion and targetSdkVersion do not always seem to be the same thing, more details in #657.

I also had some trouble using privateHelpers.getProjectTarget to get the targetSdkVersion value.

I gave some reasons in #508 (comment) why I would favor limiting these changes to minSdkVersion.

You are welcome to try it yourself, I don't have much time to test or discuss any further right now.

[brodybits]

Now tested, requesting final review from @erisu.

P.S. I would like to merge this proposal upon approval. In case anyone else decides to merge this proposal it should be a squash merge with the Co-authored-by comments preserved in the commit message.

[erisu]

LGTM

I ran the following test

$ npx cordova@nightly create androidTest3 com.foobar.androidTest3 androidTest3 && cd $_
$ npx cordova@nightly platform add github:brodybits/cordova-android\#custom-gradle-properties
Adding android project...
Creating Cordova project for the Android platform:
	Path: platforms/android
	Package: com.erisu.androidTest3
	Name: androidTest3
	Activity: MainActivity
	Android target: android-28
Subproject Path: CordovaLib
Subproject Path: app
Android project created with cordova-android@8.0.0-dev
...

$ npx cordova@nightly build android
...
BUILD SUCCESSFUL in 10s
...
$ aapt dump badging app-debug.apk
package: name='com.foobar.androidTest3' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'28'
...

$ npx cordova@nightly run android
BUILD SUCCESSFUL in 0s
...
INSTALL SUCCESS
LAUNCH SUCCESS
...

$ vi config.xml
... added <preference name="android-targetSdkVersion" value="26" />

$ npx cordova@nightly build android
...
BUILD SUCCESSFUL in 2s
...
$ aapt dump badging app-debug.apk
package: name='com.foobar.androidTest3' versionCode='10000' versionName='1.0.0' platformBuildVersionName='1.0.0' compileSdkVersion='28' compileSdkVersionCodename='9'
sdkVersion:'19'
targetSdkVersion:'26'

[brodybits]

Thanks @erisu for the extra testing.

So the android-minSdkVersion preference is now properly handled by the Gradle scripts, while the android-targetSdkVersion preference works because the value is copied to AndroidManifest.xml and not overridden by the Gradle scripts. (I think the second one should eventually go away, once we can reapply #664 (remove uses-sdk from AndroidManifest.xml), for another major release in the future.)

I did also try with the following command, which seemed to work properly:

cordova build android -- --gradleArg=-PcdvMinSdkVersion=21