When building webext-langpack pick up language version from the VCS if possible

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
In the new webext-langpacks, we're currently adding a version of the language data in the manifest.json using the same version as the whole langpack.

But the version for each language is meant to be used by L10nRegistry only to sort the priority of sources for a given language.

So, I'd like to switch that to use VCS timestamp of the push to the locale repository from which the locale files were picked, and if that's not possible, use the timestamp of the langpack build.
(Assignee)

Updated

a year ago
Blocks: 1395363
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Assignee)

Comment 2

11 months ago
It seems that Mercurial doesn't store push timestamps, so I used commit timestamp for now.

There seems to be an extension maintained by Mozilla called `pushlog` that adds ability to store pushes, but I failed to make it work, and I'm not sure if we can rely on it.

If we can, it may be additional extension to this patch to check if `pushlog` is activated and attempt to retrieve a timestamp from it.

Updated

11 months ago
Attachment #8906443 - Flags: review?(l10n) → review?(bugspam.Callek)

Comment 3

11 months ago
Redirecting this request to Callek first, as I think the way we do this depends largely on how we want to get the data in automation.

Also, no idea what happens for folks that use git via cinnabar (we do support that for development builds, http://searchfox.org/mozilla-central/source/toolkit/locales/l10n.mk#174)

My ad-hoc idea was to try to find inspiration from how we do buildid, but then I don't understand how we do that ;-)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review183288

I should note that on commit time, or based on the users system commit date can vary wildly. Its entirely possible for someone to commit with "today" then next commit be from 1989, then next commit be from 3017, then a final one from "today" again. As such this won't give you any useful data.

If you are trying to use the pushlog data locally you'll need the pushlog pulling extension enabled at clone/update time, which is probably not easy to guarantee.

Additionally there is a mozversioncontrol python component in tree [1], that I think new python code should utilize for anything like this, :gps can provide guidance on its use.

Lastly, if we are ok with network access, we can get the pushlog data with a query like: https://hg.mozilla.org/l10n-central/de/json-pushes?changeset=8da15aaa152f

Note the url must be an https:// (not an ssh://) and needs to point at the correct l10n repo (l10n-central, releases/* etc) which of course becomes easier once we have cross-channel.

[1] - https://dxr.mozilla.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/__init__.py
Attachment #8906443 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 5

11 months ago
> I should note that on commit time, or based on the users system commit date can vary wildly. Its entirely possible for someone to commit with "today" then next commit be from 1989, then next commit be from 3017, then a final one from "today" again. As such this won't give you any useful data.

So, while I understand the risk here, I believe that blocking ourselves from using commit timestamp because someone can manipulate their time locally is not necessary.

Sure, it means we cannot fully trust the timestamp to be valid, but in this case, we're not depending on it for any security related features. The worst thing that can happen is that an older language data will take priority over newer.

> If you are trying to use the pushlog data locally you'll need the pushlog pulling extension enabled at clone/update time, which is probably not easy to guarantee.

I did enable `pushlog` extension by linking it from my `.hgrc to .mozbuild/version-control-tools/hgext/pushlog, and I cloned https://hg.mozilla.org/l10n-central/pl/ after that, and I was able to do `hg verifypushlog` and it works. But `hg pushlog` gives me an unknown command error.
Not sure why.

My idea was the make it a "best-effort" approach where we try to use the pushlog date, if that fails we try to use the commit date, and if that fails, we'll use the current date.

> Additionally there is a mozversioncontrol python component in tree [1], that I think new python code should utilize for anything like this, :gps can provide guidance on its use.

Thanks, I'll take a look.

> Lastly, if we are ok with network access, we can get the pushlog data with a query like: https://hg.mozilla.org/l10n-central/de/json-pushes?changeset=8da15aaa152f

I would like to avoid having to rely on network access if that's possible, but it certainly is an option.
(Assignee)

Comment 6

11 months ago
> Also, no idea what happens for folks that use git via cinnabar (we do support that for development builds, http://searchfox.org/mozilla-central/source/toolkit/locales/l10n.mk#174)

I would like to consider extending this code to add support for git if needed later. I'm ok with falling back on the current timestamp when we build the product if HG is not available for now.

Comment 7

11 months ago
Callek, should the langpack version be in the decision task in automation?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review187524

::: python/mozbuild/mozbuild/action/langpack_manifest.py:58
(Diff revision 3)
> +#    dt = get_dt_from_hg("/var/vcs/l10n-central/pl")
> +#    dt == datetime(2017, 10, 11, 23, 31, 54, 0)
> +###
> +def get_dt_from_hg(path):
> +    repo_url = subprocess.check_output(
> +        ["hg", "paths", "default"],

This doesn't actually return a URL in all cases. I.e., if you clone via ssh://

Also, with caches in automation, no idea what this boils down to. Also not sure if it would reliably do.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:63
(Diff revision 3)
> +        ["hg", "paths", "default"],
> +        cwd=path
> +    ).strip().decode('ascii')
> +
> +    cs = subprocess.check_output(
> +        ["hg", "id", "-i"],

This should use `hg parent -T'{node|short}\n'.

`id` gives funky results if the working dir is modified, and also, does a stat.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:68
(Diff revision 3)
> +        ["hg", "id", "-i"],
> +        cwd=path
> +    ).strip().decode('ascii')
> +
> +    try:
> +        response = urlopen(pushlog_api_url.format(repo_url, cs))

This should probably have a timeout.

I wonder if this should fail the build in automation?
Attachment #8906443 - Flags: review?(l10n) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
Thanks Axel!

Updated to your review and requesting review from :callek on the rest.

Comment 13

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review187612

For this code in particular, I think having :gps review the hg commands may be worthwhile over me. I was sad to not see a corresponding feature in mozversioncontrol (at first glance).
Attachment #8906443 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 14

11 months ago
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

Thanks! r?gps then
Attachment #8906443 - Flags: review?(gps)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review188340

This is on the right track.

The big areas of feedback are that we're trying to standardize on `requests` and `mozversioncontrol` for HTTP and VCS interactions, respectively.

Strictly speaking, we're also supposed to support Git as a first-class citizen for things consuming VCS. But there is no Git equivalent of the pushlog and this functionality is somewhat niche. So I don't think anyone will complain if Git isn't fully supported here. As long as things don't crash with Git (which they won't with this change).

::: python/mozbuild/mozbuild/action/langpack_manifest.py:27
(Diff revision 4)
> +try:
> +    # For Python 3.0 and later
> +    from urllib.request import urlopen
> +except ImportError:
> +    # Fall back to Python 2's urllib2
> +    from urllib2 import urlopen

We have *requests* vendored in the repo and it is part of the virtualenv. So you should be able to `import requests` here.

(requests is the preferred HTTP client for Python)

::: python/mozbuild/mozbuild/action/langpack_manifest.py:40
(Diff revision 4)
>  def write_file(path, content):
>      with io.open(path, 'w', encoding='utf-8') as out:
>          out.write(content + '\n')
>  
>  
> +pushlog_api_url = "{0}/json-pushes?changeset={1}&version=2&tipsonly=1"

You can query `/json-rev/{revision}` and access the pushlog data there. That's a better API since it is built in to Mercurial.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:57
(Diff revision 4)
> +def get_dt_from_hg(path):
> +    repo_url = subprocess.check_output(
> +        ["hg", "paths", "default"],
> +        cwd=path
> +    ).strip().decode('ascii').replace("ssh", "https")
> +
> +    cs = subprocess.check_output(
> +        ["hg", "parent", "-T'{node|short}'"],
> +        cwd=path
> +    ).strip("'").decode('ascii')

We're trying to standardize on using `mozversioncontrol` for mitigating all VCS interactions.

I'm not asking you to implement any new functionality in `mozversioncontrol`, but could you please use the API for executing `hg` commands? It should look something like:

```
with mozversioncontrol.get_repository_from_env() as repo:
    res = repo._run_in_client(['paths', 'default'])
```

Don't worry about the internal method access. We can fix APIs later.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:59
(Diff revision 4)
> +#    dt = get_dt_from_hg("/var/vcs/l10n-central/pl")
> +#    dt == datetime(2017, 10, 11, 23, 31, 54, 0)
> +###
> +def get_dt_from_hg(path):
> +    repo_url = subprocess.check_output(
> +        ["hg", "paths", "default"],

There's no guarantee a "default" path exists or is the repo you think it is. Ideally the repo URL would be passed in from elsewhere. In automation, many jobs set the repo URL in an environment variable.

That being said, this is probably OK. But it may fail.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:64
(Diff revision 4)
> +        ["hg", "paths", "default"],
> +        cwd=path
> +    ).strip().decode('ascii').replace("ssh", "https")
> +
> +    cs = subprocess.check_output(
> +        ["hg", "parent", "-T'{node|short}'"],

`hg parent` is deprecated.

Also, short nodes should be avoided if possible in the rare case of there being a prefix collision.

Use `hg log -r . -T '{node}'` instead.
Attachment #8906443 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 17

11 months ago
Thank you :gps! Updated the patch to your feedback. Hope it's ready :)

I added `replace("hg", "https")` based on the L10N_BASEDIR schemas that toolkit/locales/l10n.mk provides.

Hope it's a good start to land this and we can extend it if needed.
(Assignee)

Updated

11 months ago
Attachment #8906443 - Flags: review?(bugspam.Callek) → review?(gps)

Comment 18

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review188784

This is much better! Just have a minor nit on argument form and a possible bug around string replacement in URLs.

::: python/mozbuild/mozbuild/action/langpack_manifest.py:54
(Diff revision 5)
> +#    dt == datetime(2017, 10, 11, 23, 31, 54, 0)
> +###
> +def get_dt_from_hg(path):
> +    with mozversioncontrol.get_repository_object(path=path) as repo:
> +        repo_url = repo._run_in_client(["paths", "default"])
> +        repo_url = repo_url.strip().replace("ssh", "https").replace("hg", "https")

What is this 'hg' -> 'https' supposed to do? Won't that rewrite the 'hg' in 'hg.mozilla.org'? Do you mean to only rewrite the protocol part of the URL?

::: python/mozbuild/mozbuild/action/langpack_manifest.py:55
(Diff revision 5)
> +###
> +def get_dt_from_hg(path):
> +    with mozversioncontrol.get_repository_object(path=path) as repo:
> +        repo_url = repo._run_in_client(["paths", "default"])
> +        repo_url = repo_url.strip().replace("ssh", "https").replace("hg", "https")
> +        cs = repo._run_in_client(["log", "-r", ".", "-T'{node}'"]).strip("'")

That's weird spacing/quoting for -T. I'm moderately surprised that works!

Please make the -T and the format string separate arguments.
Attachment #8906443 - Flags: review?(gps) → review-
Comment hidden (mozreview-request)

Comment 20

11 months ago
mozreview-review
Comment on attachment 8906443 [details]
Bug 1395459 - Store the version of the locale data in a langpack based on HG push timestamp.

https://reviewboard.mozilla.org/r/178162/#review189172

Looks good!
Attachment #8906443 - Flags: review?(gps) → review+

Comment 21

11 months ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e73300d22ba5
Store the version of the locale data in a langpack based on HG push timestamp. r=gps

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e73300d22ba5
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.