Import strings from multiple branches

[functionzz]

This PR attempts to deprecate the string deprecation process found for fenix, android-components and focus-android.

For background, localization of android strings currently works differently from projects like firefox and enterprise-firefox. Android strings involve 3 seperate projects and the current workflows only copy localization source files directly from the mozilla-firefox/firefox repository main branch. This is undesirable because the workflow does not take into account the release and beta branches, which almost certainly include strings that do not exist on main, which gives rise to the workaround removedIn tag in various localization files.

This tension is resolved in enterprise-firefox, where the strings from the various branches are collected and merged in the order of release -> beta -> main. This results in the inclusion of strings that do not exist in main, whilst allowing firefox teams to remove strings at their own discretion.

This PR copies and applies this behaviour to android projects. A seperate PR is needed to remove the current functionality.

[flodolo]

You should remove the existing workflows and scripts that become obsolete with this change.

[flodolo]

Comments on one of the workflow apply to all the others.

[flodolo]

I only tested fenix locally. update.py overall seems to work, while purge.py has some problems (the comment about working from any path applies also to this file).

The resulting XML file is completely different, we should confirm that we don't lose any string in the process.

[flodolo]

As mentioned, please remove obsolete scripts and workflows as part of this PR.

[flodolo]

One other thing that is missing: this is currently not doing anything for TOML files.
https://github.com/mozilla-l10n/android-l10n/blob/main/.github/scripts/import_strings.py#L145

We should copy them over from the head branch.

https://github.com/mozilla-l10n/firefox-l10n-source/blob/main/.github/scripts/update.py#L97-L98

[flodolo]

I'll go back to the scripts on Monday, and there is no urgency to work on this before that.

Leaving some notes in the meantime:

• Let's create one imported workflow for the main logic, not 3. It makes debugging and updating unnecessarily complicated. For one, you're cloning the l10n repository 3 times.
• Keep the matrix approach, the loop is not readable (you can check the progress of each element of the matrix when the workflow runs, example).
• The workflow is still not committing to the repo when the content of each branch is updated.
• The TOML file is still not copied over.
• Old content (workflows, scripts) is still there.

Please try to address all of these and flag for review when they're all done.

[flodolo]

This fixes the problem with the config, but the script is still writing relative to CWD, which is unreliable (the workflow would literally write in the wrong place, outside of the clone).

[eemeli]

I don't understand how the current working dir is unreliable. We only ever call this script from the repo root. Calling it from elsewhere is undefined behaviour, and it should not be supported.

[flodolo]

The workflow is currently literally calling it from outside the root of the repository. Why shouldn't we make the script predictable, independently of where it's called, given it doesn't require a lot of work?

[eemeli]

Ugh, I'd missed that -- the run step should include a working-directory: l10n as in the original.

[flodolo]

I'm going to make some small changes in a separate commit.

Overall this looks good to me, but there are a few issues with relative paths.

One interesting thing I realized: by just removing 1 string, we end up reformatting the whole file, because the output from moz-l10n is not the same as the one written by hand by developers.

We will need to run a reformatting script across the board to avoid having huge diffs impossible to review later on.

[flodolo]

You can see the reformatting problem in https://github.com/flodolo/android-l10n/pull/1

Only one string changed in mozilla-mobile/android-components/components/feature/addons/src/main/res/values/strings.xml, but the whole file is reformatted.

This should work as a one-off?

#!/usr/bin/env python3

# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

"""
Parse all XML files in the repository with moz-l10n and serialize them back.
"""

from os import pardir
from os.path import abspath, dirname, join
from pathlib import Path

from moz.l10n.resource import parse_resource, serialize_resource


if __name__ == "__main__":
    curr_dir = dirname(abspath(__file__))
    repo_root = abspath(join(curr_dir, pardir, pardir))

    for xml_file in Path(repo_root).rglob("*.xml"):
        path = str(xml_file)
        with open(path, "+rb") as file:
            try:
                resource = parse_resource(path, file.read())
            except Exception as e:
                print(f"skip {xml_file.relative_to(repo_root)}: {e}")
                continue
            file.seek(0)
            for line in serialize_resource(resource):
                file.write(line.encode("utf-8"))
            file.truncate()
        print(f"done {xml_file.relative_to(repo_root)}")

[flodolo]

For the inherit bit
https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows#using-inputs-and-secrets-in-a-reusable-workflow

[flodolo]

This should be ready for @eemeli to review.

[flodolo]

You can see the reformatting problem in flodolo#1

BTW, this also shows that the logic works. The sports suggestions are not available in main, only in beta, while we wait for the fix to be uplifted. The PR correctly tries to add them back.

[eemeli]

A partial review -- didn't look at the pruning yet.

In general, I would recommend against making this many unnecessary changes when essentially copying and adapting a pre-existing implementation; the variable name changes in particular makes reviewing this harder than it needs to be, as it increases the work needed to see what's changed from the original.

[eemeli]

I don't understand how the current working dir is unreliable. We only ever call this script from the repo root. Calling it from elsewhere is undefined behaviour, and it should not be supported.

[flodolo]

In general, I would recommend against making this many unnecessary changes when essentially copying and adapting a pre-existing implementation

It's worth noting that the implementation needs to diverge anyway.

The scripts here need to support multiple projects, and the management of TOML configuration files is much simpler (no replacements necessary).

[eemeli]

You can see the reformatting problem in flodolo#1

Only one string changed in mozilla-mobile/android-components/components/feature/addons/src/main/res/values/strings.xml, but the whole file is reformatted.

This should work as a one-off?

Is there a reason not to use the l10n-fix CLI command provided by moz.l10n?

[eemeli]

In general, I would recommend against making this many unnecessary changes when essentially copying and adapting a pre-existing implementation

It's worth noting that the implementation needs to diverge anyway.

The scripts here need to support multiple projects, and the management of TOML configuration files is much simpler (no replacements necessary).

Diverge, sure; you're descibing necessary changes. My point is about not including unnecessary changes in a first step like this.

[flodolo]

Is there a reason not to use the l10n-fix CLI command provided by moz.l10n?

I just forget it exists ;-)

[functionzz]

Is there a reason not to use the l10n-fix CLI command provided by moz.l10n?

Is adding this to the workflow directly a good idea?

[flodolo]

Is there a reason not to use the l10n-fix CLI command provided by moz.l10n?

Is adding this to the workflow directly a good idea?

No, because update.py will already format en-US, Pontoon will format all the rest.

It's a one-off operation before switching to the new workflows.

[eemeli]

Pruning looks decent, see inline for some relatively small stuff.

[eemeli]

It took me way too long to figure out that this name is wrong, and that this command does not, in fact, run any linter. Instead it appears to be parsing some linter exclusion rules from one representation into another, presumably for use by some subsequent actual linter run.

It's not at all clear to me why this intermediate stage is needed, but rearchitecting this should be beyond the scope of this PR.

Suggested change

	- name: Run linter
	- name: Update linter config

[flodolo]

The linter always had exceptions.

Developers can now add exceptions for usage of brand names directly to the XML. Since all these exceptions are reviewed, these get automatically added to the linter configuration before the linter runs in another workflow.

[eemeli]

Sure; the part I don't understand is the need for this pre-parsing stage. It should be possible to do its work immediately before or during the actual linting, rather than needing to have this separate config file updated as an intermediary asset.

[flodolo]

It is part of the import and reviewed. I would find it more confusing if a linter workflow writes to the repo.

[eemeli]

Ah! Ok, so the reason for this multi-staging is ensuring that an l10n reviewer can notice linter exclusion rule changes, yes? It would be good if that was documented somewhere.

[flodolo]

I'll document somewhere why we have separate PRs and the linter config step in the README.