Fix: Cordova requirements consider the android-targetSdkVersion

[breautek]

Platforms affected

Android

Motivation and Context

This PR fixes cordova requirements android to consider the desired target SDK as defined by the android-targetSdkVersion preference in the project's config.xml.

Fixes #846

Currently, while cordova-android can use a higher target... it fails the cordova requirements android check if the hard-coded defined target is not installed on the system.

Description

I've changed check_reqs.js get_target to do the following:

1. Get the default/minimum required target sdk, as defined by cordova-android
2. Check if config.xml exists in the repo.
3. If so, then attempt to read the android-targetSdkVersion preference.
4. 1. If read... check for validity
5. 1. 1. If valid, and is >= the default target sdk, then change the target to the preference value
6. 1. 2. If not valid, then fallback to default.
7. 1. 3. If not valid because preference target is < default target, then produce a warning.
8. 2. if config.xml, then fall back to default.

Naturally, if get_target returns 29, then the user must have the android 29 SDK installed, otherwise check_android_target will fail, as expected.

Unit tests were added to thoroughly test get_target.

Testing

Ran npm test, with newly added unit tests.
Also tested the code in a real project and manually observing the results of cordova requirements android.

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

[erisu]

I won’t be able to review this until I get back Sunday. I do not have my computer available to easy review and test.

One thing to note, the value can also be passed in via Command Line argument. There is also a possibility they set the gradle.properties file.

#699 (comment)

[breautek]

There is also a possibility they set the gradle.properties file.

The original code did look at a .properties file, and this code continues to do so. I treat the value in there as the "minimum target sdk". This might be a little confusing when there is a min sdk option, but at the same time, google is going to disallow any apps with a target less than 28 anyway.

the value can also be passed in via Command Line argument

I was not aware of this, and if this is the case, I'm fairly confident current master wouldn't work either as the original code never accepted any parameters nor looked at command line args. But if this is the case, I think it's probably best to address this in this PR as well and I'll add commits accordingly.

[breautek]

I misunderstood originally. The documentation for gradle.properties is missing for target sdk? Does this mean you can still change the target sdk via this method?

I also don't see any documentation on command line usage other than the buiild.json stuff which does not appear to include any arguments for sdk stuff.

[freescout-helpdesk]

Can this be included in the next release?

Our users are having problems because of this issue (freescout-help-desk/freescout#362)

[breautek]

There is a couple of things I still need to check actually regarding command line usage, that @erisu pointed out that I haven't found the time to get to yet.

I can't really give a timeline when I will be able to take a look. With that being said, it may take awhile before PR lands in master, then it may take some time before it is actually released in NPM. But if things are urgent, you can fork cordova-android and apply this PR against your own fork.

[breautek]

@erisu

I see in your comment listing bunch of tests using -- --targetSdkVersion=<version>

But when I run that command, it still expects (and installs sdk 28 if missing) even if the target sdk requested is not 28. Looks like I'll have to add that command line check somehow into the get_target to get consistent behaviour. Thanks for the heads up.

[erisu]

I just re-ran the test case and I get the same results.... Can you provide your steps?

My Steps

$ npm un -g cordova
removed 455 packages in 1.969s

$ npm i -g cordova
..
+ cordova@9.0.0
added 455 packages from 361 contributors in 14.819s

$ cordova create androidTest com.foobar.androidTest androidTest && $_
Creating a new cordova project.

$ cordova platform add android
Using cordova-fetch for cordova-android@^8.0.0
Adding android project...
Creating Cordova project for the Android platform:
	Path: platforms/android
	Package: com.foobar.androidTest
	Name: androidTest
	Activity: MainActivity
	Android target: android-28
Subproject Path: CordovaLib
Subproject Path: app
Android project created with cordova-android@8.1.0
Plugin 'cordova-plugin-whitelist' found in config.xml... Migrating it to package.json
Discovered saved plugin "cordova-plugin-whitelist". Adding it to the project
Installing "cordova-plugin-whitelist" for android
Adding cordova-plugin-whitelist to package.json

$ cordova build android -- --targetSdkVersion=20
...
BUILD SUCCESSFUL in 11s
44 actionable tasks: 44 executed
Built the following apk(s):
	/private/tmp/cordova-test/androidTest/platforms/android/app/build/outputs/apk/debug/app-debug.apk

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

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

targetSdkVersion version is set to 20.

Additional notes: I tested with the aapt from build-tools@29.0.0 and build-tools@28.0.3

I guess we talking about the display of cordova requirements android does not represent what was built?

[breautek]

Can you provide your steps?

It's because you have android-28 installed. If android-28 is installed, nothing complains, even if you aren't going actually going to be using android-28 because you're targeting a higher SDK level. The source of this is mostly because check_reqs has the sdk level hard-coded. So while specifying different targets may work, it will only work properly if you have the version that is hard-coded installed. This is wrong and cordova should be able to use any android sdk >= it's default.

So in my environment, the only API I have installed is android 29. If I override the cordova default and specify 29 for target sdk, cordova shouldn't require me to have android-28, nor should it install 28 automatically for me because having android-29 satisfies and is required to use target sdk 29.

Currently, this is what it does:

cordova build android -- --targetSdkVersion=29
Checking Java JDK and Android SDK versions
ANDROID_SDK_ROOT=undefined (recommended setting)
ANDROID_HOME=/development/Android/Sdk (DEPRECATED)
Subproject Path: CordovaLib
Subproject Path: app

> Configure project :app
Checking the license for package Android SDK Platform 28 in /development/Android/Sdk/licenses
License for package Android SDK Platform 28 accepted.
Preparing "Install Android SDK Platform 28 (revision: 6)".

"Install Android SDK Platform 28 (revision: 6)" ready.
<truncated output>

This PR addresses this problem when specifying the target sdk from config.xml, but it does not consider command line arguments because check_reqs has never considered looking at command line arguments.

Simply passing the arguments down to check_reqs and parsing them using nopts like build/run does doesn't want to work the same either. I don't get targetSdkVersion as a readable property for some reason, but if the command line is cordova build android --targetSdkVersion, nopts does work. Not sure why nopts is behaving differently when used in check_reqs but that's something I will have to look into deeper later.