feat!: unify & fix gradle library/tooling overrides

[breautek]

Platforms affected

Android

Motivation and Context

Fixes #1178
Depends on #1211

The original target SDK fix PR didn't actually solve problem. This is because the framework and the compile SDK did not change based on the android-targetSdkVersion preference, resulting the default API to be downloaded and used, despite the target SDK change.

Description

This is a breaking change because this PR replaces the cdkTargetSdkVersion and cdvCompileSdkVersion with a new variable, cdvSdkVersion, which the android-targetSdkVersion sets.

cdvSdkVersion will control the android target API, as well as the target SDK and compile SDK settings of the android project.

Testing

npm test & manual testing. If you remove all platform APIs, and set android-targetSdkVersion to 29 you should see:

• upon platform add, see android 29 printed
• upon cordova build, see API 29 and only API 29 tools downloaded.

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]

Needs to be rebased and there is also build failures.

FAILURE: Build completed with 2 failures.

1: Task failed with an exception.
-----------
* Where:
Build file '/cordova/sampleApp/platforms/android/app/build.gradle' line: 160

* What went wrong:
A problem occurred evaluating project ':app'.
> Could not get unknown property 'defaultSdkVersion' for project ':app' of type org.gradle.api.Project.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
==============================================================================

2: Task failed with an exception.
-----------
* What went wrong:
A problem occurred configuring project ':app'.
> compileSdkVersion is not specified. Please add it to build.gradle

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.
==============================================================================

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.8.3/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 3s
Command failed with exit code 1: /cordova/sampleApp/platforms/android/gradlew cdvBuildDebug -b /cordova/sampleApp/platforms/android/build.gradle

[codecov-commenter]

Codecov Report

Merging #1212 (171c420) into master (47aa116) will decrease coverage by 0.81%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
- Coverage   71.90%   71.08%   -0.82%     
==========================================
  Files          21       22       +1     
  Lines        1694     1705      +11     
==========================================
- Hits         1218     1212       -6     
- Misses        476      493      +17     

Impacted Files	Coverage Δ
...n/templates/cordova/lib/builders/ProjectBuilder.js	73.85% <0.00%> (-0.98%)	⬇️
bin/templates/cordova/lib/prepare.js	44.75% <36.00%> (-3.04%)	⬇️
...in/templates/cordova/lib/gradle-config-defaults.js	66.66% <66.66%> (ø)
bin/lib/create.js	94.90% <100.00%> (+0.71%)	⬆️
bin/templates/cordova/lib/check_reqs.js	71.32% <100.00%> (-1.34%)	⬇️

---

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 47aa116...171c420. Read the comment docs.

[erisu]

Everything seems OK to my now.

[breautek]

@erisu

I made one last commit, as testing the latest changes produced a fail build locally...

The change ensure the settings have proper types. For example gradle expects a number for the target SDK setting and will crash if it receives a string,

but it looks like config.json could produce these values as strings if overridden via preferences.

Let me know what you think about the latest commit.

[raphinesse]

I do not really like us inventing our own template system, especially if it's only for one file. How about we remove TemplateFile and use JS string templates instead like I did in cordova-js?

So in this case, instead of reading the project.properties and passing them through the TemplateFile class, we replace it with a file project-props-template.js that looks like this:

module.exports = ({ SDK_VERSION }) => `
# Indicates whether an apk should be generated for each density.
split.density=false

# Project target.
target=android-${SDK_VERSION}
apk-configurations=
renderscript.opt.level=O0
android.library=true
`.trimLeft();

create.js can then just import that file and pass the desired SDK_VERSION when calling the exported function.

[breautek]

@raphinesse I think I can do one better...

I agree, having the TemplateFile class is a bit wasteful now that it's barely used. Since the only now only used to build a properties file, we can even use properties-parser, which is a dependency that is already used.

[raphinesse]

@raphinesse I think I can do one better...

I agree, having the TemplateFile class is a bit wasteful now that it's barely used. Since the only now only used to build a properties file, we can even use properties-parser, which is a dependency that is already used.

Agreed, that's probably even better.

[breautek]

Only thing about using properties-parser is that they don't have a sync API, so I had to turn the function into an asynchronous method -- but I believe the module is private anyway (exported purely for unit testing purposes).

[raphinesse]

Cool! I think that's a better solution.

We should probably remove the placeholder and add a comment to the properties file now?

[breautek]

Cool! I think that's a better solution.

We should probably remove the placeholder and add a comment to the properties file now?

Will do. I'll remove it completely to avoid confusion and add a generated file notice. Also just realised this isn't working... it's writing out an empty value for some reason... so I'm troubleshooting that right now.

[raphinesse]

Left a few more comments

[erisu]

Tested the recent changes again and now everything now appears to be working as expected.

[raphinesse]

IMHO this is good to go. I would appreciate reviews of my last changes though.

[erisu]

• The code changes LGTM.
• I tested various settings to confirmed that the config file was updated or reverted to defaults.
• I ran a build test.
• I verified the built APK contained the SDK targets. With and without changes.

[MikeDimmickMnetics]

It is a bad idea to unify these settings. Target SDK is a runtime setting which controls the level of backward compatibility fixes that Android is applying to the app. Compile SDK controls which declarations are being used to compile the code. They're separate things. You can still use features newer from API levels newer than your 'target SDK' setting, while keeping backward-compatible behaviour for the older parts of your app (or libraries) that you haven't upgraded yet.

I realise this is a bit academic for apps on the Play Store, because Google require Target SDK = 30, from November 2021, even for updates to older apps. However, for anyone doing private distribution, including Managed Play Store, forcing an update to the target SDK in order to change the compile SDK is unnecessary. I'm not sure why you'd want to set the compile SDK lower than the target SDK, though.

My line-of-business app, using cordova-android 6.4, currently uses compile SDK 26, to pick up support for adaptive icons, but target SDK 23, because we've not fixed any compatibility issues later than Marshmallow. I don't want to have to suddenly fix Android 11 compat issues, such as scoped storage enforcement, when upgrading to cordova-android 10.