Closed Bug 1171738 Opened 7 years ago Closed 6 years ago

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

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: coop, Assigned: Callek)

References

Details

Attachments

(1 file)

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
Blocks: 1265595
Assignee: nobody → bugspam.Callek
I'm going to use this, for the 'date' tree level-of-quality work for android/linux64 and linux32
Summary: Migrate Linux64 l10n desktop nightly repacks to TaskCluster → Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster (on date)
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

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-
(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
Attachment #8801114 - Flags: review?(dustin)
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 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)
(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.
Blocks: 1312000
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.