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 #4628, #4667: Add support for filtering languages from builds #4687

Merged
merged 12 commits into from
Nov 11, 2022

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Nov 1, 2022

Explanation

Fixes #4628
Fixes #4667

The core fix for #4628 is splitting the language configuration between "all languages the app supports" and "production-ready languages." This is now configurable per-binary where all pre-beta binaries include all languages, and the beta & GA binaries only include production-ready languages (those are, languages for which we always aim to have 100% content translations). Tests use the "all languages" configuration, by default, for parity with the developer version of the app. This required a small change where we define the language proto as part of the shared //model module rather than as a separate package. As was before, this proto is only loaded in Bazel builds of the app since our Gradle pipeline doesn't support automatically textproto->binary proto conversion during build time.

To ensure that the app strings are filtered out, the supported languages configuration proto that's included in the app is passed through a new script that strips out all resources (including non-strings) from the binary. This has actually removed ~400KiB even from the pre-release binaries since our third-party assets include a bunch of strings outside our core languages.

New output now shows when building the app with Bazel to make it clearer what sorts of resources are being dropped due to incompatible languages:

82 resources are being removed that are tied to unsupported languages: [af, am, as, az, be, bg, bn, bs, ca, cs, da, de, el, en-AU, en-CA, en-GB, en-IN, en-XC, es, es-419, es-US, et, eu, fa, fi, fr, fr-CA, gl, gu, hr, hu, hy, in, is, it, iw, ja, ka, kk, km, kn, ko, ky, lo, lt, lv, mk, ml, mn, mr, ms, my, nb, ne, nl, or, pa, pl, pt, pt-PT, ro, ru, si, sk, sl, sq, sr, sr-Latn, sv, ta, te, th, tl, tr, uk, ur, uz, vi, zh-CN, zh-HK, zh-TW, zu] (size reduction: 391239 bytes).

(When building oppia_dev)

85 resources are being removed that are tied to unsupported languages: [af, am, ar, as, az, be, bg, bn, bs, ca, cs, da, de, el, en-AU, en-CA, en-GB, en-IN, en-XC, es, es-419, es-US, et, eu, fa, fi, fr, fr-CA, gl, gu, hi, hr, hu, hy, in, is, it, iw, ja, ka, kk, km, kn, ko, ky, lo, lt, lv, mk, ml, mn, mr, ms, my, nb, ne, nl, or, pa, pl, pt, pt-PT, ro, ru, si, sk, sl, sq, sr, sr-Latn, sv, sw, ta, te, th, tl, tr, uk, ur, uz, vi, zh-CN, zh-HK, zh-TW, zu] (size reduction: 351087 bytes).

(When building oppia_beta)

#4667 is included in this PR despite it being an asset downloader-only fix out of convenience & since it's a related issue. The problem in the downloader was that translations were being incorrectly copied over to the new proto structure for lessons (which isn't used in the app yet, but is used for image downloading) which led to non-English images not being scraped from content HTMLs and thus not downloaded and included in the embedded assets for releases. We'll be adding a manual QA test to make sure that non-English content images load correctly.

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

Screenshots showing how English, Brazilian Portuguese, and Swahili are handled with the changes in this PR (between developer and beta builds of the app):

Dev flavor -- with PR changes Beta flavor -- with PR changes
English image image
Brazilian Portuguese image image
Swahili image image

As can be seen from the table, all three languages are fully supported except when using beta with the changes introduced by this PR (where Swahili is dropped since it's not yet a 100% supported, production-ready language for the app). Note that in the case of Swahili, it falls back to English simply because that's how my device was configured at the time (it will fall back to whatever the system-secondary language is set to).

Note that other typical screenshots/tests are not included in this PR, including for Espresso. From a high-level perspective, the main difference in this PR is which app and content strings are ultimately included and supported within the app. Given this is inclusion/exclusion, the screenshots above + manual verification via e2e tests are sufficient. Beyond that, properly testing these changes would require e2e tests that can use the production assets (which current Espresso tests don't have access to).

The filtering criteria is governed by the languages textproto file under
config.
@BenHenning
Copy link
Sponsor Member Author

I'll update the description tomorrow, and include an explanation for why #4667 is included here (technically the PR doesn't fix it, but it provides a means of confirming code-wise when it's done given that it requires changes in a private repository).

The PR is actually ready for review other than missing the description and the extra confirmation for assets, so I'm hoping to get this assigned & wrapped up in the next 1-2 days.

@BenHenning BenHenning requested a review from a team as a code owner November 6, 2022 12:00
@BenHenning BenHenning assigned seanlip and unassigned BenHenning Nov 9, 2022
@BenHenning
Copy link
Sponsor Member Author

@seanlip PTAL (for the whole PR, not just owners). FYI that I fully self-reviewed this PR.

I'm also sending this before the CI runs finish simply because I'm about to go to sleep, but all but 1 test were passing before & I verified it passing locally (so any failures from the latest run are very likely Actions flakes and I'll restart them later).

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.

LGTM -- just one comment, but it might be moot if the existing practice is there because of existing conventions. Thanks for the detailed explanations btw!

@seanlip seanlip assigned BenHenning and unassigned seanlip Nov 9, 2022
@oppiabot oppiabot bot added the PR: LGTM label Nov 9, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 9, 2022

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 BenHenning enabled auto-merge (squash) November 10, 2022 23:13
@BenHenning BenHenning merged commit fea5b7e into develop Nov 11, 2022
@BenHenning BenHenning deleted the filter-unsupported-languages branch November 11, 2022 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants