VPN-6763: Do not include shared translations when importing addon-specific ts files

[mcleinman]

Description

When testing, I found that the git branch wasn't properly picked up by this script, and I fixed that. That one line is not part of this bug, but is a fix I included in this PR.

This is 100% egg on my face. While we did extensive testing of the addons shared string work to ensure it extracts shared strings, builds the app with shared strings, and uses those shared strings, I neglected to consider that additionally we do not want those shared strings to be duplicated for translation within the addons that they're utilized in. I take full responsibility for this, with a giant caveat that our use of GitHub actions makes true end-to-end testing of this pipeline extremely difficult.

The duplicated addon translations come from this path:

1. Within the mozilla-vpn-client-l10n repo, the update.yaml github action runs generate_ts.sh
2. generate_ts.sh runs cmake for addons (lines 70-71).
3. /addons/CMakeLists.txt calls ./scripts/cmake/addons.cmake
4. ./scripts/cmake/addons.cmake calls build.py (in lines 64 and 74, depending on which forking logic path is used)
5. build.py ultimately creates the *.ts files for each addon's translations, in addition to the .ts file for the shared strings for addons. Thus, translations for shared strings are duplicated within the addons they're used for.

While generate_ts.sh starts this, we can't just make changes. We need to be cognizant that it is called from 3 places, from what I can tell:

1. update.yaml in the l10n repo (the culprit here)
2. linters.yaml in the client repo
3. translations.yaml in the client repo

In the last two situations, we need the .ts files to be created for all addons, including ones that use shared strings - these are what are used when creating the addons, and they will have the shared strings swapped out for real strings appropriately. (Side note: We need all of this work to be done in scripts and cmake and such - as it currently is - and not the client so that older client versions still properly show an "update your client" addon that happens to use shared strings.)

However in the first situation, we need to skip .ts files for addons that use the shared strings file - otherwise we'll send the strings out for translations too many times - once with the shared string file, and then once again for every addon that those strings are used in.

So, we need some sort of flag to decide whether or not to include .ts files for addons that use shared strings.

• We could take a flag for generate_ts.sh, and pass it down through the entire chain of scripts (see the first list in this PR) until we pass that flag into build.py. This seems very fragile, very brittle, and prone to breakage.
• Alternatively, we could hack something intogenerate_ts.sh to remove the unneeded ts files.

Given these tradeoffs, I went with the latter option. I also made the inclusion of the flag mean that we'll keep all addon .ts files (rather than lack of flag means keep all addons, and including flag means remove the shared addon ones), which has the benefit of not requiring any updates to the translation repo, just our client one.

Of course, both of these options are messy, and our translation pipeline is a giant house of cards | dumpster fire (pick your favorite metaphor). That said, rewriting the entire translation pipeline is a large project, and right now we just need a fix.

And, ultimately, GitHub actions are no easier to test than they were last week. So while I've tested this locally, I can't be quite as confident in this fix as I'd like to be. 🤞🏻

Reference

VPN-6763

Checklist

• My code follows the style guidelines for this project
• I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
• I have performed a self review of my own code
• I have commented my code PARTICULARLY in hard to understand areas
• I have added thorough tests where needed

[flodolo]

So, we need some sort of flag to decide whether or not to include .ts files for addons that use shared strings.

Can an addon have both shared and not shared strings?

[flodolo]

This works for me (tested in the l10n repo, which allows running automation against a branch).

[flodolo]

We actually have another problem, not sure if you want to address it here: looks like the strings for the Firefox extension are duplicated in the shared string file.
mozilla-l10n/mozilla-vpn-client-l10n#498

[flodolo]

Change of plan: the Firefox extension addon is using shared strings, so the file should not be created.

[flodolo]

This is not reliable. The FIrefox extension addon uses shared strings, but no strings with vpn.commonString IDs.
https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/addons/message_try_firefox_extension/manifest.json

Can we use the ts_file to go back to the addon manifest, and check for "usesSharedStrings": true?

[flodolo]

Worth noting that I don't see the code going through message_try_firefox_extension for some reason (not sure if it's because my system is only partially set up).

[flodolo]

Worth noting that I don't see the code going through message_try_firefox_extension for some reason (not sure if it's because my system is only partially set up).

I think this was indeed due to my system.

Something like this is clunky but could work?

if [ $KEEP_ALL_TS_FILES == 0 ]; then
  print Y "Checking for addons using shared strings"
  for ts_file in ./addon_ts/*
  do
    # Use the addon name from the .ts file to build a path to the manifest
    manifest_file="$(pwd)/addons/$(basename $ts_file .ts)/manifest.json"
    if [[ -f $manifest_file ]]; then
      uses_shared_strings=$(jq '.message.usesSharedStrings // false' < $manifest_file)
      if [[ "$uses_shared_strings" == "true" ]]; then
        print G "Deleting file because the addon uses shared strings: $ts_file"
        rm $ts_file
      fi
    else
      print R "Manifest file not found: $manifest_file"
    fi
  done
fi

Not sure why I'm getting an error on Manifest file not found: mozilla-vpn-client/addons/message_update_v2.24/manifest.json. I can see the .ts file being generated, but that addon doesn't exist in /addons 🤔

Never mind, this was in build-addons, coming from a previous attempt with multiple branches enabled. Testing this stuff is a huge PITA.

[mcleinman]

I want to die if the manifest file isn't found, because it should always be there.

To get this working, I had to have similar logic in two spots - doing it once at the start (on the branch the user was on when calling the script), then once again each time we loop through the release branches. I really do not like this pattern, but it's in line with how this file currently handles things - it does a bunch of work on the first branch, then essentially duplicates most of those calls (and adds in calls that combine the various files) for each release branch in the for loop.

[mcleinman]

Good catch, and I'm sorry - my fix neglects a situation where none of the shared strings are a common string.

Agreed testing this is somewhat awful, thanks for all your support with it.

To answer another question: Shared strings for messages are indeed all or nothing. Either all strings come from that file, or we use the old fashioned way. (My intention would be to use only the new shared file moving forward.)

[flodolo]

We can't die here, given the whole "we extract string for addons from all branches". An addon is bound to be missing here and there :-(

See https://github.com/mozilla-l10n/mozilla-vpn-client-l10n/actions/runs/12166789612/job/33933835196

Checking for .ts files using shared strings
Manifest file not found: ./addons/message_try_firefox_extension/manifest.json

[mcleinman]

I discovered this as well, have a fix I'm putting up shortly

[mcleinman]

This is ready for another round of review.