Closed
Bug 1171738
Opened 10 years ago
Closed 8 years ago
Migrate Linux64, Linux32, and Android l10n desktop nightly repacks to TaskCluster (on date)
Categories
(Release Engineering :: General, defect)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: coop, Assigned: Callek)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
Details |
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
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugspam.Callek
Assignee | ||
Comment 1•8 years ago
|
||
I'm going to use this, for the 'date' tree level-of-quality work for android/linux64 and linux32
Assignee | ||
Updated•8 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•8 years ago
|
Attachment #8801114 -
Flags: feedback?(dustin)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Justin Wood (:Callek) from comment #2)
> Created attachment 8801114 [details]
> Bug 1171738 - Migrate Linux64, Linux32, and Android l10n desktop nightly
> repacks to TaskCluster. f=dustin
>
> Review commit: https://reviewboard.mozilla.org/r/85910/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/85910/
Fennec: https://tools.taskcluster.net/push-inspector/#/DzPAru3fQkGC4x7y491YCw/DzPAru3fQkGC4x7y491YCw?_k=gytefz
Desktop: https://tools.taskcluster.net/push-inspector/#/DzPAru3fQkGC4x7y491YCw/DzPAru3fQkGC4x7y491YCw?_k=gytefz
Assignee | ||
Comment 4•8 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•8 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•8 years ago
|
Attachment #8801114 -
Flags: review?(dustin)
Comment 7•8 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•8 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)
Assignee | ||
Comment 10•8 years ago
|
||
fwiw this latest is pushed to try and nightlies kicked off with:
https://tools.taskcluster.net/push-inspector/#/SkuiHnQbTa-zfeb7cRzsyQ/
https://tools.taskcluster.net/push-inspector/#/aRBMsX3XQgaQfHmOgAFNRA/
Comment 11•8 years ago
|
||
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•8 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•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•