From 7045e3a9ce3fea18619fb7162b541447b7aba3d1 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 22 Nov 2024 10:59:33 -0800 Subject: [PATCH 1/6] VPN-6749 fix shared addon IDs and add check on PRs --- .github/l10n/check_l10n_issues.py | 110 +++++++++++++++++++ scripts/utils/generate_shared_addon_xliff.py | 12 ++ 2 files changed, 122 insertions(+) diff --git a/.github/l10n/check_l10n_issues.py b/.github/l10n/check_l10n_issues.py index 695cecb9c0..40dd498ef8 100644 --- a/.github/l10n/check_l10n_issues.py +++ b/.github/l10n/check_l10n_issues.py @@ -8,11 +8,121 @@ import xml.etree.ElementTree as etree import json import os +import subprocess import sys +import tempfile script_folder = os.path.abspath(os.path.dirname(__file__)) vpn_root_folder = os.path.realpath(os.path.join(script_folder, os.pardir, os.pardir)) +def fileContentsAsJSON(filepath): + try: + with open(filepath, 'r') as file: + content = json.load(file) + + if not content: + print(f"No content found in: {filepath}") + sys.exit(1) + return content + except FileNotFoundError: + print(f"File not found: {filepath}") + sys.exit(1) + except Exception as e: + print(f"An error occurred while loading {filepath}: {e}") + sys.exit(1) + +# Check if it uses shared strings. If so, pull the value for “title”, “subtitle”, then look within "blocks" +def getSharedStringsInManifest(mainifest_contents): + shared_string_ids = [] + # If it includes the key “message” and if within "message", there is a “usesSharedStrings” key that is true... + if "message" in manifest_contents and "usesSharedStrings" in manifest_contents["message"] and manifest_contents["message"]["usesSharedStrings"]: + #...collect all the string IDs + if "title" in manifest_contents["message"]: + shared_string_ids.append(manifest_contents["message"]["title"]) + else: + print(f"No title found for: {addon}") + sys.exit(1) + if "subtitle" in manifest_contents["message"]: + # It is acceptable if there is no subtitle (but title is required) + shared_string_ids.append(manifest_contents["message"]["subtitle"]) + if not "blocks" in manifest_contents["message"]: + print(f"No blocks found for: {addon}") + sys.exit(1) + for block in manifest_contents["message"]["blocks"]: + if not "content" in block: + print(f"No content found in {addon} for {block}") + sys.exit(1) + if isinstance(block["content"], str): + shared_string_ids.append(block["content"]) + elif isinstance(block["content"], list): + # This is the contents of a bulleted or ordered list; go one layer deeper. + for list_item in block["content"]: + if isinstance(list_item["content"], str): + shared_string_ids.append(list_item["content"]) + else: + print(f"No content found in {addon} for {list_item}") + sys.exit(1) + else: + print(f"Content found in {addon} was unknown type for {block}") + sys.exit(1) + return shared_string_ids + +### 1. Check Addons' shared strings - all string IDs should be present in the translation file. ### +# Find list of all addon folders +addon_path = os.path.join(vpn_root_folder, "addons") +try: + addon_list = [item for item in os.listdir(addon_path) if os.path.isdir(os.path.join(addon_path, item))] +except FileNotFoundError: + print(f"Path not found: {addon_path}") + sys.exit(1) +except Exception as e: + print(f"An error occurred when finding all addons: {e}") + sys.exit(1) + +if len(addon_list) == 0: + print(f"No themes found") + sys.exit(1) + +shared_string_ids = [] + +# For each, open the manifest file and pull out the shared strings +for addon in addon_list: + manifest_path = os.path.join(addon_path, addon, "manifest.json") + manifest_contents = fileContentsAsJSON(manifest_path) + shared_string_ids = shared_string_ids + getSharedStringsInManifest(manifest_contents) + +# Create temporary translation file +tmp_path = tempfile.mkdtemp() +shared_addon_strings = os.path.join(addon_path, "strings.yaml") +shared_addons_tmp_file = os.path.join(tmp_path, "strings.xliff") +generate_addon_python_file = os.path.join(vpn_root_folder, "scripts", "utils", "generate_shared_addon_xliff.py") +p = subprocess.run(['python', generate_addon_python_file, '-i', shared_addon_strings, '-o', shared_addons_tmp_file]) + +# Check that the shared strings are present in the translation file +if not os.path.isfile(shared_addons_tmp_file): + print(f"Unable to find {shared_addons_tmp_file}") + sys.exit(1) +try: + with open(shared_addons_tmp_file, 'r') as file: + shared_string_content = file.read() + if not shared_string_content: + print(f"No content found in: {shared_addons_tmp_file}") + sys.exit(1) +except FileNotFoundError: + print(f"File not found: {shared_addons_tmp_file}") + sys.exit(1) +except Exception as e: + print(f"An error occurred while loading {shared_addons_tmp_file}: {e}") + sys.exit(1) + +for shared_string_id in shared_string_ids: + string_id_unit = "" + if string_id_unit not in shared_string_content: + print(f"{string_id_unit} not found in shared string file") + sys.exit(1) + +### 2. Check all strings for overall issues. ### + # Paths are relative to the root folder paths = [ "translations.ts", diff --git a/scripts/utils/generate_shared_addon_xliff.py b/scripts/utils/generate_shared_addon_xliff.py index b936a5a1f9..56efdc486a 100644 --- a/scripts/utils/generate_shared_addon_xliff.py +++ b/scripts/utils/generate_shared_addon_xliff.py @@ -28,6 +28,16 @@ def prune_lists_to_strings(data): sys.exit("Unexpected input") return data +def use_proper_id(data): + return_data = {} + for value in data.values(): + string_id = value["string_id"] + return_data[string_id] = { + "value": value["value"], + "comments": value["comments"] + } + return return_data + # Make sure we have all the required things # Lookup our required tools for addon generation. @@ -59,6 +69,8 @@ def prune_lists_to_strings(data): print("First, pull in the strings from the YAML file") translation_strings = parseYAMLTranslationStrings(args.infile) translation_strings = prune_lists_to_strings(translation_strings) +# parseYAMLTranslationStrings gives a different ID than we want to use +translation_strings = use_proper_id(translation_strings) print("Then, write the strings to a .ts file") tmp_path = tempfile.mkdtemp() From 9639a66ef2d78ed8e0447ad2693153f6cebb8118 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 22 Nov 2024 11:19:22 -0800 Subject: [PATCH 2/6] run translations when building addons --- addons/CMakeLists.txt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/addons/CMakeLists.txt b/addons/CMakeLists.txt index fecc8b1fa5..9c0bdb9921 100644 --- a/addons/CMakeLists.txt +++ b/addons/CMakeLists.txt @@ -11,6 +11,14 @@ project(addons VERSION 1.0.0 LANGUAGES CXX find_program(PYTHON_EXECUTABLE NAMES python3 python) find_package(Qt6 REQUIRED COMPONENTS Core Qml) +## In case new shared strings were added to ./addons/strings.yaml, run the generation script so actual strings are shown +add_custom_command( + OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/../3rdparty/i18n/en/addons/strings.xliff + DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/strings.yaml + COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/utils/generate_shared_addon_xliff.py + -i ${CMAKE_CURRENT_SOURCE_DIR}/strings.yaml + -o ${CMAKE_CURRENT_SOURCE_DIR}/../3rdparty/i18n/en/addons/strings.xliff) + ## Create the addons target. include(${CMAKE_CURRENT_SOURCE_DIR}/../scripts/cmake/addons.cmake) add_addon_target(addons From e7b825623e7f8d63952961039338462229bff875 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 22 Nov 2024 12:01:41 -0800 Subject: [PATCH 3/6] can't change main directory --- addons/CMakeLists.txt | 8 -------- 1 file changed, 8 deletions(-) diff --git a/addons/CMakeLists.txt b/addons/CMakeLists.txt index 9c0bdb9921..fecc8b1fa5 100644 --- a/addons/CMakeLists.txt +++ b/addons/CMakeLists.txt @@ -11,14 +11,6 @@ project(addons VERSION 1.0.0 LANGUAGES CXX find_program(PYTHON_EXECUTABLE NAMES python3 python) find_package(Qt6 REQUIRED COMPONENTS Core Qml) -## In case new shared strings were added to ./addons/strings.yaml, run the generation script so actual strings are shown -add_custom_command( - OUTPUT ${CMAKE_CURRENT_SOURCE_DIR}/../3rdparty/i18n/en/addons/strings.xliff - DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/strings.yaml - COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/../scripts/utils/generate_shared_addon_xliff.py - -i ${CMAKE_CURRENT_SOURCE_DIR}/strings.yaml - -o ${CMAKE_CURRENT_SOURCE_DIR}/../3rdparty/i18n/en/addons/strings.xliff) - ## Create the addons target. include(${CMAKE_CURRENT_SOURCE_DIR}/../scripts/cmake/addons.cmake) add_addon_target(addons From b6affa1b24d62880e5564ab486966483b74d4f7b Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Fri, 22 Nov 2024 12:09:02 -0800 Subject: [PATCH 4/6] improve documentation --- docs/Components/Addons/index.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/Components/Addons/index.md b/docs/Components/Addons/index.md index 8bd6b430c2..5155f84cad 100644 --- a/docs/Components/Addons/index.md +++ b/docs/Components/Addons/index.md @@ -152,7 +152,8 @@ If you want to implement new add-ons, you need to follow these steps: 1. Create a manifest in a separate folder in the `addons` directory of the mozilla VPN repository. 2. Read and follow the documentation for the add-on type you want to implement. -3. Build the CMake project in the `addons` directory. +3. If new shared strings were added, process them so that strings are shown in the addon: `python ./scripts/utils/generate_shared_addon_xliff.py -i ./addons/strings.yaml -o ./3rdparty/i18n/en/addons/strings.xliff` (This step is not necessary, strictly speaking. This script is what will be run as new strings are ingested into l10n repo, and once the new strings have done a round trip to the l10n repo and then back to the client repo, the string will show up in the addon as expected. Manually running this script as part of the build process prevents the brand new addon from showing "vpn.newStringCategory.newString" until this round trip happens.) +4. Build the CMake project in the `addons` directory. ``` mkdir -p build-addons/ @@ -160,10 +161,10 @@ cmake -S /addons -B build-addons -GNinja cmake --build build-addons ``` -4. Expose the generated build directory through a webservice. For example: `python3 -m http.server --directory build-addons/` -5. Open the dev-menu from the get-help view and set a custom add-on URL: `http://localhost:8000/` -6. Scroll down and disable the signature-addon feature from the dev-menu, list of features -7. Be sure you are doing all of this using a staging environment +5. Expose the generated build directory through a webservice. For example: `python3 -m http.server --directory build-addons/` +6. Open the dev-menu from the get-help view and set a custom add-on URL: `http://localhost:8000/` +7. Scroll down and disable the signature-addon feature from the dev-menu, list of features +8. Be sure you are doing all of this using a staging environment If all has done correctly, you can see the app fetching the manifest.json (and not! the manifest.json.sig) resource from the webservice. From 3334b5d06dae2790aa7d730657d4073afbeb0472 Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Mon, 25 Nov 2024 12:16:36 -0800 Subject: [PATCH 5/6] PR feedback --- .github/l10n/check_l10n_issues.py | 4 ++-- scripts/addon/build.py | 2 +- scripts/shared.py | 7 +++++-- scripts/utils/generate_shared_addon_xliff.py | 14 +------------- 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/.github/l10n/check_l10n_issues.py b/.github/l10n/check_l10n_issues.py index 40dd498ef8..10229b6646 100644 --- a/.github/l10n/check_l10n_issues.py +++ b/.github/l10n/check_l10n_issues.py @@ -35,7 +35,7 @@ def fileContentsAsJSON(filepath): def getSharedStringsInManifest(mainifest_contents): shared_string_ids = [] # If it includes the key “message” and if within "message", there is a “usesSharedStrings” key that is true... - if "message" in manifest_contents and "usesSharedStrings" in manifest_contents["message"] and manifest_contents["message"]["usesSharedStrings"]: + if manifest_contents.get("message", {}).get("usesSharedStrings", False): #...collect all the string IDs if "title" in manifest_contents["message"]: shared_string_ids.append(manifest_contents["message"]["title"]) @@ -80,7 +80,7 @@ def getSharedStringsInManifest(mainifest_contents): sys.exit(1) if len(addon_list) == 0: - print(f"No themes found") + print(f"No addons found") sys.exit(1) shared_string_ids = [] diff --git a/scripts/addon/build.py b/scripts/addon/build.py index b15cb2ad3d..58d7abc88f 100755 --- a/scripts/addon/build.py +++ b/scripts/addon/build.py @@ -461,7 +461,7 @@ def transform_shared_strings(input_file, output_file, relevant_strings, short_ve print("Create localization file...") os.mkdir(os.path.join(tmp_path, "i18n")) template_ts_file = os.path.join(args.dest, f"{manifest['id']}.ts") - write_en_language(template_ts_file, strings) + write_en_language(template_ts_file, strings, True) # This will be probably replaced by the en locale if it exists en_ts_file = os.path.join(tmp_path, "i18n", "locale_en.ts") diff --git a/scripts/shared.py b/scripts/shared.py index 46ea197009..39fa314e1f 100644 --- a/scripts/shared.py +++ b/scripts/shared.py @@ -2,7 +2,7 @@ import sys import xml.etree.ElementTree as ET -def write_en_language(filename, strings): +def write_en_language(filename, strings, key_as_id): ts = ET.Element("TS") ts.set("version", "2.1") ts.set("language", "en") @@ -12,7 +12,10 @@ def write_en_language(filename, strings): for key, value in strings.items(): message = ET.SubElement(context, "message") - message.set("id", key) + if key_as_id: + message.set("id", key) + else: + message.set("id", value["string_id"]) location = ET.SubElement(message, "location") location.set("filename", "addon.qml") diff --git a/scripts/utils/generate_shared_addon_xliff.py b/scripts/utils/generate_shared_addon_xliff.py index 56efdc486a..88ebe0bd51 100644 --- a/scripts/utils/generate_shared_addon_xliff.py +++ b/scripts/utils/generate_shared_addon_xliff.py @@ -28,16 +28,6 @@ def prune_lists_to_strings(data): sys.exit("Unexpected input") return data -def use_proper_id(data): - return_data = {} - for value in data.values(): - string_id = value["string_id"] - return_data[string_id] = { - "value": value["value"], - "comments": value["comments"] - } - return return_data - # Make sure we have all the required things # Lookup our required tools for addon generation. @@ -69,13 +59,11 @@ def use_proper_id(data): print("First, pull in the strings from the YAML file") translation_strings = parseYAMLTranslationStrings(args.infile) translation_strings = prune_lists_to_strings(translation_strings) -# parseYAMLTranslationStrings gives a different ID than we want to use -translation_strings = use_proper_id(translation_strings) print("Then, write the strings to a .ts file") tmp_path = tempfile.mkdtemp() ts_file = os.path.join(tmp_path, "sharedAddonsStrings.ts") -write_en_language(ts_file, translation_strings) +write_en_language(ts_file, translation_strings, False) print("Finally, convert the ts file into an xliff file") os.system(f"{lconvert} -if ts -i {ts_file} -of xlf -o {args.outfile}") From c804db1cfa7e1cafa531801e8e2437cc57403c7b Mon Sep 17 00:00:00 2001 From: Matt Cleinman <9295855+mcleinman@users.noreply.github.com> Date: Mon, 2 Dec 2024 11:04:22 -0500 Subject: [PATCH 6/6] better formatting --- docs/Components/Addons/index.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/Components/Addons/index.md b/docs/Components/Addons/index.md index 5155f84cad..e5a500f356 100644 --- a/docs/Components/Addons/index.md +++ b/docs/Components/Addons/index.md @@ -152,7 +152,13 @@ If you want to implement new add-ons, you need to follow these steps: 1. Create a manifest in a separate folder in the `addons` directory of the mozilla VPN repository. 2. Read and follow the documentation for the add-on type you want to implement. -3. If new shared strings were added, process them so that strings are shown in the addon: `python ./scripts/utils/generate_shared_addon_xliff.py -i ./addons/strings.yaml -o ./3rdparty/i18n/en/addons/strings.xliff` (This step is not necessary, strictly speaking. This script is what will be run as new strings are ingested into l10n repo, and once the new strings have done a round trip to the l10n repo and then back to the client repo, the string will show up in the addon as expected. Manually running this script as part of the build process prevents the brand new addon from showing "vpn.newStringCategory.newString" until this round trip happens.) +3. If new shared strings were added, process them so that strings are shown in the addon: +`python ./scripts/utils/generate_shared_addon_xliff.py -i ./addons/strings.yaml -o ./3rdparty/i18n/en/addons/strings.xliff` +(This step is not necessary, strictly speaking. This script is what will be run as new +strings are ingested into l10n repo, and once the new strings have done a round trip to +the l10n repo and then back to the client repo, the string will show up in the addon as +expected. Manually running this script as part of the build process prevents the brand +new addon from showing "vpn.newStringCategory.newString" until this round trip happens.) 4. Build the CMake project in the `addons` directory. ```