-
Notifications
You must be signed in to change notification settings - Fork 528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #1875, #3744, #3735, #2432: Add support for app bundles via Bazel, Proguard, and build flavors #3750
Conversation
There are a lot of details to cover here--see the PR for the complete context.
- Add missing codeowner - Add support for configuring base branch reference - Update CI for dev/alpha AAB builds to use 'develop' since there's no origin configured by default in the workflows
Moving this into review for now despite it being blocked on #3752. |
@rt4914 PTAL for the bits that affect UI layer & if you have any thoughts on the build aspects, @anandwana001 PTAL for Bazel-specific pieces, and @seanlip PTAL for codeowners. |
Keeping assignment to myself per #3752 as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm for codeowners.
Unassigning @seanlip since they have already approved the PR. |
This is needed to open a PR on GitHub. This commit is being made so that the PR can start off in a broken Actions state. This also initially disables most non-Bazel workflows to make workflow iteration faster and less impacting on other team members.
This introduces a new mechanism for passing lists of tests to sharded test targets in CI, and hooks it up. No actual sharding is occurring yet. This led to some simplifications in the CI workflow since the script can be more dynamic in computing the full list of targets (which also works around a previous bug with instrumentation tests being run). Java proto lite also needed to be upgraded for the scripts to be able to use it. More testing/documentation needed as this functionality continues to expand.
This simply partitions bucketed groups of targets into chunks of 10 for each run. Only 3 buckets are currently retained to test sharding in CI before introducing full support.
Fixes some caching bucket and output bugs. Also, introduces while loop & keep_going to introduce resilience against app test build failures (or just test failures in general).
Add docstrings for proto.
@anandwana001 you need to use Bazel 4.0.0 with build tools 29.0.2. This is actually why I recently pinned the version but that hasn't made it into this branch yet. I'll update it once I pull in #3757. |
…/oppia-android into add-bundles-proguard-build-flavors
@anandwana001 just updated the branch--can you try again? It should give a clearer error now. Also, hoping that CI will run this time. |
Aha, it appears sharding is successfully unblocking CI for this PR. :) I think all that's needed to unblock the merge now is:
|
I'm running locally now. Waiting for the result, I will update here once build is complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM on CI.
Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
@anandwana001 are you building from a clean directory? If not, I suggest running Also, are you building on Mac? That could also potentially cause differences I suppose. |
Neither 'mv -t' nor piping to mv work on OSX so we needed to find an alternative (in this case just trying to move everything). This actually works a bit better since it's doing a per-file move rather than accommodating for files that shouldn't be moved (which isn't an issue since the destination directory is different than the one containing the AAB file).
To close the loop: there was an issue with one of the unpack commands on OSX--this has been patched in the latest version. |
Explanation
Fixes #1875
Fixes #3744
Fixes #3735
Fixes #2432
At a high-level, this PR accomplishes three things:
Each of these have various caveats mentioned in the subsections below.
App bundle support
I initially tried using the android_application Starlark macro introduced as part of the pre-release alpha version of rules_android that was suggested by the Bazel team. However, it has some issues with wrangling resources that led to a dead-end set of errors that we couldn't work around. With help from the Bazel team, a lot of research of various bundle-tool & Bazel resources, and some referencing the rules_proto macros, I was able to build a medium-term custom version of android_application that can successfully build an app bundle. This has been tested both on local devices & via a Play Store deployment (using the internal channel). See below section for how to build & run these new targets.
Building app bundles effectively requires four steps:
There's a fair amount of complexity hidden behind the new Starlark rules being introduced (in oppia_android_application.bzl), especially around linking to the aapt2 & bundletool binaries (the former of which is part of the user's local Android SDK & the latter which is conveniently downloaded from GitHub). Android's public documentation for bundletool (which is linked in a few places in code) was really helpful in understanding the steps, but rules_android was also critical since Android's documentation doesn't really describe how to properly fix the directory structure when using aapt2 to convert an APK to an AAB (it actually suggests an alternative strategy of linking directly from AAPT2 rather than going via an APK). I chose to follow the APK route since it's closer to rules_android which I'm hoping will mean an easier migration in the future.
Please note the new bundle_config.pb.json file. This is actually really important: it allows us to tweak how bundletool assembles the final AAB. Since bundles are templates for Play Store to generate APKs, we needed to make sure that bundles don't split on language (which they do by default) since this would require users to be connected to the internet in order to switch languages (& it also inconveniently ruins custom language support since it strips unrecognized language values from the resources list).
Regarding CI, two new workflows are added for each of the two new build flavors (see below), both of which are AABs. These are individually uploaded in the same way that oppia.apk is uploaded. Note that we intentionally keep both the APK and AAB builds intentionally since:
Finally, this PR also introduces a convenience
bazel run
command for running AABs by using the bundletool to deploy them to the device (this is helpful since there are two commands that need to be run to actually deploy an AAB to a device). See below section for examples of this new command. Note that this command does not support having multiple devices/emulators present at the same time, and it's actually not known how the command will behave in these circumstances.Multiple build flavors
build_flavors.bzl introduces a macro for defining custom build flavors. In order to do this "properly" we're also adding support for dynamically transforming the app manifest during compile time to generate a version name & embed other version information within it. This simplifies the release process & consolidates version tracking to a new version.bzl file (which we conceivably may automatically update in the future if we introduce automatic releases from GitHub CI).
Some details on this:
To start, two flavors are being introduced: dev and alpha. 'dev' is meant to be the version of the app that has parity with
//:oppia
(i.e. has fully debugging support), whereas 'alpha' is meant to be our current production flavor of the app.The following targets can be used to build the new AABs, respectively:
bazel build //:oppia_dev
bazel build //:oppia_alpha
As mentioned above, each of these can also be installed conveniently directly from Bazel:
bazel run //:install_oppia_dev
bazel run //:install_oppia_alpha
Proguard & resource shrinking
In order to have proper production flavors (such as oppia_alpha), it's important to also have a properly optimized, obfuscated, and shrunk version of the app. This is done through a series of new Proguard configuration files (.pro) which were compiled through research & trail and error (in a way where I actively tried to reduce broad
-dontwarn
patterns). I've played through most of the app to verify that everything works, but we should more extensively test before deploying to a broad audience. These configurations will not apply to Gradle.Resource shrinking has also been enabled however it seems to like to strip away some critical resources that actually broke Firebase. To mitigate, this PR introduces a new raw resource file for shrink exemptions & explicitly prohibits those resources from being removed. This configuration will apply to Gradle, but I think Gradle already works around this issue.
We saw a pretty substantial reduction in AAB size: ~13.2MB (oppia_dev) -> ~7.4MB (oppia_alpha) vs. ~14.2MB (oppia.apk). This is with Proguard 3-pass optimization and resource shrinking. I think we could see much more substantial reductions in the future (partially related: #3381 & #3751).
Also, unsurprisingly there is a high cost to running Proguard (especially for the 3 configured optimization passes). On my machine it takes about an extra ~2.5 minutes just for the optimization step, so it's not recommended to use oppia_alpha for regular development (oppia_dev is intentionally not Proguarded so that builds are fast).
Note that, as mentioned above, we are now building oppia_alpha in CI as of this PR. This means that new dependencies may break Proguard and require updates to the various .pro files. This is by design--there's work needed to make sure that new or updated dependencies play nice with Proguard, and these CI builds will help us find those situations.
Miscellaneous
compilation_mode=opt
in order to produce a deployable AAB for the Play Store. This is demonstrated in CI, but I haven't yet figured out a way to define this automatically via bazelrc or the target definition yet. Test suite surpasses max number of parallel tasks supported by GitHub #3752 is tracking.Essential Checklist
For UI-specific PRs only
N/A