Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster (on date)

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
3 years ago
15 days ago

People

(Reporter: coop, Assigned: Callek)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

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

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
These repacks use the desktop_l10n.py mozharness script:

https://hg.mozilla.org/build/mozharness/file/13239bfb8b61/scripts/desktop_l10n.py

/tools/buildbot/bin/python scripts/scripts/desktop_l10n.py --branch-config single_locale/mozilla-central.py --platform-config single_locale/linux64.py --environment-config single_locale/production.py --balrog-config balrog/production.py --total-chunks 3 --this-chunk 3

As seen in: http://buildbot-master72.bb.releng.usw2.mozilla.com:8001/builders/Firefox%20mozilla-central%20linux64%20l10n%20nightly-3/builds/0
(Reporter)

Updated

2 years ago
Blocks: 1265595
(Assignee)

Updated

2 years ago
Assignee: nobody → bugspam.Callek
(Assignee)

Comment 1

2 years ago
I'm going to use this, for the 'date' tree level-of-quality work for android/linux64 and linux32
(Assignee)

Updated

2 years ago
Summary: Migrate Linux64 l10n desktop nightly repacks to TaskCluster → Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster (on date)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8801114 - Flags: feedback?(dustin)
(Assignee)

Comment 4

2 years ago
Comment on attachment 8801114 [details]
Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster. f=dustin

self f- (but dustin's feedback still welcome)

3 Issues:

* EN_US_PACKAGE_NAME needs to be passed via mozharness as well
* EN_US_BINARY_URL is not a task-reference, so is not expanding
* `date` configs missing
Attachment #8801114 - Flags: feedback-
(Assignee)

Comment 5

2 years ago
(In reply to Justin Wood (:Callek) from comment #4)
> Comment on attachment 8801114 [details]
> 3 Issues:
> 
> * EN_US_PACKAGE_NAME needs to be passed via mozharness as well

This was wrong, it doesn't need an explicit MH reference (my own test was using a trailing path sep which caused wget to 404, and wasn't related to this var which was working fine)

> * EN_US_BINARY_URL is not a task-reference, so is not expanding

fixed locally.

> * `date` configs missing

Working on this now and will submit a new mozreview for it in a moment
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8801114 - Flags: review?(dustin)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8801114 [details]
Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster. f=dustin

https://reviewboard.mozilla.org/r/85910/#review85048

I think my biggest concern here is with the repetitiveness of the task definition.  This kind of job should be generated automatically, one-on-one, for each nightly build job, without copy/pasting large blocks of YAML.

::: taskcluster/ci/nightly-l10n/kind.yml:5
(Diff revision 2)
> +# 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

Please don't add more such TODOs.

Based on the comments this file is clearly a modified copy of `l10n/kind.yml`.  In general, when you find yourself copy/pasting entire files, the result is not DRY, and you should look for a more efficient way to represent what you're doing.

It would be better to fix the l10n kind, rather than build on top of it.

::: taskcluster/ci/nightly-l10n/kind.yml:40
(Diff revision 2)
> +        job-script: taskcluster/scripts/builder/build-l10n.sh
> +    chunks: 6
> +
> +jobs:
> +    # Names correspond to the <platform> nightly label (as used in signing)
> +    # In order to automatically set the dependency.

This file looks really redundant -- basically the same thing over and over.  Better to generate the task definitions through transforms based on information about signed builds that already exist in the task graph, similar to what kim has done with the signing tasks.

::: taskcluster/taskgraph/transforms/build_attrs.py:28
(Diff revision 2)
> -        attributes.update({
> -            'build_platform': build_platform,
> -            'build_type': build_type,
> -        })
> +        attributes.update({'build_type': build_type})
> +        if 'build_platform' not in attributes:
> +            # Allow some jobs to specify build_platform differently
> +            attributes.update({'build_platform': build_platform})

Why is this required?

::: taskcluster/taskgraph/transforms/l10n.py:19
(Diff revision 2)
>  transforms = TransformSequence()
>  
>  
>  @transforms.add
> +def setup_nightly_dependancy(config, jobs):
> +    """ Sets up a task dependancy to the signing job this relates to """

"dependency" :)

::: taskcluster/taskgraph/transforms/l10n.py:21
(Diff revision 2)
> +        if 'nightly' not in config.kind:
> +            yield job
> +            continue  # do not add a dep unless we're a nightly

This is a little weird.  First, I dislike substring matching on names of things, as it introduces difficult-to-find dependencies on the formats of those names.  Second, wouldn't a "regular" l10n job also have a dependency on the thing it should sign?
Attachment #8801114 - Flags: review?(dustin) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
Comment on attachment 8801114 [details]
Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster. f=dustin

Remember this is "is this ok to land on date" and not a "full review for m-c" (the latter will get its own cycle)
Attachment #8801114 - Flags: feedback?(dustin)
Comment on attachment 8801114 [details]
Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster. f=dustin

My feedback remains unchanged (even the spelling error)

I appreciate the need to move quickly, but experience shows that it's *harder* to fix bugs after they've been added to a codebase than before.  This is increasing the difficulty of review when the time comes to merge from date, and the amount of work required to respond to that review.  And it's increasing that difficulty at a superlinear rate.
Attachment #8801114 - Flags: feedback?(dustin)
(Assignee)

Comment 12

2 years ago
(In reply to Dustin J. Mitchell [:dustin] from comment #11)
> Comment on attachment 8801114 [details]
> Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly
> repacks to TaskCluster. f=dustin
> 
> My feedback remains unchanged (even the spelling error)
> 
> I appreciate the need to move quickly, but experience shows that it's
> *harder* to fix bugs after they've been added to a codebase than before. 
> This is increasing the difficulty of review when the time comes to merge
> from date, and the amount of work required to respond to that review.  And
> it's increasing that difficulty at a superlinear rate.

Hashed it out in IRC -- many of those issues are indeed blockers for "land on central" but this patch can proceed to land on date.

I still plan to iterate on this design on date, but if this lands first I can then also iterate on signing/balrog/beetmover at the same time.
(Assignee)

Updated

2 years ago
Blocks: 1312000
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.