Skip to content
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

Merged
merged 20 commits into from
Oct 8, 2021

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Oct 7, 2021

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:

  1. Introducing a permission needed by AndroidX WorkManager
  2. Introducing proper legacy multidex support (with Bazel-specific issues--see below & Bazel/Gradle Support on Kitkat #3803)
  3. Adding support for vector graphics (which is API 21+ normally -- Bazel not support vector drawables #3804)
  4. Removing/disabling OkHttp (which doesn't have KitKat support -- Unable to run app on API 19 because of Okhttp library #3853)
  5. Fixing some Java 8 incompatibilities
  6. Properly setting up the build for KitKat due to build-specializations

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:

  1. Minimizing the startup dependencies will result in a potentially less efficient startup compared with a fine-tuned list (though that would likely require automation like redex to keep it up-to-date)
  2. Changes to the OppiaApplication class or certain third-party dependencies can very likely break the KitKat startup flow, so introducing a KitKat e2e test that runs in CI will be really important (Introduce end-to-end test to ensure KitKat startup support #3911 is tracking introducing the test).

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:

  1. Don't use vector drawables (not preferable since vectors are smaller in file size, higher fidelity, more VCS-friendly, and easier to manually change if needed)
  2. Generate PNGs for vector drawables at different display densities as a fallback on SDK 19 & below (Gradle-only; not an option on Bazel--see Bazel not support vector drawables #3804)
  3. Use the compatibility portion of the AndroidX library to load vector drawables at runtime through AndroidX rather than native Android

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):

  • Needing to use the 'compat' variants for drawable attributes, particularly for image:src, android:drawableStart, android:drawableTop, etc.
  • Needing to enable replacing the resource loading pipeline for drawables to route through AndroidX (to cover cases when attributes aren't replaced, such as compound drawables, layer-lists, and background drawables)
  • Needing to ensure that when running AAPT2 not to inadvertently move vector drawables to an API 21+ qualified resource folder since they're technically unsupported below that version *

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:

  • //:oppia was updated so that it actually uses version codes & a transformed manifest for parity with its AAB counterpart
  • An //:oppia_kitkat build was introduced for parity with //:oppia, but for KitKat-compatibility
  • oppia_dev & oppia_alpha AAB flavors now have KitKat complements: oppia_dev_kitkat & oppia_alpha_kitkat, respectively
  • The "KitKat" variants of each of the above leverage a different minimum SDK & manual dex list, and the non-KitKat versions are L+ with native multidex configured for better performance (and eventual true build differences once Dagger Hilt is supported)

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)

  • See the vector graphics section for details on the regex pattern & test changes for drawable-related patterns
  • The shadow, tests, and implementation for BidiFormatter were updated to use AndroidX's instead since the built-in BidiFormatter isn't available on API 19
  • A new regex pattern was added to ban Java 8 Optional per the incompatibility mentioned above
  • I only tested this on an emulator since I don't have access at the moment to a physical KitKat device (though I'm working with a PM contributor to get this tested on a physical KitKat device)
  • I noticed upon testing that Glide seems to have some issues when loading & transforming vector drawables from disk. I don't have an immediate fix, but Investigate Glide drawable native crash on KitKat #3887 is tracking the issue.
  • I added myself as the codeowner for the new main dex file list since I think I have the best context to maintain this file at this point, but it's something that could go to a general release team in the future

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

There are no UI-specific changes corresponding to this PR since it's simply providing basic KitKat support.

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
@BenHenning BenHenning marked this pull request as ready for review October 7, 2021 01:51
@BenHenning BenHenning marked this pull request as draft October 7, 2021 01:52
@BenHenning BenHenning changed the title Fix #3803, #3804: Add basic KitKat support in Bazel Fix #3803, #3804, #3853: Add basic KitKat support in Bazel Oct 7, 2021
The removed files were accidentally re-added upon merge.
@BenHenning BenHenning changed the title Fix #3803, #3804, #3853: Add basic KitKat support in Bazel Fix #3803, #3804, #3853, #3669: Add basic KitKat support in Bazel Oct 7, 2021
@BenHenning BenHenning changed the title Fix #3803, #3804, #3853, #3669: Add basic KitKat support in Bazel Fix #3803, #3804, #3853, #3669, #3383: Add basic KitKat support in Bazel Oct 7, 2021
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).
@BenHenning BenHenning marked this pull request as ready for review October 7, 2021 08:12
@BenHenning BenHenning requested a review from a team as a code owner October 7, 2021 08:12
@BenHenning
Copy link
Sponsor Member Author

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.
@seanlip PTAL for new codeowner file/change.
@anandwana001 PTAL for codeowners & the general Bazel setup going on here since it'd be good to get a set of eyes on those changes.

The develop branch needs to also be pulled for building //:oppia dev now
since it uses a transformed manifest.
Copy link
Member

@seanlip seanlip left a 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.

@oppiabot oppiabot bot unassigned seanlip Oct 7, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 7, 2021

Unassigning @seanlip since they have already approved the PR.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@anandwana001 anandwana001 removed their assignment Oct 7, 2021
@BenHenning
Copy link
Sponsor Member Author

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).

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks.

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Oct 8, 2021
@oppiabot oppiabot bot added the PR: LGTM label Oct 8, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 8, 2021

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!

@BenHenning
Copy link
Sponsor Member Author

Thanks all! Merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants