1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Justin Wood (:Callek)]

In order to support signing l10n repacks, we need to support passing in an explicit list of locales to the task, rather than having mozharness do it.

This is so that we can ensure a matching chunking algorithm and have it happen in one place.

This patch/bug does *not* remove the chunking code from l10n in mozharness, since buildbot still needs it (for now).

Comment 1

[Justin Wood (:Callek)]

Attachment: Bug 1312585 - Vendor chunkify

for - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking

XXX Note: gps disagrees with vendoring it this way, will instead be imported as part of mozbuild/

Review commit: https://reviewboard.mozilla.org/r/88220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88220/

Comment 2

[Justin Wood (:Callek)]

Attachment: Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

Review commit: https://reviewboard.mozilla.org/r/88222/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88222/

Comment 3

[Justin Wood (:Callek)]

Attached is my WIP here, and has a pushed-to-try with nightly-kickoff set of jobs here:

android: https://tools.taskcluster.net/push-inspector/#/atWRV1YwTxWZWVqFwYy2Ow?_k=9ik6pz
desktop: https://tools.taskcluster.net/push-inspector/#/Inmpi33zTGuoOQm9C3XlnA/aZBCAgukQIaai1vdBzOb3Q?_k=ersiqs

*** Noteworthy, I think I discovered an actual fault with multilocale in that it seems to be invoking py2.6 for the locale part, this patch also hopes to confirm that for me :-) ***

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 8804076 [details]
Bug 1312585 - Vendor chunkify

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88220/diff/1-2/

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88222/diff/1-2/

Comment 6

[Justin Wood (:Callek)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88222/diff/2-3/

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 8804076 [details]
Bug 1312585 - Vendor chunkify

https://reviewboard.mozilla.org/r/88220/#review87622

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

https://reviewboard.mozilla.org/r/88222/#review87716

A bunch of comments, but all pretty minor

::: taskcluster/ci/l10n/kind.yml:5
(Diff revision 3)
>  # 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/.
>  
>  # NOTE: please write a description of this kind in taskcluster/docs/kinds.rst

plz? :)

::: taskcluster/ci/l10n/kind.yml:16
(Diff revision 3)
>     - taskgraph.transforms.build_attrs:transforms
>     - taskgraph.transforms.job:transforms
>     - taskgraph.transforms.task:transforms
>  
>  job-defaults:
> +    locales_file: browser/locales/all-locales

use `-`

::: taskcluster/taskgraph/transforms/l10n.py:24
(Diff revision 3)
> +    """ Parse the passed locales file for a list of locales.
> +        If platform is unset matches all platforms.
> +    """
> +    locales = []
> +    if locales_file.endswith('json'):
> +        raise NotImplementedError("Release Process stuff seems to use this path")

What does this mean?

::: taskcluster/taskgraph/transforms/l10n.py:47
(Diff revision 3)
> +        # XXX Ignore based on platform, but for now, ignore mac
> +        locales = locales - set(("ja-JP-mac", ))

Why is this here?  If this is a common thing, yeah, let's spend a few lines of code finding a convenient way to do it.

::: taskcluster/taskgraph/transforms/l10n.py:52
(Diff revision 3)
> +        # XXX Ignore based on platform, but for now, ignore mac
> +        locales = locales - set(("ja-JP-mac", ))
> +        # Convert to mutable list.
> +        locales = [locale for locale in sorted(locales)]
> +        attributes = job.get('attributes', {})
> +        attributes["all_locales"] = locales

The `_` is right here, but please add this attribute to `taskcluster/docs/attributes.rst`.

::: taskcluster/taskgraph/transforms/l10n.py:79
(Diff revision 3)
>                  chunked['attributes']['l10n_chunk'] = this_chunk
> +                chunked['attributes']['chunk_locales'] = my_locales

please doc these attributes as well

::: testing/mozharness/mozharness/mozilla/l10n/locales.py:70
(Diff revision 3)
> -        if not locales and c.get("locales", None):
> +        print("XXXDEBUG_CALLEK: %s" % sys.version)
> +        print("XXXDEBUG_CALLEK: %s" % repr(c.get("locales", "")))

remove debug prints

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

https://reviewboard.mozilla.org/r/88222/#review87716

> What does this mean?

There are two formats of locale files, none seem to be used for m-c nightlies, but the alternate format does seem to be used in the release process for now. This is primarily so that if someone switches this or adds to it, we fail loudly here rather than confusingly due to parsing things as locales that are not. I've adjusted the commentary here.

> Why is this here?  If this is a common thing, yeah, let's spend a few lines of code finding a convenient way to do it.

This is a common thing for this locale only, its usually explicitly written out to ignore. my XXX is to be better about excluding it for when we do get ready to ship osx. But since nothing we're doing here *now* needs ja-JP-mac we don't need to worry.

https://dxr.mozilla.org/mozilla-central/search?q=ja-JP-mac&redirect=false

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 8804076 [details]
Bug 1312585 - Vendor chunkify

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88220/diff/2-3/

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88222/diff/3-4/

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

https://reviewboard.mozilla.org/r/88222/#review88478

All minor notes -- this looks good!

::: taskcluster/docs/kinds.rst:39
(Diff revision 4)
> +
> +nightly-l10n
> +------------
> +
> +The nightly l10n kind repacks a specific nightly build (from the same source code)
> +In order to provide localized versions of the same source.

lowercase `i`

::: taskcluster/taskgraph/transforms/l10n.py:48
(Diff revision 4)
>  
>  @transforms.add
> -def chunkify(config, jobs):
> +def all_locales_attribute(config, jobs):
> +    for job in jobs:
> +        locales = set(_parse_locales_file(job["locales-file"]))
> +        # XXX Ignore based on platform, but for now, ignore mac

I think I get the explanation, but I dislike landing something with an "# XXX" comment.  Perhaps

    # ja-JP-mac is a mac-only platform, but there
    # are no mac builds being repacked, so just omit
    # it unconditionally

::: taskcluster/taskgraph/transforms/l10n.py:51
(Diff revision 4)
> +    for job in jobs:
> +        locales = set(_parse_locales_file(job["locales-file"]))
> +        # XXX Ignore based on platform, but for now, ignore mac
> +        locales = locales - set(("ja-JP-mac", ))
> +        # Convert to mutable list.
> +        locales = [locale for locale in sorted(locales)]

locales = list(sorted(locales))

::: taskcluster/taskgraph/transforms/l10n.py:55
(Diff revision 4)
> +        # Convert to mutable list.
> +        locales = [locale for locale in sorted(locales)]
> +        attributes = job.get('attributes', {})
> +        attributes["all_locales"] = locales
> +        job["attributes"] = attributes
> +        if 'locales-file' in job:

you referred to `job["locales-file"]` 10 lines ago, so no need for this conditional

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8804076 [details]
Bug 1312585 - Vendor chunkify

https://reviewboard.mozilla.org/r/88220/#review88512

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 8804077 [details]
Bug 1312585 - Set l10n repacks to pass in explicit list of locales, rather than having mozharness do chunking.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88222/diff/4-5/

Comment 15

[Justin Wood (:Callek)]

  https://hg.mozilla.org/projects/date/rev/b0a0539d9d55b3b54fd024afbadbf687616a9443
  https://hg.mozilla.org/projects/date/rev/7cf267b61752e3c5881cea780f91d06f902fae3e