cordova-android icon indicating copy to clipboard operation
cordova-android copied to clipboard

feat!: API 34 Support

Open breautek opened this issue 2 years ago • 1 comments

Platforms affected

Android

Motivation and Context

Adds in support for the next android platform

Closes https://github.com/apache/cordova-android/issues/1661 Closes https://github.com/apache/cordova-android/pull/1596

Description

This PR supercedes and includes https://github.com/apache/cordova-android/pull/1596

This PR does a few different things to provide API 34 support which will be noted below.

Default Configuration Changes

Target SDK: 34 Gradle Version: 8.5 AGP Version: 8.2.1 Kotlin JVM Target: 1.8 (If kotlin is enabled) Min Build Tools: 34

Java Changes

~Source/Target compatibility we changed from Java 8 to 17 on both the framework and app template projects.~

Source & Target compatibility have been reverted back to 8, but 2 new preferences are added to allow developers to experiment: AndroidJavaSourceCompatibility and AndroidJavaTargetCompatibility which adjusts the respective gradle compile options. By default, these values are 8, which is the same values used before.

BuildConfig

Previously this I believe was implicitly enabled, but now needs to be explictly enabled otherwise the following may occur

* What went wrong:
A problem occurred configuring project ':app'.
> com.android.builder.errors.EvalIssueException: defaultConfig contains custom BuildConfig fields, but the feature is disabled.
  To enable the feature, add the following to your module-level build.gradle:
  `android.buildFeatures.buildConfig true`

I however did encounter this on a simple project, so it might be required depending on what the plugins does.

AGP 8.2

This is the minimum version required for API 34 builds. Attempting build against API 34 on earlier versions will produce warnings and/or errors.

Gradle -b flag

This flag has been deprecated and scheduled to be removed in gradle's next major release. The gradle projects must follow a standard structure now. Cordova's generated project already follows the standard structure thus for cordova build commands the -b append can simply just be removed, we don't need to explicitly declare the build.gradle file.

It was also used against the wrapper.gradle file which changes are noted below.

gradle.wrapper

The "intentionally empty" gradle.wrapper file has been removed.

We no longer manually manage gradle versions. Instead, we let gradle do it for us by utilising their --gradle-version argument. A new function was introduced to replace the old runGradleWrapper to:

  1. setup the gradle wrapper if not already present
  2. install/update the gradle wrapper to the specified version.

By default, this will be 8.4 as defined in the defaults json config file, but may be overwritten by GradleVersion preference, or CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL environment variable.

If CORDOVA_ANDROID_GRADLE_DISTRIBUTION_URL is defined, the preference will be ignored and --distribution-url will be used instead, maintaining the ability for corporations to install their own gradle wrapper from their own source.

Android Studio

The recommended Android Studio to use with AGP 8.2 is Android Studio Hedgehog.

CI

JDK was updated from 11 to 17, which is a requirement to build for API 34

Testing

Ran NPM test Ran in sample cordova hello world app Ran in one of my projects

Pending Tasks:

These are tasks that are still need to be done before this PR is ready for review.

  • Make Kotlin JVM target obey the AndroidJavaTargetCompatibility preference value, as they should be equal.

Checklist

  • [x] I've run the tests to see all new and existing tests pass
  • [x] I added automated test coverage as appropriate for this change
  • [x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • [x] 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)
  • [x] I've updated the documentation if necessary

breautek avatar Oct 21 '23 01:10 breautek

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 72.50%. Comparing base (ed8e5d2) to head (b974bba).

Files Patch % Lines
lib/builders/ProjectBuilder.js 80.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1678      +/-   ##
==========================================
+ Coverage   72.15%   72.50%   +0.35%     
==========================================
  Files          23       23              
  Lines        1839     1837       -2     
==========================================
+ Hits         1327     1332       +5     
+ Misses        512      505       -7     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 21 '23 01:10 codecov-commenter

Having an issue with npm t not able to download and use gradle wrapper.

I think I know what you're running into. It's dumb but gradle cannot install the gradle wrapper if the project does a gradle version check (which the android gradle plugin does). In otherwords if AGP requires Gradle >= 8.7.0 it will prevent a lesser gradle version from using the gradle wrapper task to install a 8.7.0 wrapper.

Using gradle's --gradle-version is hella lot easier than what we did before, which involved deleting the wrapper because even if you switched to the new version it won't use it if it already had a wrapper installed. The wrapper task manages all of that for us and is what Gradle recommends.

They also recommend to check in the wrapper, which is also dumb imo (breaks the general rule of no "built" artefacts in the repo, cause it causes headaches during PR and merge times).

So we either:

  1. check in gradle wrapper into the repo, and exclusively use it always (e.g. remove all references of the gradle executable, which is only ever used to manage the wrapper).

  2. Require users to require the appropriate system gradle version.

Option 1 is following Gradle's recommendation but I think can run into issues that can be problematic. For example, what happens if the end user uses the preference to install a gradle wrapper less than the minimum required by AGP? Will it run and switch for it to fail later once it starts using the unsupported wrapper version? Once the wrapper is out of date, the only way to fix it is to use the appropriate system gradle at the proper version. Option 1 seems like a better approach and follows Gradle recommendations has some concerns.

Option 2 seems like a much safer approach, just more annoying for end users and defeats the purpose of using the wrapper really.

breautek avatar May 09 '24 12:05 breautek

IIRC we're not allowed to commit the gradle jar file, this was an issue that came up when we were originally moving from ant to gradle and there were ASF policies that prevented redistribution of the jar.

dpogue avatar May 09 '24 18:05 dpogue

IIRC we're not allowed to commit the gradle jar file, this was an issue that came up when we were originally moving from ant to gradle and there were ASF policies that prevented redistribution of the jar.

Do you recall where Apache states so?

https://infra.apache.org/licensing-howto.html#permissive-deps makes it sound like it's permissible for bundling dependencies, for as long as the license permits redistributions. And there is clarification on how to handle binaries here

breautek avatar May 09 '24 18:05 breautek

It's not regarding licensing per se, it's under the source packages policy:

Source packages

Every ASF release MUST contain one or more source packages, which MUST be sufficient for a user to build and test the release provided they have access to the appropriate platform and tools. A source release SHOULD not contain compiled code.

That last line in particular.

With that said, I looked and see several other ASF projects with gradle wrappers committed to their repositories, so I don't think I have any actual objection to this.

dpogue avatar May 09 '24 18:05 dpogue

It's not regarding licensing per se, it's under the source packages policy:

Source packages

Every ASF release MUST contain one or more source packages, which MUST be sufficient for a user to build and test the release provided they have access to the appropriate platform and tools. A source release SHOULD not contain compiled code.

That last line in particular.

With that said, I looked and see several other ASF projects with gradle wrappers committed to their repositories, so I don't think I have any actual objection to this.

I believe another section allows it given certain requirements:

https://www.apache.org/legal/release-policy.html#distribute-other-artifacts

ASF releases typically contain additional material together with the source package. This material may include documentation concerning the release but must contain LICENSE and NOTICE files. As mentioned above, these artifacts must be signed by a committer with a detached signature if they are to be placed in the project's distribution directory.

Again, these artifacts may be distributed only if they contain LICENSE and NOTICE files. For example, the Java artifact format is based on a compressed directory structure and those projects wishing to distribute jars must place LICENSE and NOTICE files in the META-INF directory within the jar.

I unpacked the wrapper jar file and found that they have their own license file, but does lack a NOTICE file.

At the end of the day, the contents is still going to be signed, comes from a trusted source, assuming it gets updated via gradle tooling and the jar is a build tool dependency, it isn't part of the apache source code. But I understand how that could tie into the "provided they have access to the appropriate platform and tools." In legal terms they also used SHOULD instead of MUST or SHALL. "Should" is more of a suggestion rather than mandatory whereas "must" or "shall" is mandatory terms.

Therefore including compiled code should be a thoughtful process (like this one). It shouldn't be made a habit, it should only be done when it's truly necessary. I think the gradle wrapper fits all of those ticks.

breautek avatar May 09 '24 18:05 breautek

Latest Android Studio updates gradle wrapper to 8.6, should we use the same version Android Studio updates to instead of latest?

jcesarmobile avatar May 10 '24 00:05 jcesarmobile

Latest Android Studio updates gradle wrapper to 8.6, should we use the same version Android Studio updates to instead of latest?

Unless if there is a specific issue with 8.7 I see no reason to use it by default as the current stable release. Android Studio might default to 8.6 perhaps as the stable gradle at the time of their release.

8.7 includes a ton of bug fixes and improves including security patches that affects 8.6.

breautek avatar May 10 '24 00:05 breautek

FYI, was talking with Erisu last night and we came up with a plan to remove the JAR and thus avoid the security concerns about the integrity (e.g. xz backdoor style attacks) of the JAR file by introducing an empty gradle project, which effectively will serve as the old wrapper.gradle that we had.

Running system gradle against the actual project will result in loading up the AGP plugin and thus requiring the end-user to have a system gradle version that satisfies AGP gradle requirements. Previously with the (now deprecated/pending removal of -b flag) wrapper.gradle file, it was used to install the wrapper without loading the AGP plugin, virtually allowing the user to be able to use any gradle version to install the wrapper.

With the removal of the -b flag gradle is enforcing a consistent project structure. So the plan is to introduce a secondary gradle project that is just empty and serves the purpose of installing gradle wrapper, which should be usable by the cordova build. If time permits, I'm planning on making these changes later today.

breautek avatar May 10 '24 12:05 breautek

Some last few changes which I think improves the PR significantly.

Like in the original post I really don't like submitting generated content. Utilising an empty gradle project we are able to now generate the wrapper on the fly rather than checking it in and redistributing it.

Using an empty project allows us to avoid AGP's version requirements when running the system gradle. Allowing us to use virtually any gradle version to generate the 8.6+ gradle wrapper.

The gradle wrapper and it's jar has been removed.

breautek avatar May 10 '24 23:05 breautek

Additional note regarding PR description:

Android Studio

The recommended Android Studio version to use with AGP 8.4 is Android Studio Jellyfish.

I consider it a 'hard requirement' for users to upgrade to Android Studio Jellyfish at a minimum for successful builds and seamless functionality from Android Studio, as the term 'recommended' suggests that the version is not mandatory.

Older versions of Android Studio are not compatible with AGP 8.4 and will fail.

For instance, attempting to build on the following older versions of Android Studio will result in the following error messages:

Android Studio Hedgehog

The project is using an incompatible version (AGP 8.4.0) of the Android Gradle plugin. The latest supported version is AGP 8.2.0-rc03.
See Android Studio & AGP compatibility options.

Android Studio Iguana

The project is using an incompatible version (AGP 8.4.0) of the Android Gradle plugin. The latest supported version is AGP 8.3.0-alpha14.
See Android Studio & AGP compatibility options.

erisu avatar May 13 '24 03:05 erisu

Also the .ratignore changes could be reverted, but if reverted, need to add at least gradle-wrapper.properties. This this file is detected if anyone runs locally a rat test after running npm test.

Sounds like we can keep the .ratignore as is then, cause if it detects those files after a test run, then you'll probably need the gradlew scripts to be ignored as well.

breautek avatar May 13 '24 10:05 breautek

I consider it a 'hard requirement' for users to upgrade to Android Studio Jellyfish at a minimum for successful builds and seamless functionality from Android Studio, as the term 'recommended' suggests that the version is not mandatory.

Older versions of Android Studio are not compatible with AGP 8.4 and will fail.

I've updated the wording, also added a smaller Breaking Changes section that highlights in short bullet-form of the notable breaking changes.

breautek avatar May 13 '24 10:05 breautek

I've reverted AGP down to 8.3

Jellyfish appears to bundle Gradle 8.4 by default and will fail on installing the wrapper due to AGP 8.4's requirement on Gradle 8.6. In order to use AGP 8.4 on Jellyfish, the user would have to do additional configurations to specify an alternate gradle.

breautek avatar May 13 '24 11:05 breautek

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Hello,

Does Cordova support Android 14? I saw you were working on it (api34-support), but didn't see any update on the oficial documentation here https://cordova.apache.org/docs/en/12.x/guide/platforms/android/index.html ? Is there a version 13.0.0 of Cordova which targets Android 14? Thank you!

ramona-cristea avatar Jul 04 '24 12:07 ramona-cristea

Is there a version 13.0.0 of Cordova which targets Android 14? Thank you!

Yes. See here for more details.

breautek avatar Jul 04 '24 12:07 breautek