-
Notifications
You must be signed in to change notification settings - Fork 550
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 #3803, #3804, #3853, #3669, #3383: Add basic KitKat support in Bazel #3910
Conversation
At the moment, about ~40% of drawables can be successfully converted (with some coordinate system inconsistencies for their gradients that need to be corrected). Some other SVG constructs still need to be added for the remaining drawables (such as groups & clip regions).
This includes: - Defining KitKat-specific flavors - Utilizing a manual main dex file list (minimized) so that the Bazel-built KitKat flavor of the app can start successfully - Migrating all drawable usage over to compat-compatible so that vectors can load properly in the KitKat world (+ opted-in compatibility layer) - Adding a missing permission that seems only needed on KitKat with WorkManager (but isn't unreasonable to request in general)
Lock vector compat call behind KitKat gate.
Conflicts: app/src/main/res/layout-land/pin_password_activity.xml app/src/main/res/layout-land/topic_practice_subtopic.xml app/src/main/res/layout-sw600dp-land/pin_password_activity.xml app/src/main/res/layout-sw600dp-land/topic_practice_subtopic.xml app/src/main/res/layout-sw600dp-port/pin_password_activity.xml app/src/main/res/layout-sw600dp-port/topic_practice_subtopic.xml app/src/main/res/layout/pin_password_activity.xml app/src/main/res/layout/topic_practice_subtopic.xml app/src/main/res/values/styles.xml build_flavors.bzl scripts/assets/file_content_validation_checks.textproto scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt version.bzl
The removed files were accidentally re-added upon merge.
This came from an unrelated previously merged PR.
The styles simplification comes from https://stackoverflow.com/a/50854279/3689782. Apparently, Bazel is more permissive about this sort of thing than Gradle, but it might have actually been wrong before (i.e. this version seems more correct).
The PR is looking pretty good locally, so I'm going ahead and sending this into review now. There might be some CI hiccups, but I'll address them tomorrow my time. @rt4914 PTAL for codeowners. |
The develop branch needs to also be pulled for building //:oppia dev now since it uses a transformed manifest.
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.
Thanks @BenHenning! LGTM for codeowners.
Unassigning @seanlip since they have already approved the PR. |
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 🎉
Note that the builds in CI seem to be working well. See https://github.com/oppia/oppia-android/pull/3910/checks?check_run_id=3824699336 for artifacts (you can easily download & try out the KitKat builds if you'd like). |
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, thanks.
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! |
Thanks all! Merging now. |
## Explanation Fixes #5012 Removes references and code corresponding to KitKat (SDK version 19) since the app has been set to a minimum version of Lollipop (SDK 21) since #3910 (roughly--technically KitKat was still supported but we no longer shipped a KitKat version). Because of this, the changes in this PR are largely a no-op from a build perspective. More broadly, this change is motivated by a desire to decrease CI time (which was already reduce considerably in the recent merge of #5629) by removing extraneous app builds being done in CI: - ``//:oppia`` since we should really only use ``//:oppia_dev`` moving forward. - Dev and alpha KitKat builds (the latter of which takes a long time since only the alpha job was building 2 proguarded builds of the app and thus taking much more time than the beta and GA build jobs). Separately, ``build_tests.yml`` and ``unit_tests.yml`` were updated to no longer support caching since we disabled this behavior a while back (#2884) and are unlikely to reintroduce it due to high storage costs. Some other noteworthy changes: - The optional providing of ``Retrofit`` and Retrofit services has been reverted since it's no longer necessary (and corresponding tests have been removed since there's no longer any condition to verify). - Code that was gated to run below Lollipop has been removed since it can't execute except on KitKat devices. - Support for a main dex target list has been removed as we're unlikely to need it with native multidex support (which wasn't an option for KitKat builds), though we may choose to reintroduce it to speed up cold app starts since it _can potentially_ help improve time-to-splash app startup performance. - Updated manifest min SDK values to avoid potential developer confusion on the min SDK version supported (though, strictly speaking, this doesn't technically matter with the way the app builds). - This updates the default min version supported for OS deprecation. No additional work is needed to actually activate the deprecation notice on the KitKat version of the app since we no longer deploy it, and OS deprecation wasn't added to the last version that was deployed. - Three file content pattern failure messages were updated to no longer reference KitKat (though the value of these checks mostly still seems realized, so I think it's fine to keep them in). - Some additional licenses were added due to tooling around the dev AAB (even though these aren't dependencies that ship with the app). I think it's fine including extra licenses in this case. - The protobuf license was corrected to BSD 3-Clause as declared for the dependency and in the GitHub repository for the dependencies. - This bumps version codes due to removing two flavors (for KitKat dev and KitKat alpha). - ``NetworkModuleTest`` has had its tests redesigned and better filled in in order to pass the code coverage minimum requirement (since old tests were removed). Note that it's not quite perfect in what it's testing, but it is at least verifying the complete configuration of the ``Retrofit`` instance, and the singleton properties of the two services currently supported (verifying that the services are set up would require a lot more testing that seems outside the direct scope of this test, so it seems okay to ignore here). - As part of the previous item's work, ``NetworkLoggingInterceptor`` had an issue fixed where it could cause an OkHttp exception to be thrown when trying to process logs (due to reading the response body twice). Reverting this change will now cause breakage failures in ``NetworkModuleTest``. - ``NetworkModule``'s initialization order was changed for slightly better network request logging (see the new comment in that file for more context). Note that there are some workflow job removals and renames in this PR. Any required CI checks will be correspondingly updated once this PR is approved and ready to merge (to avoid blocking any other in-flight PRs). ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only This shouldn't have an side effects on non-KitKat UIs, and KitKat support itself is being removed in this PR (so there's nothing to demonstrate).
Explanation
Fix #3803
Fix #3804
Fix #3853
Fix #3669
Fix #3383
Introducing basic KitKat support (that is, support for the app to open and be minimally stable during usage) required the following work:
Each of these are covered in more detail in later sections. Note that this PR continues the work from #3740 and #3723 which provided the basis for addressing the OkHttp dependency issue, and parts of the dexing issues.
New permission
Per https://developer.android.com/reference/androidx/work/package-summary AndroidX WorkManager requires the WAKE_LOCK permission before SDK 23 in order to properly handle background service/keep-device-awake scenarios for triggering workflows depending on certain conditions. This isn't KitKat specific, but it was a prerequisite for the app to run on KitKat devices.
Legacy multidex support in Bazel
Bazel supports three types of multidex modes: legacy, native, and manual. Native relies on the native ART environment to support reading from multiple dex fiiles at runtime, and works without issue.
Legacy assumes that the application extends AndroidX's multidex application to handle scenarios where multidex is needed (which it is for Oppia since we have 3 dex files compiled for the size of the developer flavor of the app). However, a big part of this is computing what files should go into the main dex file list for the app. Bazel uses some mechanism to compute this list (which I think uses D8 via a compatibility bridge in Android buiol-tools), but unlike Gradle it seems to have issues computing a list that can actually fit within the main dex list. This is a problem because the main dex list needs to contain everything necessary to load the main application class so that multidex can kick in & initialization can start (more on this later). It's not clear to me why Gradle works and Bazel fails (as in, will fail with a compiler error when trying to use "legacy"), but I suspect it has to do with Gradle using the new R8 dexer (which isn't yet available via Bazel per #3748). Finally, this isn't an issue for the alpha version of the app since Proguard brings the dex file list much lower (down to 2), so Bazel seems fine computing a proper main dex list.
Given neither native nor legacy are options for a KitKat-enabled dev build with Bazel, we are left with using the manual list. Some background on how multidex works might be helpful for why this is needed. Android 19 and prior assumed there is only ever 1 dex file on the system (and prior to I think SDK 4 it wasn't even possible to load more than one at runtime). This is an issue when the app exceeds dex file limits (such as having more than 64K methods), so multidex was introduced to allow loading additional dex files at runtime to extend the size potential of apps. This comes at a large performance cost, but it provides more flexibility. However, the main dex list still must contain the entire dependency closure of the core application class pathway in order for multidex to be initialized. I think this is why Bazel's computation breaks down because OppiaApplication references the core Dagger component which, in turn, essentially references every class in the Oppia codebase.
In reality, we actually need a much smaller minimal list to run. I started off by using Facebook's redex utility (https://github.com/facebook/redex/blob/master/docs/interdex.md which required some monkeypatching due to a Python incompatibility) to generate an initial list (https://gist.github.com/BenHenning/23b4887f94c42e8654bf552d26376fcc) of ~16k dependencies that was weeded down to the bare minimum necessary for the central pathway to work. Note that the main dex file will still be filled up with other dependencies that Bazel thinks are likely to be loaded early on, but the list guarantees that at least those files will be present. Note that this approach has a few drawbacks:
Support for vector graphics on API 19
Support for vector drawables weren't added until API 21. There are essentially three ways to work around this issue:
Given (1) isn't an option, I spent some time trying to get (2) to work by introducing a custom routine that would convert vector drawables to SVGs (taking inspiration from posts like https://stackoverflow.com/a/46350218/3689782), where I then planned to render out the SVGs as PNGs using https://github.com/sterlp/svg2png. There were other libraries I came across that tried to compute SVGs from vector drawables (essentially the reverse of Android Studio's import process), but they didn't work for vector drawables that use linear gradients as a fill type (which some of Oppia Android's do). I built an initial script (which can be viewed in one of the commits in the history of this branch) that successfully converted about 40% of Oppia Android's vector drawables, but given the additional work to bring in PNG support & convert to the correct display density, it seemed better to go with (3).
For (3), I came across resources:
All of which were really helpful in outlining the core considerations when trying to properly hook into the compat library for vectors. These are (in no particular order):
This PR makes the compat changes & adds both new regex checks & corresponding tests to ensure these stay correct long-term. It also updates OppiaApplication to enable the pipelining bit, though this does come at some potential cost to memory and stability (see medium articles above & https://issuetracker.google.com/issues/37090849 for some background). It's much more straightforward than the alternative (which would probably requiring abstraction all of our drawable loading to ensure mistakes aren't introduced that would caused tricky-to-anticipate crashes on KitKat devices), so I think it's a reasonable first step until we get more data on general app stability on KitKat devices. Another need for this is that the databinding library itself will load drawables through the native resources object rather than utilizing the compat library (see https://issuetracker.google.com/issues/111345022 for the open issue tracking this).
Note also that some binding adapter changes were needed to accommodate the new uses of compat attributes, but none of these seemed too unusual (though do note that some layout uses needed to be updated to use databinding).
Finally, https://stackoverflow.com/a/41855830/3689782 is a really nice infographic someone made that better explains the situation (we fall under the bottom case since we decided to leverage the compatibility bridge).
* Note that I didn't do anything extra here for Bazel since it seems to already pass the correct flag. Gradle requires additional configuration (see the medium articles above), but I didn't bother since it already generates the PNG variants today & we are planning to utilize Bazel as our distribution build system for alpha MR3 & onward.
OkHttp dependency workaround
The previous PRs cover this topic in good detail, but just to recap so that it's in one place: OkHttp dropped KitKat support (see #3723 (comment) for thoughts & context).
We're still trying to figure out from the product side what we should do about the de facto networking library no longer supporting KitKat, but at the moment since the app doesn't actually rely on networking for core functionality we decided just to hide Retrofit (which uses OkHttp underneath) behind an Optional wherein it's absent on KitKat devices to avoid loading in the OkHttp stack. Longer-term, this ought to be compiled-out entirely in KitKat builds (another TODO was added on #1720 to track this).
A new test suite was added to verify that Retrofit & services are properly present/absent when expected.
Java 8 incompatibility fix
There are some unexpected incompatibilities when trying to desugar Java 8 APIs for KitKat. It's not entirely clear to me why this is broken (seemingly for both Bazel & Gradle builds), but the easy workaround was to use Guava's Optional instead (since we're already pulling in Guava for select other uses).
To ensure compatibility, a new regex check was added to ensure that only Guava Optionals are used in the future (since another case of Java 8 Optional needed to be fixed in order to resolve an incompatibility which inadvertently fixed #3383).
There may be other Java 8 desguaring incompatibilities that we will run into later, but for now we'll continue heavily using Kotlin collections since they reduce this risk.
Build specializations for KitKat & version code changes
In order to simplify build management, some big changes were made around our build flavors:
Version codes were updated to be flavor-specific (based on AABs since these are what we're planning to ship). This is the first iteration of multiple future changes around building an automated release flow. The numbers themselves are starting at 10 since version codes 9 and before have been used on the Play Store or in previous APKs we've used in prototypes, demos, or studies. The order is intentional such that KitKat is always lower than non-KitKat (so that non-KitKat devices prefer the non-KitKat variant), and more stable builds are higher precedence than less table (i.e. alpha > dev).
CI has been updated to include steps for each of the 3 new KitKat builds (not separate jobs). This might cause an issue with longer CI runtimes (particularly for alpha since there could be 2 Proguard builds in sequence in the future per the Bazel Proguard/main dex list issue mentioned below), but if that happens we can split up each build to its own workflow (preferably with a shared action script since the workflows are becoming highly redundant at this point).
Finally, note that there's one issue with the alpha KitKat builds. Bazel seems to have an issue when trying to use a manual main dex class list with Proguard and just fails. #3886 is tracking this, but the takeaway is that we won't be able to ship a Proguard optimized version of the app for KitKat. This is fine in our current release stage, but it's definitely not optimal and something we'll want to fix once we see broader KitKat usage.
Script pattern changes & miscellaneous considerations (including Glide stability issues)
Essential Checklist
For UI-specific PRs only
There are no UI-specific changes corresponding to this PR since it's simply providing basic KitKat support.