I'm going to start stuffing some patches here via mozreview, none of them are needed for r+ yet, since I hope to have confidence in most of the set before landing.

Comment on attachment 8816760 [details] Bug 1322041 - Vendor chunkify https://reviewboard.mozilla.org/r/97330/#review98236

Comment on attachment 8816761 [details] Bug 1322041 - Support android x86, arm; and desktop linux, linux64 nightlies via Taskcluster. Unsigned. https://reviewboard.mozilla.org/r/97332/#review98374

Comment on attachment 8816762 [details] Bug 1322041 - Upload nightly symbols. https://reviewboard.mozilla.org/r/97334/#review98376 I don't understand this, so no r+ yet :) ::: taskcluster/taskgraph/transforms/upload_symbols.py:36 (Diff revision 6) > + if 'android' in build_platform: > + treeherder.setdefault('platform', "{}/opt".format("android-4-0-armv7-api15")) > + elif build_platform == "linux-nightly": > + treeherder.setdefault('platform', "{}/opt".format("linux32")) > + else: > + treeherder.setdefault('platform', > + "{}/opt".format(build_platform).replace("-nightly", "")) Can you add some comments on why the special-cases are required here? It looks like the first makes all android platforms appear as "android-4.0-armv6-api15", which seems like it might be problematic. The second seems to be translating linux -> linux32?

(In reply to Dustin J. Mitchell [:dustin] from comment #54) > Comment on attachment 8816762 [details] > Bug 1322041 - Upload nightly symbols. > > https://reviewboard.mozilla.org/r/97334/#review98376 > > I don't understand this, so no r+ yet :) > > ::: taskcluster/taskgraph/transforms/upload_symbols.py:36 > (Diff revision 6) > > + if 'android' in build_platform: > > + treeherder.setdefault('platform', "{}/opt".format("android-4-0-armv7-api15")) > > + elif build_platform == "linux-nightly": > > + treeherder.setdefault('platform', "{}/opt".format("linux32")) > > + else: > > + treeherder.setdefault('platform', > > + "{}/opt".format(build_platform).replace("-nightly", "")) > > Can you add some comments on why the special-cases are required here? It > looks like the first makes all android platforms appear as > "android-4.0-armv6-api15", which seems like it might be problematic. The > second seems to be translating linux -> linux32? n-i to kmoir for the added details... but I *think* this was our way of "make upload symbols appear on treeherder on the correct lines"

Comment on attachment 8816856 [details] Bug 1322041 - Add signing support for all nightlies. https://reviewboard.mozilla.org/r/97378/#review98378 Pretty minor notes, although enough to warrant a re-run through try ::: taskcluster/taskgraph/task/signing.py:15 (Diff revision 5) > + We use a dictionary to create the input to the transforms. > + It will have added to it keys `build-label`, the label for the build task, > + and `build-platform` / `build-type`, its platform and type. It looks like this isn't accurate anymore ::: taskcluster/taskgraph/transforms/build_signing.py:61 (Diff revision 5) > + > + symbol = 'Ns' > + group = 'tc' > + > + job['treeherder'] = { > + 'symbol': join_symbol(group, symbol), You could hard-code this as `'symbol': 'tc(Ns)'` to save a few lines (not a big deal, but the `join_symbol` function isn't magic like `os.path.join`, it's just a utility). Indeed, that's the default set in `signing.py`, so perhaps this isn't needed at all ::: taskcluster/taskgraph/transforms/signing.py:55 (Diff revision 5) > + # Formats to use to sign the artifacts > + Required('signing-formats'): [basestring], This is redundant - it can be calculated in `signing.py` based on the union of `formats` from all `upstream-artifacts`. ::: taskcluster/taskgraph/transforms/signing.py:94 (Diff revision 5) > + 'treeherder', {}).get('machine', {}).get('platform', '') > + treeherder.setdefault('platform', "{}/opt".format(dep_th_platform)) > + treeherder.setdefault('tier', 2) > + treeherder.setdefault('kind', 'build') > + > + label = job.get('label', "{}-signing".format(dep_job)) I think you want `...format(dep_job.label)`!

Comment on attachment 8816762 [details] Bug 1322041 - Upload nightly symbols. https://reviewboard.mozilla.org/r/97334/#review98584

Comment on attachment 8816857 [details] Bug 1322041 - Add Docs for L10n and add locales_file parsing. https://reviewboard.mozilla.org/r/97380/#review98588 ::: taskcluster/ci/l10n/kind.yml (Diff revision 6) > # 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 thanks!! ::: taskcluster/taskgraph/transforms/l10n.py:42 (Diff revision 6) > + # ja-JP-mac is a mac-only locale, but there are no > + # mac builds being repacked, so just omit it unconditionally > + locales = locales - set(("ja-JP-mac", )) > + # Convert to mutable list. > + locales = list(sorted(locales)) > + attributes = job.get('attributes', {}) if you use attributes = job.setdefault('attributes', {}) then you can omit the reassignment to `job["attributes"]` below

Comment on attachment 8816856 [details] Bug 1322041 - Add signing support for all nightlies. https://reviewboard.mozilla.org/r/97378/#review98600

re my needinfo, I have opened bug 1323485 for this issue and attached a patch for review.

Comment on attachment 8816861 [details] Bug 1322041 - Add date configs to land on central. https://reviewboard.mozilla.org/r/97388/#review99018

Comment on attachment 8817508 [details] Bug 1322041 - Add changes necessary for tc-nightly to m-c files. https://reviewboard.mozilla.org/r/97762/#review99020 ::: testing/mozharness/configs/single_locale/mozilla-central_android-api-15.py:39 (Diff revision 5) > "branch": "default", > + "dest": "tools", > + }, { > + "vcs": "hg", > + "repo": "https://hg.mozilla.org/mozilla-central", > + "revision": "%(revision)s", so these changes will go live on next bbot nightly if landed on central? Why were we not using revision before?

Comment on attachment 8818066 [details] Bug 1322041 - Support SIMPLE_NAMES in a forced-way (via mozharness) for l10n. https://reviewboard.mozilla.org/r/98172/#review99022 r- until discussed or repatched against in-line issue ::: testing/mozharness/scripts/desktop_l10n.py:806 (Diff revision 2) > ret = SUCCESS > else: > self.error('failed to upload %s' % locale) > ret = FAILURE > + > + # XXX Move the files to a SIMPLE_NAME format until we can enable this seems really fragile and I fear we will forget about it prior to it biting. Also, if ret==FAILURE, should we continue with this renaming? Maby this logic should be ripped out into a more static helper like we have for other filename conversions: e.g. https://dxr.mozilla.org/build-central/source/tools/lib/python/release/platforms.py#76

Comment on attachment 8817508 [details] Bug 1322041 - Add changes necessary for tc-nightly to m-c files. https://reviewboard.mozilla.org/r/97762/#review99020 > so these changes will go live on next bbot nightly if landed on central? Why were we not using revision before? Yes, they will go live on next bbot nightly once landed on central, that is ok! We were not using revision before due to legacy process, mostly. Repacks used to clone tip of repo (well still sorta do on central) and then run their stuff, finally updating to the central rev of the nightly they pull down. When we switched to mozharness we matched that behavior, and kept it even when we moved to in-tree via archiver. This issue has also been a cause of some of the bustages on central over the last year, so is a think thats been desired to fix.

Comment on attachment 8818066 [details] Bug 1322041 - Support SIMPLE_NAMES in a forced-way (via mozharness) for l10n. https://reviewboard.mozilla.org/r/98172/#review99022 > this seems really fragile and I fear we will forget about it prior to it biting. > > Also, if ret==FAILURE, should we continue with this renaming? Maby this logic should be ripped out into a more static helper like we have for other filename conversions: e.g. https://dxr.mozilla.org/build-central/source/tools/lib/python/release/platforms.py#76 Sadly, yes its VERY fragile. But also sadly its needed until we can fix the build system to support simple names (something I *do* intend to do). It will also need touching for other platforms (thats ok, I think) -- in the original bug 1313662, https://hg.mozilla.org/projects/date/rev/92f531d1cd77 aki helped fix the process to have error conditions if, e.g. we duplicate a move. I'm not sure a helper method like the platforms.py in tools is a good fit here for such a short-lived (hopefully) piece of code. If we need to discuss, please ping me today.

Comment on attachment 8816762 [details] Bug 1322041 - Upload nightly symbols. https://reviewboard.mozilla.org/r/97334/#review98376 > Can you add some comments on why the special-cases are required here? It looks like the first makes all android platforms appear as "android-4.0-armv6-api15", which seems like it might be problematic. The second seems to be translating linux -> linux32? Fixed in diff for 7-8 from a patch kim landed on date.

Comment on attachment 8818066 [details] Bug 1322041 - Support SIMPLE_NAMES in a forced-way (via mozharness) for l10n. https://reviewboard.mozilla.org/r/98172/#review99022 > Sadly, yes its VERY fragile. But also sadly its needed until we can fix the build system to support simple names (something I *do* intend to do). > > It will also need touching for other platforms (thats ok, I think) -- in the original bug 1313662, https://hg.mozilla.org/projects/date/rev/92f531d1cd77 aki helped fix the process to have error conditions if, e.g. we duplicate a move. > > I'm not sure a helper method like the platforms.py in tools is a good fit here for such a short-lived (hopefully) piece of code. > > If we need to discuss, please ping me today. discussed over irc. comfortable dropping once the following is addressed: <jlund> Jordan Lund Callek: okay, looking at old bug this was already seen and discussed 13:06:28 only question remaining in my issue was "Also, if ret==FAILURE, should we continue with this renaming?" — Callek looks closer 13:11:46 <Callek> ...ok I think you're right that we should return early in FAILURE

Comment on attachment 8816762 [details] Bug 1322041 - Upload nightly symbols. https://reviewboard.mozilla.org/r/97334/#review99946 ::: taskcluster/taskgraph/transforms/upload_symbols.py:34 (Diff revisions 7 - 8) > treeherder = task.get('treeherder', {}) > treeherder.setdefault('symbol', 'tc(Sym)') > - if 'android' in build_platform: > + if 'android-api-15' in build_platform: > treeherder.setdefault('platform', "{}/opt".format("android-4-0-armv7-api15")) > + elif 'android-x86' in build_platform: > + treeherder.setdefault('platform', "{}/opt".format("android-4-2-x86")) > elif build_platform == "linux-nightly": > treeherder.setdefault('platform', "{}/opt".format("linux32")) > else: > treeherder.setdefault('platform', > "{}/opt".format(build_platform).replace("-nightly", "")) > treeherder.setdefault('tier', 2) > treeherder.setdefault('kind', 'build') > task['treeherder'] = treeherder I think all of this is basically re-implementing the config in `taskcluster/ci/build/android.yml`: android-api-15/debug: ... treeherder: platform: android-4-0-armv7-api15/debug ... android-x86/opt: ... treeherder: platform: android-4-2-x86/opt Along with bits of the config in `linux.yml` I suppose. Instead, why not just modify `post_build.py` to pass the whole build task, and copy its `extra.treeherder` over verbatim, changing just the symbol?

Comment on attachment 8816856 [details] Bug 1322041 - Add signing support for all nightlies. https://reviewboard.mozilla.org/r/97378/#review99948

Comment on attachment 8816858 [details] Bug 1322041 - Nightly l10n support, and docs. https://reviewboard.mozilla.org/r/97382/#review99958 It looks like most of my concerns in bug 1171738 comment 7 are still relevant here. In particular, this is repetitive and its differences from the `l10n` kind are not very clear. Let me try to break it down based on what I understand. Any misunderstandings here should be treated as highlighting things that are unclear. Fundamentally, both kinds are running mozharness tasks which take an en_US build and repack for a bunch of locales. The `l10n` kind uses the latest nightly as its input, rather than the current build, and finds that latest nightly through the index. The `nightly-l10n` kind depends on a signing operation and repacks the signed build. Both can be configured with chunks. The mozharness invocations are "messy" and have lots of project- and platform-specific bits in them. The "Xxx Description" model seems to be working pretty well. Let's define a "Localization Description" that specifies * number of chunks * `build-to-repack: latest-nightly` or `build-to-repack: {task-label: "...", artifact-name: "..."}` to specify what to repack (automatically adding a dependency for the second option) * just enough platform information to figure out which style of mozharness invocation to use (android-style or linux-style) Then `taskcluster/taskgraph/transforms/l10n.py` would transform that localization description into a job description. Then the `l10n` kind can use TransformTask and enumerate all of the localization descriptions it should perform. The `nightly-l10n` kind can use a subclass of TransformTask with `get_inputs` set up to scan for `build-signing` tasks and create a localization description for each one. Its `kind.yml` would be pretty small, then -- probably just containing configuration for the number of chunks for different platforms, maybe the mozharness config name for each platform (yay, another set of names for platforms!), and a few other minor bits. This has the advantage of separating the implementation of these l10n tasks, with all of the mozharness config and so on, from the specification for the tasks. ::: taskcluster/docs/kinds.rst:42 (Diff revision 8) > +The nightly l10n kind repacks a specific nightly build (from the same source code) > +in order to provide localized versions of the same source. This answers my "why" in bug 1171738 but leads to the followon question, "why?!" So I see why they are different now -- `l10n` is a localization check that can run on push, while `nightly-l10n` is an l10n repack of an artifact from a build tasks. I wonder if those could be renamed to `l10n-check` and `l10n-repack`, respectively? ::: taskcluster/taskgraph/transforms/build_attrs.py:28 (Diff revision 8) > - 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? Shouldn't this be generated from the job key name in `kind.yml` ::: taskcluster/taskgraph/transforms/l10n.py:34 (Diff revision 8) > +def setup_nightly_dependency(config, jobs): > + """ Sets up a task dependency to the signing job this relates to """ > + for job in jobs: > + if 'nightly' not in config.kind: > + yield job > + continue # do not add a dep unless we're a nightly > + dep_label = "build-{}".format(job['name']) > + job['dependencies'] = {'unsigned-build': dep_label} > + yield job This still seems really weird to me. In fact, I initially commented on it in this review without remembering that I had commented on it in bug 1171738, so it's nice to see that I'm consistent, at least :) I'd like to have this dependency information visible somehow in the yaml. ::: taskcluster/taskgraph/transforms/l10n.py:41 (Diff revision 8) > + for job in jobs: > + if 'nightly' not in config.kind: > + yield job > + continue # do not add a dep unless we're a nightly > + dep_label = "build-{}".format(job['name']) > + job['dependencies'] = {'unsigned-build': dep_label} This kind depends on the `build-signing` kind, so is this really an `unsigned-build`?

Comment on attachment 8816859 [details] Bug 1322041 - Add chain of trust to l10n jobs. https://reviewboard.mozilla.org/r/97384/#review99970 ::: taskcluster/taskgraph/transforms/l10n.py:138 (Diff revision 8) > + job['extra']['chainOfTrust']['inputs']['docker-image'] = { > + "task-reference": "<docker-image>" > + } what about `unsigned-build`?

Comment on attachment 8816860 [details] Bug 1322041 - Sign nightly l10n jobs. https://reviewboard.mozilla.org/r/97386/#review99972 I pretty much like this except for the kind name. I'd like `l10n-repack-signing` to parallel the (renamed) `l10n-repack` kind. ::: taskcluster/taskgraph/task/signing.py:18 (Diff revision 9) > - if (config.get('kind-dependencies', []) != ["build"]): > + if (config.get('kind-dependencies', []) != ["build"] and > + config.get('kind-dependencies', []) != ["nightly-l10n"]): > raise Exception("Signing kinds must depend on builds") ..or `nightly-l10n`s (this might make more sense if the kind was named `l10n-repack`) ::: taskcluster/taskgraph/transforms/nightly_l10n_signing.py:63 (Diff revision 9) > + symbol = 'Ns{}'.format(dep_job.attributes.get('l10n_chunk')) > + group = 'tc-L10n' > + > + job['treeherder'] = { > + 'symbol': join_symbol(group, symbol), > + } You can simplify this to job['treeherder'] = { 'symbol': 'tc-L10n(Ns{})'.format(..), }

(In reply to Dustin J. Mitchell [:dustin] from comment #102) > Comment on attachment 8816762 [details] > Bug 1322041 - Upload nightly symbols. > > https://reviewboard.mozilla.org/r/97334/#review99946 > Filed Bug 1324763 for this.

(In reply to Dustin J. Mitchell [:dustin] from comment #104) > Comment on attachment 8816858 [details] > Bug 1322041 - Nightly l10n support, and docs. > > https://reviewboard.mozilla.org/r/97382/#review99958 Before addressing this review, I think we should chat to clear up my future plans and current tradeoffs I've chosen. And then we can arm wrestle to see where the fallout of this is, I'll try and corner you today.

(In reply to Dustin J. Mitchell [:dustin] from comment #105) > Comment on attachment 8816859 [details] > Bug 1322041 - Add chain of trust to l10n jobs. > > https://reviewboard.mozilla.org/r/97384/#review99970 > > ::: taskcluster/taskgraph/transforms/l10n.py:138 > (Diff revision 8) > > + job['extra']['chainOfTrust']['inputs']['docker-image'] = { > > + "task-reference": "<docker-image>" > > + } > > what about `unsigned-build`? :aki wrote this, n-i him to clarify.

(In reply to Justin Wood (:Callek) from comment #109) > (In reply to Dustin J. Mitchell [:dustin] from comment #105) > > Comment on attachment 8816859 [details] > > Bug 1322041 - Add chain of trust to l10n jobs. > > > > https://reviewboard.mozilla.org/r/97384/#review99970 > > > > ::: taskcluster/taskgraph/transforms/l10n.py:138 > > (Diff revision 8) > > > + job['extra']['chainOfTrust']['inputs']['docker-image'] = { > > > + "task-reference": "<docker-image>" > > > + } > > > > what about `unsigned-build`? > > :aki wrote this, n-i him to clarify. We can add it, but it's not necessary. We look at task.payload.upstreamArtifacts for part of the chain, and add the decision task and task.extra.chainOfTrust.inputs. Since unsigned-build is in the upstreamArtifacts, adding it to the inputs would be redundant, though I believe it would be harmless.

Comment on attachment 8816858 [details] Bug 1322041 - Nightly l10n support, and docs. https://reviewboard.mozilla.org/r/97382/#review99958 > This answers my "why" in bug 1171738 but leads to the followon question, "why?!" > > So I see why they are different now -- `l10n` is a localization check that can run on push, while `nightly-l10n` is an l10n repack of an artifact from a build tasks. I wonder if those could be renamed to `l10n-check` and `l10n-repack`, respectively? the `l10n` kind will be going away eventually, or merged with the nightly one. But we'll get there! > Why is this required? Shouldn't this be generated from the job key name in `kind.yml` No longer needed (dropped using the build_attrs transform) > This still seems really weird to me. In fact, I initially commented on it in this review without remembering that I had commented on it in bug 1171738, so it's nice to see that I'm consistent, at least :) > > I'd like to have this dependency information visible somehow in the yaml. This is now done with the newer work that landed on date, and that you reviewed. > This kind depends on the `build-signing` kind, so is this really an `unsigned-build`? Fixed in the new code, the dependency was more in ordering and reflected my initial work in actually using a signed build, I now depend on unsigned build kinds, and it is really an unsigned build.

Comment on attachment 8816859 [details] Bug 1322041 - Add chain of trust to l10n jobs. https://reviewboard.mozilla.org/r/97384/#review99970 > what about `unsigned-build`? aki answered this in bugzilla-only, but not-an-issue.

Comment on attachment 8816860 [details] Bug 1322041 - Sign nightly l10n jobs. https://reviewboard.mozilla.org/r/97386/#review99972 chose not to rename the kinds for now.

Comment on attachment 8818066 [details] Bug 1322041 - Support SIMPLE_NAMES in a forced-way (via mozharness) for l10n. https://reviewboard.mozilla.org/r/98172/#review103388

Comment on attachment 8816762 [details] Bug 1322041 - Upload nightly symbols. https://reviewboard.mozilla.org/r/97334/#review103458

Comment on attachment 8816858 [details] Bug 1322041 - Nightly l10n support, and docs. https://reviewboard.mozilla.org/r/97382/#review103466 ::: taskcluster/taskgraph/transforms/l10n.py:240 (Diff revision 10) > + for k in subfields.keys(): > + if not job.get(k): > + continue > + job[k] = copy.deepcopy(job[k]) # don't overwrite dict values here > + for subfield in subfields[k]: > + job[k][subfield] = get_keyed_by(item=job, field=k, subfield=subfield, note that get_keyed_by was removed in bug 1326547

Comment on attachment 8816859 [details] Bug 1322041 - Add chain of trust to l10n jobs. https://reviewboard.mozilla.org/r/97384/#review103470

Comment on attachment 8816860 [details] Bug 1322041 - Sign nightly l10n jobs. https://reviewboard.mozilla.org/r/97386/#review103474 ::: taskcluster/taskgraph/task/signing.py:20 (Diff revision 12) > > @classmethod > def get_inputs(cls, kind, path, config, params, loaded_tasks): > - if (config.get('kind-dependencies', []) != ["build"]): > + if (config.get('kind-dependencies', []) != ["build"] and > + config.get('kind-dependencies', []) != ["nightly-l10n"]): > raise Exception("Signing kinds must depend on builds") ..or l10n repacks.

(In reply to Dustin J. Mitchell [:dustin] from comment #136) > Comment on attachment 8816858 [details] > Bug 1322041 - Nightly l10n support, and docs. > > https://reviewboard.mozilla.org/r/97382/#review103466 > > ::: taskcluster/taskgraph/transforms/l10n.py:240 > (Diff revision 10) > > + for k in subfields.keys(): > > + if not job.get(k): > > + continue > > + job[k] = copy.deepcopy(job[k]) # don't overwrite dict values here > > + for subfield in subfields[k]: > > + job[k][subfield] = get_keyed_by(item=job, field=k, subfield=subfield, > > note that get_keyed_by was removed in bug 1326547 Fixed on date with https://hg.mozilla.org/projects/date/rev/db530d7a4672 and I folded that patch into this one with your IRC f+

Pushed by dmitchell@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dde4c340e377 Add date configs to land on central. r=jlund https://hg.mozilla.org/integration/autoland/rev/e3354e29e276 Add changes necessary for tc-nightly to m-c files. r=jlund https://hg.mozilla.org/integration/autoland/rev/2033645ff938 Support android x86, arm; and desktop linux, linux64 nightlies via Taskcluster. Unsigned. r=dustin https://hg.mozilla.org/integration/autoland/rev/cd307e302983 Upload nightly symbols. r=dustin https://hg.mozilla.org/integration/autoland/rev/5d11cc6efc0c Add signing support for all nightlies. r=dustin https://hg.mozilla.org/integration/autoland/rev/4fc455ee4e67 Vendor chunkify r=gps https://hg.mozilla.org/integration/autoland/rev/b9dfabd68e0c Add Docs for L10n and add locales_file parsing. r=dustin https://hg.mozilla.org/integration/autoland/rev/b2937432df14 Nightly l10n support, and docs. r=dustin https://hg.mozilla.org/integration/autoland/rev/c81852b2cec9 Add chain of trust to l10n jobs. r=dustin https://hg.mozilla.org/integration/autoland/rev/2a2835f86448 Support SIMPLE_NAMES in a forced-way (via mozharness) for l10n. r=jlund https://hg.mozilla.org/integration/autoland/rev/5ca44beb912a Sign nightly l10n jobs. r=dustin

https://hg.mozilla.org/mozilla-central/rev/dde4c340e377 https://hg.mozilla.org/mozilla-central/rev/e3354e29e276 https://hg.mozilla.org/mozilla-central/rev/2033645ff938 https://hg.mozilla.org/mozilla-central/rev/cd307e302983 https://hg.mozilla.org/mozilla-central/rev/5d11cc6efc0c https://hg.mozilla.org/mozilla-central/rev/4fc455ee4e67 https://hg.mozilla.org/mozilla-central/rev/b9dfabd68e0c https://hg.mozilla.org/mozilla-central/rev/b2937432df14 https://hg.mozilla.org/mozilla-central/rev/c81852b2cec9 https://hg.mozilla.org/mozilla-central/rev/2a2835f86448 https://hg.mozilla.org/mozilla-central/rev/5ca44beb912a