Closed Bug 1362489 Opened 3 years ago Closed 3 years ago

Add tasks and mozharness support for windows mach repackage

Categories

(Release Engineering :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 3 obsolete files)

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)
Depends on: 1360525
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.
Attached patch 1_generic_repackage_config.diff (obsolete) — Splinter Review
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
Attachment #8866986 - Attachment is obsolete: true
This should allow for windows repackage task config.  We still need the mozharness config for windows.
Attachment #8866993 - Attachment is obsolete: true
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.
Attachment #8867360 - Attachment is obsolete: true
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.
Assignee: nobody → aki
Attachment #8869537 - Flags: feedback?(mshal)
Attachment #8869537 - Flags: feedback?(kmoir)
Attachment #8869538 - Flags: feedback?(mshal)
Attachment #8869538 - Flags: feedback?(kmoir)
Attachment #8869539 - Flags: feedback?(mshal)
Attachment #8869539 - Flags: feedback?(kmoir)
Attachment #8869540 - Flags: feedback?(mshal)
Attachment #8869540 - Flags: feedback?(kmoir)
Attachment #8869541 - Flags: feedback?(mshal)
Attachment #8869541 - Flags: feedback?(kmoir)
Attached patch 6_l10nSplinter Review
Attachment #8869542 - Flags: feedback?(mshal)
Attachment #8869542 - Flags: feedback?(kmoir)
Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

Whoever has time and headspace :)
Attachment #8869537 - Flags: feedback?(bugspam.Callek)
Attachment #8869538 - Flags: feedback?(bugspam.Callek)
Attachment #8869539 - Flags: feedback?(bugspam.Callek)
Attachment #8869540 - Flags: feedback?(bugspam.Callek)
Attachment #8869541 - Flags: feedback?(bugspam.Callek)
Attachment #8869542 - Flags: feedback?(bugspam.Callek)
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!
Attachment #8869537 - Flags: feedback?(mshal) → feedback+
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.
Attachment #8869538 - Flags: feedback?(mshal) → feedback+
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.
Attachment #8869539 - Flags: feedback?(mshal)
Attachment #8869540 - Flags: feedback?(mshal)
Attachment #8869541 - Flags: feedback?(mshal)
Attachment #8869542 - Flags: feedback?(mshal)
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
Attachment #8869539 - Flags: feedback?(kmoir) → feedback+
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.
Attachment #8869537 - Flags: review-
Attachment #8869537 - Flags: feedback?(bugspam.Callek)
Attachment #8869537 - Flags: feedback+
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)
Attachment #8869538 - Flags: feedback?(bugspam.Callek) → feedback+
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
Attachment #8869539 - Flags: feedback?(bugspam.Callek)
Attachment #8869539 - Flags: feedback?
Attachment #8869539 - Flags: feedback+
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.
Attachment #8869540 - Flags: feedback?(bugspam.Callek) → feedback+
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...
Attachment #8869541 - Flags: feedback?(bugspam.Callek) → feedback+
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.
Attachment #8869542 - Flags: feedback?(bugspam.Callek) → feedback+
Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

removing myself from reviews since Callek and mshal are enough reviewers
Attachment #8869537 - Flags: feedback?(kmoir)
Attachment #8869538 - Flags: feedback?(kmoir)
Attachment #8869540 - Flags: feedback?(kmoir)
Attachment #8869541 - Flags: feedback?(kmoir)
Attachment #8869542 - Flags: feedback?(kmoir)
(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.
Assignee: aki → bugspam.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.
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.
Blocks: 1362534
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Component: General Automation → General
Attachment #8869539 - Flags: feedback?
You need to log in before you can comment on or make changes to this bug.