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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Dec 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e93ced
first draft
mcleinman Dec 3, 2024
2cd126c
fix branch name
mcleinman Dec 3, 2024
40d76b0
use flag for generate_ts
mcleinman Dec 3, 2024
b74807e
better shared strings
mcleinman Dec 4, 2024
046e606
move into existing loop
mcleinman Dec 4, 2024
7b54d41
Add for first round
mcleinman Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/linters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:

- name: Generate updated .ts files and check for l10n errors
run: |
./scripts/utils/generate_ts.sh
./scripts/utils/generate_ts.sh -a
python .github/l10n/check_l10n_issues.py

- name: Check for QRC errors
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/translations.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ jobs:

- name: Generating translations
run: |
./scripts/utils/generate_ts.sh
./scripts/utils/generate_ts.sh -a

- name: Uploading
uses: actions/upload-artifact@v4
Expand Down
33 changes: 31 additions & 2 deletions scripts/utils/generate_ts.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,26 @@
print N "This script generates 'ts' files for the Mozilla VPN app."
print N ""

# When the `-a`/`--all` flag is present, .ts files are created
# for every addon.
# Without an argument of `-a` or `--all`, .ts files will NOT
# be created for addons that use the shared strings file.
KEEP_ALL_TS_FILES=0
# Parse Script arguments
while [[ $# -gt 0 ]]; do
key="$1"
case $key in
(-a | --all)
KEEP_ALL_TS_FILES=1
shift
;;
esac
done

cd $(dirname $0)/../.. || die

printn Y "Branch name: "
BRANCHNAME="$(git symbolic-ref HEAD 2>/dev/null)"
BRANCHNAME=${BRANCHNAME##refs/heads/}
BRANCHNAME="$(git rev-parse --abbrev-ref HEAD 2>/dev/null)"
print G "$BRANCHNAME"

printn Y "Caching the dep scripts... "
Expand Down Expand Up @@ -113,6 +128,20 @@ for branch in $(git branch -r | grep origin/releases); do
done
done

# When creating translation files for l10n repo, remove any addon-specific files that use
# shared strings, as they are translated via `strings.yaml`, which is set up for translation in
# `build.py` (which like the addon ts files, is created above when addon's cmake calls `build.py`)"
if [ $KEEP_ALL_TS_FILES == 0 ]; then
print Y "Checking for .ts files using shared strings"
for ts_file in ./addon_ts/*
do
if grep vpn.commonString $ts_file > /dev/null; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@flodolo flodolo Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@flodolo flodolo Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

print G "Deleting file because found shared strings: $ts_file"
rm $ts_file
fi
done
fi

if [ "$BRANCHNAME" ]; then
printn Y "Go back to the initial branch... "
git checkout "$BRANCHNAME" &>/dev/null || die
Expand Down
Loading