We need to modify mach repackage for the work in Bug 1360525 Specifically ./mach repackage calls in mozharness needs to change to support the subcommand format, and we need to taskgraph it. Specifically taskgraph: tc(N) --> signing (signs .zip and "the setup files") --> mach repackage (zip, installer, complete.mar, etc) --> signing-pt2 (sign the installer.exe complete.mar and etc)

We're going to have to change this to allow for multiple signed inputs, with differing `mach repackage` commandlines. But the idea here is to allow for multiple `mach repackage` calls per script/config run.

This patch allows for multiple urls/input files, as well as multiple commandline `mach repackage` formats. If we go this route, we'll have to either define multiple SIGNED_INPUT type environment variables for windows, or we'll need to do something like comma-delimited urls (e.g. SIGNED_INPUT="https://url/1,https://url/2,..."). The former sounds more doable; it'll require changes in taskcluster/taskgraph/transforms/repackage.py

This should allow for windows repackage task config. We still need the mozharness config for windows.

https://github.com/escapewindow/gecko-projects/tree/win-repackage instead of the bugzilla or try patch queue :) Should allow me to share patches without locking us into a set with mozreview.

Untested: https://github.com/escapewindow/gecko-projects/compare/date...escapewindow:win-repackage I think we still have to create a build-signing and a repackage-signing task, and then hook those up to beetmover etc

I've made some fixes and rebased, so the ordering of the patches is all screwy, but there's some progress. `./mach taskgraph tasks` works with 06a9fbe, but dies with "Exception: duplicate tasks with label build-win64-nightly/opt" on the tip of win-repackage. Something in those last 2 patches is broken.

As of https://github.com/escapewindow/gecko-projects/commit/20306f89a4e2dfdf1f02fa1f8cd8d83549b21c27 , ./mach taskgraph tasks works. This adds win64-nightly signing and repackage. We need to: - add repackage-signing - verify the signing formats don't need signingscript changes, or add the required changes - change beetmover to allow for repackage and repackage-signing deps - enable balrog for win64 - then support win32 - land and test

https://github.com/mozilla/gecko-projects/compare/date...escapewindow:win-repackage as of 2938e80, We need to: X add repackage-signing X add l10n support _ the mozharness configs may need fixing _ verify the signing formats don't need signingscript changes, or add the required changes _ change beetmover to allow for repackage and repackage-signing deps _ enable balrog for win64 _ then support win32 _ land and test

Talked with mshal. We're at a point where we can land on date-branch and test, so I'm going to get feedback now.

Comment on attachment 8869537 [details] [diff] [review] 1_generic_repackage_config Whoever has time and headspace :)

Comment on attachment 8869537 [details] [diff] [review] 1_generic_repackage_config >commit ceccd95c67d88ccb11d4e6197cad4ae8861a7fdb >Author: Aki Sasaki <aki@escapewindow.com> >Date: Fri May 12 15:36:42 2017 -0700 > > 1_generic_repackage_config > >diff --git a/testing/mozharness/configs/repackage/osx_signed.py b/testing/mozharness/configs/repackage/osx_signed.py >index 1bdb133..9aecb1d 100644 >--- a/testing/mozharness/configs/repackage/osx_signed.py >+++ b/testing/mozharness/configs/repackage/osx_signed.py >@@ -1,11 +1,17 @@ > import os > > config = { >- "input_filename": "target.tar.gz", >- "output_filename": "target.dmg", > "input_home": "/home/worker/workspace/inputs", > "src_mozconfig": "browser/config/mozconfigs/macosx64/repack", > >+ "download_config": { >+ "target.tar.gz": os.environ.get("SIGNED_INPUT"), >+ }, >+ >+ "repackage_config": [[ >+ "dmg", "-i", "target.tar.gz", "-o" "target.dmg" >+ ]], >+ > # ToolTool > "tooltool_manifest_src": 'browser/config/tooltool-manifests/macosx64/cross-releng.manifest', > "tooltool_url": 'http://relengapi/tooltool/', >diff --git a/testing/mozharness/scripts/repackage.py b/testing/mozharness/scripts/repackage.py >index efb2128..c484c3a 100644 >--- a/testing/mozharness/scripts/repackage.py >+++ b/testing/mozharness/scripts/repackage.py >@@ -10,21 +10,6 @@ from mozharness.mozilla.mock import ERROR_MSGS > > class Repackage(BaseScript): > >- config_options = [[ >- ['--signed-input', ], >- {"action": "store", >- "dest": "signed_input", >- "type": "string", >- "default": os.environ.get('SIGNED_INPUT'), >- "help": "Specify the signed input (url)"} >- ], [ >- ['--output-file', ], >- {"action": "store", >- "dest": "output_file", >- "type": "string", >- "help": "Specify the output filename"} >- ]] >- > def __init__(self, require_config_file=False): > script_kwargs = { > 'all_actions': [ >@@ -35,24 +20,19 @@ class Repackage(BaseScript): > } > BaseScript.__init__( > self, >- config_options=self.config_options, > require_config_file=require_config_file, > **script_kwargs > ) > >- # Assert we have it either passed in or in environment >- assert self.config.get('signed_input'), \ >- "Must pass --signed-input or be set in the environment as SIGNED_INPUT" >- > def download_input(self): > config = self.config > >- url = config['signed_input'] >- status = self.download_file(url=url, >- file_name=config['input_filename'], >- parent_dir=config['input_home']) >- if not status: >- self.fatal("Unable to fetch signed input from %s" % config['signed_input']) >+ for path, url in config["download_config"]: >+ status = self.download_file(url=url, >+ file_name=repack_config['input_filename'], Do you have access to repack_config here? It looks like it's just a local variable in repackage(). >+ command = [python, 'mach', '--log-no-times', 'repackage'] FYI bug 1361912 changed "python" here to "sys.executable". I think we probably just need to merge m-c into date to get the change, but you'll have to update your patch as well. I was a bit concerned that introducing the installer repackage script would've been messy, but it looks like this is a clever solution to work in the various requirements. Nice work!

Comment on attachment 8869538 [details] [diff] [review] 2_win_mozconfig_mozharness_config >+ "repackage_config": [[ >+ "installer", "--package", "target.zip", "--tag", >+ "browser/installer/windows/app.tag", "--setupexe", "setup.exe", "-o", >+ "installer.exe" >+ ], [ >+ "installer", "--tag", "browser/installer/windows/stub.tag", >+ "--setupexe", "setup-stub.exe", "-o", "installer-stub.exe" Are "installer.exe" and "installer-stub.exe" what we actually want the final outputs to be called? Or should it be "target.installer.exe" and "target.stub-installer.exe" or something? I've no real opinion on the matter, but it looks like these are just the example names I used in bug 1360525. >+ ], [ >+ "mar", "-tag", "target.zip", "--mar", "mar.exe", "-o", >+ "target.complete.mar" >+ ]], I think this should be ["mar", "-i", "target.zip", ...]. The -tag argument is only used in the installer repacks.

Comment on attachment 8869539 [details] [diff] [review] 3_enable_build_signing I'm not going to be much help in providing feedback for the task definitions without taking a chunk of time to get up to speed. I'll leave these for kmoir / Callek.

https://hg.mozilla.org/projects/date/rev/8b84084b5652be320e4a3cc856323e2ff1fd14f5 bug 1362489 - 1_generic_repackage_config https://hg.mozilla.org/projects/date/rev/da3fbf4c8a21a8b037772038e5a4dee1006af01b bug 1362489 - 2_win_mozconfig_mozharness_config

Landed the 1st 2 patches, with mshal's comment fixes.

Updated the commits in https://github.com/escapewindow/gecko-projects/commits/win-repackage to match the new changes (essentially, target.installer.exe and target.stub-installer.exe)

Status: 2/6 patches fixed and landed on Date. Status from https://bugzilla.mozilla.org/show_bug.cgi?id=1362489#c9 is still accurate: We need to: X add repackage-signing X add l10n support _ the mozharness configs may need fixing _ verify the signing formats don't need signingscript changes, or add the required changes _ change beetmover to allow for repackage and repackage-signing deps _ enable balrog for win64 _ then support win32 _ land and test

Comment on attachment 8869537 [details] [diff] [review] 1_generic_repackage_config Review of attachment 8869537 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, r- for the reasons noted below. ::: testing/mozharness/configs/repackage/osx_signed.py @@ +8,5 @@ > + "target.tar.gz": os.environ.get("SIGNED_INPUT"), > + }, > + > + "repackage_config": [[ > + "dmg", "-i", "target.tar.gz", "-o" "target.dmg" meant to use a comma between "-o" and "target.dmg"? ::: testing/mozharness/scripts/repackage.py @@ +61,5 @@ > python = self.query_exe('python2.7') > + for repack_config in config["repackage_config"]: > + command = [python, 'mach', '--log-no-times', 'repackage'] > + for arg in repack_config["repackage_args"]: > + command.append(arg % repack_config) config["repackage_config"] is an array of arrays of strings. Which means the line `repack_config["repackage_args"]` fails. Additionally `arg % repack_config` doesn't feel as useful in this scenario.

Comment on attachment 8869538 [details] [diff] [review] 2_win_mozconfig_mozharness_config Review of attachment 8869538 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mozharness/configs/repackage/win_signed.py @@ +8,5 @@ > + plat = "win32" > + > +config = { > + "input_home": "/home/worker/workspace/inputs", > + "src_mozconfig": "browser/config/mozconfigs/win64/repack", probably confusing to have win64 always for this, but switch on platform above.... (non blocking)

Comment on attachment 8869539 [details] [diff] [review] 3_enable_build_signing Review of attachment 8869539 [details] [diff] [review]: ----------------------------------------------------------------- I would find the transforms easier to review if in mozreview (since I could expand surrounding context)... ::: taskcluster/taskgraph/transforms/beetmover.py @@ +110,4 @@ > 'android-x86-old-id-nightly': _MOBILE_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > 'android-api-15-old-id-nightly': _MOBILE_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > 'macosx64-nightly': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > + 'win64-nightly': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, could avoid these changes in this patch if you did the `not-for-build-platform` magic in the beetmover kind.yml too

Comment on attachment 8869540 [details] [diff] [review] 4_windows_repackage_task_config Review of attachment 8869540 [details] [diff] [review]: ----------------------------------------------------------------- This feel hard to read to me, but I don't have any good suggestions on any short-term improvements. It looks correct which is the biggest piece we're going for.

Comment on attachment 8869541 [details] [diff] [review] 5_repackage_signing Review of attachment 8869541 [details] [diff] [review]: ----------------------------------------------------------------- I'd love if we refactored the repackage transforms to have this one share a lot of the same logic, but I can see why you went this way for now. ::: taskcluster/taskgraph/transforms/repackage_signing.py @@ +45,5 @@ > + for job in jobs: > + dep_job = job['dependent-task'] > + > + treeherder = job.get('treeherder', {}) > + treeherder.setdefault('symbol', 'tc-rs(N)') nit: can we not invent a group just for this one thing? ::: taskcluster/taskgraph/transforms/task.py @@ +443,4 @@ > 'tc-BM-L10n': 'Beetmover for locales executed by Taskcluster', > 'tc-Up': 'Balrog submission of updates, executed by Taskcluster', > 'tc-cs': 'Checksum signing executed by Taskcluster', > + 'tc-rs': 'Repackage signing executed by Taskcluster', notable: treeherder hard caches group names for a given label, and thus it can't easily be changed (by us) once treeherder sees it once. Not a big deal since they can clear it, but that is something I learned when doing treeherder stuff for l10n for linux...

Comment on attachment 8869542 [details] [diff] [review] 6_l10n Review of attachment 8869542 [details] [diff] [review]: ----------------------------------------------------------------- Looks promising, but I should note: I've done zero testing of l10n on windows via TC so far.

Comment on attachment 8869537 [details] [diff] [review] 1_generic_repackage_config removing myself from reviews since Callek and mshal are enough reviewers

(In reply to Justin Wood (:Callek) from comment #25) > Comment on attachment 8869537 [details] [diff] [review] > 1_generic_repackage_config > > Review of attachment 8869537 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good, r- for the reasons noted below. > > ::: testing/mozharness/configs/repackage/osx_signed.py > @@ +8,5 @@ > > + "target.tar.gz": os.environ.get("SIGNED_INPUT"), > > + }, > > + > > + "repackage_config": [[ > > + "dmg", "-i", "target.tar.gz", "-o" "target.dmg" > > meant to use a comma between "-o" and "target.dmg"? Nice catch! > ::: testing/mozharness/scripts/repackage.py > @@ +61,5 @@ > > python = self.query_exe('python2.7') > > + for repack_config in config["repackage_config"]: > > + command = [python, 'mach', '--log-no-times', 'repackage'] > > + for arg in repack_config["repackage_args"]: > > + command.append(arg % repack_config) > > config["repackage_config"] is an array of arrays of strings. > > Which means the line `repack_config["repackage_args"]` fails. Additionally > `arg % repack_config` doesn't feel as useful in this scenario. This is left over from when repackage_args was a dict, and I had %(input_filename)s type strings in there. Since this patch is already landed on date, I'll land a followup to fix these issues.

(In reply to Justin Wood (:Callek) from comment #27) > Comment on attachment 8869539 [details] [diff] [review] > 3_enable_build_signing > > Review of attachment 8869539 [details] [diff] [review]: > ----------------------------------------------------------------- > > I would find the transforms easier to review if in mozreview (since I could > expand surrounding context)... Aiui, mozreview doesn't allow for piecemeal landing, nor does it allow for someone else to easily take over a bug in progress. We could certainly file new if we were to switch bug owners. > ::: taskcluster/taskgraph/transforms/beetmover.py > @@ +110,4 @@ > > 'android-x86-old-id-nightly': _MOBILE_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > > 'android-api-15-old-id-nightly': _MOBILE_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > > 'macosx64-nightly': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > > + 'win64-nightly': _DESKTOP_UPSTREAM_ARTIFACTS_UNSIGNED_EN_US, > > could avoid these changes in this patch if you did the > `not-for-build-platform` magic in the beetmover kind.yml too Sure, but I was going for enabling as much as possible so we can see what's broken.

https://hg.mozilla.org/projects/date/rev/c536a48307bc3094f3bde4bb7adad4e92729f8ef bug 1362489 - address callek's review comments.

(In reply to Justin Wood (:Callek) from comment #29) > Comment on attachment 8869541 [details] [diff] [review] > 5_repackage_signing > > Review of attachment 8869541 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd love if we refactored the repackage transforms to have this one share a > lot of the same logic, but I can see why you went this way for now. > > ::: taskcluster/taskgraph/transforms/repackage_signing.py > @@ +45,5 @@ > > + for job in jobs: > > + dep_job = job['dependent-task'] > > + > > + treeherder = job.get('treeherder', {}) > > + treeherder.setdefault('symbol', 'tc-rs(N)') > > nit: can we not invent a group just for this one thing? Decided not to change this due to the l10n as well.

Pushed those patches to date, and am about to trigger a win nightly https://hg.mozilla.org/projects/date/rev/9aec8d9ec61866593b30f782c1ba7e0aee40afa0 https://hg.mozilla.org/projects/date/rev/8a3353ab8effd8addb929177f279ed2bf2aa3cc2 https://hg.mozilla.org/projects/date/rev/140de387e7dba76096381868915f3a3c97b5c8f1 https://hg.mozilla.org/projects/date/rev/2e474f65c2e9a1b6774a9567f56416e6a5fe62c4

We're going to have to enable task.payload.features.chainOfTrust on the windows builds: https://docs.taskcluster.net/reference/workers/generic-worker/payload

Also, https://tools.taskcluster.net/task-inspector/#Psp4S0zoRXa5oyN_G0aw7w/0 specifies the upstream artifacts as "upstreamArtifacts": [ { "paths": [ "public/build/target.zip", "public/build/setup.exe", "public/build/setup-stub.exe" ], "formats": [ "sha2signcode" ], "taskId": "Psp4S0zoRXa5oyN_G0aw7w", "taskType": "build" } ] In https://tools.taskcluster.net/task-inspector/#Psp4S0zoRXa5oyN_G0aw7w/0 , we have public/build/target.zip, but no public/build/setup.exe or public/build/setup-stub.exe . I'm not sure if we need to add another two artifacts to upload, or what, but we'll need to get those two files somewhere.

(In reply to Aki Sasaki [:aki] from comment #38) > Also, https://tools.taskcluster.net/task-inspector/#Psp4S0zoRXa5oyN_G0aw7w/0 > specifies the upstream artifacts as > > "upstreamArtifacts": [ > { > "paths": [ > "public/build/target.zip", > "public/build/setup.exe", > "public/build/setup-stub.exe" > ], > "formats": [ > "sha2signcode" > ], > "taskId": "Psp4S0zoRXa5oyN_G0aw7w", > "taskType": "build" > } > ] > > In https://tools.taskcluster.net/task-inspector/#Psp4S0zoRXa5oyN_G0aw7w/0 , > we have public/build/target.zip, but no public/build/setup.exe or > public/build/setup-stub.exe . I'm not sure if we need to add another two > artifacts to upload, or what, but we'll need to get those two files > somewhere. Per Rail, https://hg.mozilla.org/projects/date/file/tip/browser/confvars.sh#l18 will prevent us from building the stub installer unless. We either need different task definitions for win32 vs win64, or we need to build the stub for both.

Tried manually signing. This won't work til I fix bug 1368192.

(In reply to Aki Sasaki [:aki] from comment #40) > Tried manually signing. This won't work til I fix bug 1368192. Manually signing worked. Also, generic-worker chain of trust support should be in. We need to change the windows build to enable chain of trust generation (task.payload.features.chainOfTrust=True): https://docs.taskcluster.net/reference/workers/generic-worker/docs/payload

https://public-artifacts.taskcluster.net/C61zV_3-Q7-np2KcPiDgrg/0/public/logs/live_backing.log , got it to where it's dying on tooltool rather than downloading artifacts.