1362489 - Add tasks and mozharness support for windows mach repackage

Status: RESOLVED FIXED

(Release Engineering :: General, enhancement)

Description

[Justin Wood (:Callek)]

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)

Comment 1

[Aki Sasaki (not active)]

Attachment: wip: 1_generic_repackage_arg.diff

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.

Comment 2

[Aki Sasaki (not active)]

Attachment: 1_generic_repackage_config.diff

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

Comment 3

[Aki Sasaki (not active)]

Attachment: 2_windows_repackage_task_config.diff

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

Comment 4

[Aki Sasaki (not active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b64527a2a4c52c82bcb9c2999c51b3c61d498b3b

Comment 5

[Aki Sasaki (not active)]

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.

Comment 6

[Aki Sasaki (not active)]

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

Comment 7

[Aki Sasaki (not active)]

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.

Comment 8

[Aki Sasaki (not active)]

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

Comment 9

[Aki Sasaki (not active)]

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

Comment 10

[Aki Sasaki (not active)]

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 11

[Aki Sasaki (not active)]

Attachment: 1_generic_repackage_config

Comment 12

[Aki Sasaki (not active)]

Attachment: 2_win_mozconfig_mozharness_config

Comment 13

[Aki Sasaki (not active)]

Attachment: 3_enable_build_signing

Comment 14

[Aki Sasaki (not active)]

Attachment: 4_windows_repackage_task_config

Comment 15

[Aki Sasaki (not active)]

Attachment: 5_repackage_signing

Comment 16

[Aki Sasaki (not active)]

Attachment: 6_l10n

Comment 17

[Aki Sasaki (not active)]

Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

Whoever has time and headspace :)

Comment 18

[Michael Shal [:mshal]]

Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

>commit ceccd95c67d88ccb11d4e6197cad4ae8861a7fdb
>Author: Aki Sasaki <[e-mail]>
>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 19

[Michael Shal [:mshal]]

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 20

[Michael Shal [:mshal]]

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.

Comment 21

[Aki Sasaki (not active)]

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

Comment 22

[Aki Sasaki (not active)]

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

Comment 23

[Aki Sasaki (not active)]

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)

Comment 24

[Aki Sasaki (not active)]

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 25

[Justin Wood (:Callek)]

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 26

[Justin Wood (:Callek)]

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 27

[Justin Wood (:Callek)]

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 28

[Justin Wood (:Callek)]

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 29

[Justin Wood (:Callek)]

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 30

[Justin Wood (:Callek)]

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 31

[Kim Moir [:kmoir] ET]

Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

removing myself from reviews since Callek and mshal are enough reviewers

Comment 32

[Aki Sasaki (not active)]

(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.

Comment 33

[Aki Sasaki (not active)]

(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.

Comment 34

[Aki Sasaki (not active)]

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

Comment 35

[Justin Wood (:Callek)]

(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.

Comment 36

[Justin Wood (:Callek)]

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

Comment 37

[Aki Sasaki (not active)]

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

Comment 38

[Aki Sasaki (not active)]

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.

Comment 39

[Aki Sasaki (not active)]

(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.

Comment 40

[Aki Sasaki (not active)]

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

Comment 41

[Aki Sasaki (not active)]

(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

Comment 42

[Aki Sasaki (not active)]

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.