breautek commented 4 years ago

Platforms affected

Android

Motivation and Context

This PR adds support to build android bundle (aab) files. Fixes: #729

Description

~Adds an --bundle flag to the cordova build command. If present, cordova will do the necessary steps to run the gradlew bundleDebug command (or gradlew bundleRelease if --release flag is present)~

Accepts packageType property from build.json, or optionally the --packageType=apk|bundle command line argument. The packageType property was chosen to keep consistency with the iOS platform.

This PR does not change the current build commands. Building APKs is still necessary for deploying apps to test devices, or of course to upload to the Google Play store using the traditional way. Bundle files are exclusively for deploying to Google Play Store.

You can however use the google provided bundletool program to build APKs from a bundle file so this PR still provides the ability to build bundles in both debug and release variants.

If build.json is present in the cordova project, just like building APKs, bundles will automatically be signed using the key and password as defined in build.json.

This PR does alter a previous constraint where if a signing property is present, but is missing the required properties for signing a warning would be displayed. This was altered to only show the warning if a property for signing is present is missing the required properties for signing. In other words, if packageType is the only property defined in the build.json file, then a warning about missing required properties for signing won't be emitted.

The cordova run android command has been modified to produce an error if packageType is set to bundle. This is because you cannot execute/deploy a bundle directly to the phone. I believe it is possible to replace all usages of the adb tool and instead use Google's bundletool, which provides tools to deploy bundle files to devices and thus support the cordova run command using bundles, but that would be another PR for another day.

Testing

I have done the following:

Added unit tests for using the --packageType flag
Ran npm test and ensure all tests passed (Running Node 10.x & cordova@9)
I installed cordova-android platform to a test app via cordova platform add https://github.com/breautek/cordova-android.git#app-bundle and manually ran build commands and all combinations of --debug, --release, and --bundle
I have created a test app on Google Play store to ensure that google will accept a signed aab file created by cordova build android --bundle --release.
I have tested app-bundle branch against one of my work projects which involves a more complicated project structure and includes the use of native libraries (crosswalk) and observed successful builds.

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

TODO

[x] Additionally fallback to build.json packageType to decide whether to build an APK or bundle. Overridable by --packageType (Removing the --bundle flag)
[x] Update build.json documentation
[x] Fix the added unit test so that they are consistent (one uses withContext, the other doesn't)
[x] Revert devDependencies nyc and jasmine

Crosswalk Users

See https://github.com/apache/cordova-android/pull/764#issuecomment-534101278

breautek commented 4 years ago

Travis seems to be failing to build because it is missing sdkmanager?

dpogue commented 4 years ago

Thanks for the pull request!!

I haven't looked at this in any detail yet, but in my recent musing about how we might add support for this feature I had imagined we might reuse the packageType field from build.json (with Android-specific values) to determine whether to make a bundle or an APK.

janpio commented 4 years ago

Travis seems to be failing to build because it is missing sdkmanager?

I opened https://github.com/apache/cordova-android/pull/765 to start investigating what is going wrong in the current CI configuration and hopefully fix it. (CI hasn't been run for quite some time here, so it probably broke somewhere along the way.)

janpio commented 4 years ago

https://github.com/apache/cordova-android/pull/765 is ready and should be merged soon, then you can rebase your PR onto master again and have proper tests running again.

janpio commented 4 years ago

Restart CI with now passing tests in master.

codecov-io commented 4 years ago

Codecov Report

Merging #764 into master will decrease coverage by 0.55%. The diff coverage is 51.72%.

@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
- Coverage   66.57%   66.02%   -0.56%     
==========================================
  Files          18       19       +1     
  Lines        1822     1860      +38     
==========================================
+ Hits         1213     1228      +15     
- Misses        609      632      +23

Impacted Files	Coverage Δ
bin/templates/cordova/Api.js	`53.48% <0%> (ø)`	:arrow_up:
bin/templates/cordova/lib/PackageType.js	`100% <100%> (ø)`
bin/templates/cordova/lib/build.js	`27.77% <36.36%> (+1%)`	:arrow_up:
...n/templates/cordova/lib/builders/ProjectBuilder.js	`66.3% <60%> (-3.34%)`	:arrow_down:
bin/templates/cordova/lib/run.js	`98.43% <66.66%> (-1.57%)`	:arrow_down:

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 38c6627...b0a113b. Read the comment docs.

breautek commented 4 years ago

Thanks for the pull request!!

I haven't looked at this in any detail yet, but in my recent musing about how we might add support for this feature I had imagined we might reuse the packageType field from build.json (with Android-specific values) to determine whether to make a bundle or an APK.

I'm open to suggestions and will gladly modify this PR. I opened this PR with exactly that in mind actually.

I do have one question about using a setting from build.json. For some context, I'll describe a bit of android bundles that I understand to make sure we are on the same page.

Bundles won't be replacing APKs. Developers actively developing their app will still build APKs and deploy to their devices/emulators for testing. But one might opt to build a bundle instead of an APK to release to the app store.

So I think the general use case is most of the time, developers will be building APKs, just like they have always done traditionally. Then they'll build a bundle when they are ready to make a release. If the setting to decide whether to build an APK is in a json file, I think most people will end up requiring to edit the file constantly based on what they want to build. This is why I opted to add a --bundle flag instead, something that is easy to tack on when required, and easy to configure CI as well.

Interested to hear your thoughts on this.

janpio commented 4 years ago

This is probably not about either/or, but about "additionally". I generate the build.json for builds on CI for example, and being able to switch to a bundle there would match my use case pretty well I think.

breautek commented 4 years ago

This is probably not about either/or, but about "additionally". I generate the build.json for builds on CI for example, and being able to switch to a bundle there would match my use case pretty well I think.

Ok that makes sense to me. Later tonight I can make an edit where I can grab the setting from build.json packageType. Overriding it if ~--bundle~ --packageType=value is present on the command line.

dpogue commented 4 years ago

I think what we do on iOS with packageType is that we try to read it from build.json, but it can also be specified on the command-line as --packageType=value. The values we want to support are probably just going to be apk and bundle, with apk being the default if it's not specified.

erisu commented 4 years ago

Is the bumping of the devDependencies jasmine and nyc required for this PR to pass? Could they be in a separate PR or made in a separate commit (for regular merge instead of squash merge)?

breautek commented 4 years ago

Is the bumping of the devDependencies jasmine and nyc required for this PR to pass? Could they be in a separate PR or made in a separate commit (for merge vs squash merge)?

nyc had a high vulnerability, but no I don't believe it was required to change these versions for the tests to pass. I can revert these changes if it is more proper for them to be made in a separate PR.

brodybits commented 4 years ago

I would really favor keeping the jasmine & nyc updates to devDependencies in a separate PR, as I think they have nothing to do with this feature. I would also really favor getting these devDependencies merged before we merge this one, for the sake of cleaning up the warning asap.

breautek commented 4 years ago

I would really favor keeping the jasmine & nyc updates to devDependencies in a separate PR, as I think they have nothing to do with this feature. I would also really favor getting these devDependencies merged before we merge this one, for the sake of cleaning up the warning asap.

Thanks for your explanation, I'm a little new to this. :) Didn't think of potential conflicts that could occur later. The changeset was more or less and artefact of me messing around with jasmine when I having troubles with Node12, that I forgot to revert before I made the PR. I've added a TODO item for myself to revert these devDependency changes later tonight.

brodybits commented 4 years ago

What is the status of this proposal? Is anything blocking it?

janpio commented 4 years ago

From my viewpoint this looks good!

(Might want to update the PR description though according to the changes done via reviews)

But as it is a significant addition, I think @breautek should write a short email to the dev list for lazy consensus on how this functionality is added (. We also need a brother PR for the Android docs that can be merged when the next Android release is done (minor release should be fine as it just adds functionality).

breautek commented 4 years ago

Sorry, currently the the only thing missing is documentation, which I've been too busy this weekend to write it. But i think that's done in cordova-docs repo anyway.

I'd also like to do some more testing before I give it my stamp of approval since I've removed the --bundle flag and replaced it with using the build.json stuff. I haven't thoroughly tested it myself outside of using npm test and quick manual checks.

brodybits commented 4 years ago

Thanks @breautek for your work on this. I tried to help by updating nyc and getting AppVeyor & Travis CI with npm test working on Node.js 12. Please let us know if there is anything else we can do to help (no pressure intended).

I really think that this is not the easiest codebase for new contributors to work with. Thanks again for your help so far.

breautek commented 4 years ago

Thanks @breautek for your work on this. I tried to help by updating nyc and getting AppVeyor & Travis CI with npm test working on Node.js 12. Please let us know if there is anything else we can do to help (no pressure intended).

I really think that this is not the easiest codebase for new contributors to work with. Thanks again for your help so far.

Thanks, I should have time to work on the last bits tonight. I think I'll probably have questions regarding how to update the documentation in the cordova-docs repo, but I'll ask more specific questions when/if I run into problems.

I really think that this is not the easiest codebase for new contributors to work with.

It was a fun learning experience. :)

janpio commented 4 years ago

Pointers re docs: https://cordova.apache.org/docs/en/latest/guide/platforms/android/index.html#signing-an-app https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/guide/platforms/android/index.md Probably compare to how iOS documents its parameters.

breautek commented 4 years ago

Currently, if you don't have a build.json file, or otherwise don't have the keystore and alias parameters filled but have --packageType argument present, the following warning is displayed.

'keystore' and 'alias' need to be specified to generate a signed archive.

It's just a warning, but it feels a little weird how the presence of --packageType triggers the warning as the --packageType flag doesn't really have to do with signing.

These lines are the trigger of the warning https://github.com/apache/cordova-android/blob/a64d459c8e339ec1b480672c028eef01027a1c02/bin/templates/cordova/lib/build.js#L102-L111

To re-iterate this occurs when keystore and alias is both undefined, but --packageType is displayed. Because packageType is a build.json key, it is inside packageArgs, thusObject.keys(packageArgs)` returns 1, which allows it to enter line 109.

I feel like this should be changed so perhaps the warning only gets emitted if one of the signing arguments is present (keystore/alias/password/storePassword), but lacks keystore or alias arguments. Thoughts?

janpio commented 4 years ago

I feel like this should be changed so perhaps the warning only gets emitted if one of the signing arguments is present (keystore/alias/password/storePassword), but lacks keystore or alias arguments. Thoughts?

+1

(Although might indicate that using --packageType or build.json for the bundle option might not follow the original thoughts behind that - but I assume you didn't encounter any other problems and it feels right? Then that should be fine.)

breautek commented 4 years ago

~Alright, assuming travis/appveyor comes back successful, this PR is ready for more criticism.~

You can't "run" a bundle, but doing cordova run android -- --packageType=bundle will still build a bundle, but only install the latest apk built, or it will error if you've never built an apk.

I think --packageType should be forced to apk if using cordova run.

breautek commented 4 years ago

You can't "run" a bundle, but doing cordova run android -- --packageType=bundle will still build a bundle, but only install the latest apk built, or it will error if you've never built an apk.

I think --packageType should be forced to apk if using cordova run.

Not sure what's the best way to handle this. I believe that cordova-lib repo is what fires the build process, before cordova-android run script executes when using cordova run android and it doesn't look like cordova-lib offers a chance for the platform to manipulate the build options before the build method is invoked.

https://github.com/apache/cordova-lib/blob/dfbab745d918de1cb24b2b0fe6bd1652ac2b227b/src/cordova/run.js#L46-L47

I don't see any other way where I can force --packageType to be apk when using cordova run so I think what I originally want will quickly become out of scope in this PR...

The other alternative is to make cordova run support bundles... which will involve incorporating the new bundletool which will needed to be downloaded and installed on the users machine. Note that using bundletool is basically a replacement of using adb as it has commands and flags to install APKs to connected devices, so I also see this quickly becoming out of scope of this PR.

Will it be okay if Cordova just exits with an useful message stating that --packageType must be apk when using cordova run android? If so, how is this typically done in cordova? I've tried using

events.emit('error', 'msg...');

which produced...

Unhandled error. ('Package type "bundle" is not supported during cordova run. use Package Type "apk" instead')

... seemingly because the the error isn't caught/handled upstream.

janpio commented 4 years ago

I think --packageType should be forced to apk if using cordova run.

Not sure what's the best way to handle this.

Can we somehow ignore --packageType for cordova run and output a warning message to the user that run will ignore that parameter and build an APK anyway maybe? I think that would be the most intuitive for a user - or the "error" thing you already tried.

The other alternative is to make cordova run support bundles... [...] so I also see this quickly becoming out of scope of this PR.

I agree, not necessary right now although you might open a new issue for this to be implemented in the future.

brodybits commented 4 years ago

You can't "run" a bundle, but doing cordova run android -- --packageType=bundle will still build a bundle, but only install the latest apk built, or it will error if you've never built an apk. I think --packageType should be forced to apk if using cordova run.

Not sure what's the best way to handle this. […]

I would favor a dumb but foolproof agile approach where cordova run android -- --packageType=bundle would fail with an ugly error message. I think it should be easy to do this in run.js.

At this point I think it would be an OK rough edge for Cordova to first attempt to build the bundle then fail with an ugly error message in run.js. My idea is get this into the hands of the user community now, then look into improving the whole thing in a new major release.

breautek commented 4 years ago

I would favor a dumb but foolproof agile approach where cordova run android -- --packageType=bundle would fail with an ugly error message. I think it should be easy to do this in run.js.

Okay, I won't touch run.js for the time being then.

Can we somehow ignore --packageType for cordova run and output a warning message to the user that run will ignore that parameter and build an APK anyway maybe?

This is what my original plan was. The setback here is that run.js is only executed after it decides to build. cordova-lib appears to control this flow and currently (as far as I know...) there is no way I can manipulate the build options before the build step is executed.

This means this PR at this current state... if you decide to use cordova run android -- -- packageType=bundle or you have "packageType": "bundle" inside build.json, then it will build a bundle, but try to deploy an APK. The APK that will be deployed will be the APK that was last build, or an error will fire stating he couldn't find the APK if no APK exists.

brodybits commented 4 years ago

I find it interesting that your recent commit be85316 both reverts a code style change and removes some changes you had made to run.js. The changes you had made to run.js in ca2fa615060c1f95414435ac74da68dda135ed77 looks to me like it would have been a good thing to keep in this PR. While tried to say it was not absolutely necessary, I do think it would be nice to have if it works correctly.

breautek commented 4 years ago

The changes you had made to run.js in ca2fa61 looks to me like it would have been a good thing to keep in this PR. While tried to say it was not absolutely necessary, I do think it would be nice to have if it works correctly.

Sorry must have misunderstood. The events.emits('error') did work give some meaningful feedback. I can re-add that commit.

Edit: Although the error sometimes occurs pretty late (such as after attempting to boot the emulator). If you're deploying to a device, it will fail fast.

breautek commented 4 years ago

I believe this PR is pretty sound now, for the first iteration of android bundle support anyway.

brodybits commented 4 years ago

I think my final nit is that it would be good for a test to verify that the error is produced if run is done with packageType=bundle. I think this test should be in spec/unit/run.spec.js. I would be happy to make this test if you like.

Otherwise I think this change is good to go.

breautek commented 4 years ago

I would be happy to make this test if you like.

Sure thing! Unit testing is actually still a learning experience for me, and I'm not sure how to actually determine if the log has been printed to console. But I can learn from what you do.

brodybits commented 4 years ago

I just pushed some quick changes to your branch:

pull from the master branch, which gives us the verbose test reporter among other things
added the test case, and added a throw statement that I think is needed for run to fail when it is called with --packageType=bundle (it may be better for the code to reject the promise instead of throwing an error)

The test case is known to fail since I put bogus string values into both the throw statement and the test case. @breautek, I am leaving it up to you to fix it as you feel is most appropriate here.

I am also not 100% confident that the run code will do the right thing if the packageType=bundle setting is configured in the build.config file. I would appreciate it if @breautek or any other experts would double-check this.

I will probably finish my day pretty soon, would be happy to take another look when I start my new day.

janpio commented 4 years ago

You both lost me along the way, but that's not a bad thing - progress is made. Pleaes ping me @breautek when you updated the PR description to reflect the full state of this PR (describes all the changes and behavior) and I will look over the code again. 🚀

breautek commented 4 years ago

FYI, I'll be pretty busy for the rest of the week/weekend. So I'll probably only come back to this Monday night.

breautek commented 4 years ago

@janpio I have updated the original PR description to include the most up to date changes and I have no more pending changes to be made against this branch, unless of course further feedback dictates more changes.

janpio commented 4 years ago

There is a conflicted file, then this should be fine for another review.

breautek commented 4 years ago

There is a conflicted file, then this should be fine for another review.

My bad, I thought that conflict had something to do with the PR hold. I resolved the conflict.

breautek commented 4 years ago

Pinging the reviewers as requested from the mailing list discussion

@dpogue @erisu @brodybits @janpio

brodybits commented 4 years ago

Merging now

breautek commented 4 years ago

Virtual drinks on me :beers:

brodybits commented 4 years ago

🍻

should be shared I think, thanks again for the contribution

beevelop commented 4 years ago

You guys definitely rock 🍻 Thank you for that feature ✌️

tomriddle1234 commented 4 years ago

Hi, but can someone tell me how to use, Google play just enhanced 64bit check, cordova is not working at all for Android, all my apk get rejected.

I tried,

cordova build android --release --packageType=bundle

cordova build android --bundle

still getting apk instead of aab.

cordova-android installed with

ionic cordova platform add android@https://github.com/apache/cordova-android.git

janpio commented 4 years ago

Step 1: Open a new issue instead of commenting on an unrelated, merged and PR.

steebchen commented 4 years ago

Thanks for this change, it works great! Would it maybe be possible to create a pre-release for cordova 9, or some other kind of release for this change on npm? It would be a bit easier to use compared to setting up this repo/master branch as a custom android platform.

janpio commented 4 years ago

@nightly should include this (and all other merged changes of course).

steebchen commented 4 years ago

Thanks! However, npm i cordova-android@nightly (or yarn) installs 8.1.0-nightly.2019.6.28.4cf3dcfa, which seems to be a commit 4cf3dcfaae6dc82ddb1ccf439d209cbcc2f474a0 from April 13. What can I do?

janpio commented 4 years ago

Ugh, that's true - you should create an issue that cordova-android currently doesn't build nightlies :/

Then your best bet is to install from master (which Cordova CLI supports) or fork the repo and publish under your own npm package if you really don't want to use via Git.

steebchen commented 4 years ago

Thanks for the info. Created #810

apache / cordova-android

feat: Build app bundles (.aab files) #764

Platforms affected

Motivation and Context

Description

Testing

Checklist

TODO

Crosswalk Users

Codecov Report