Add tasks and mozharness support for windows mach repackage

RESOLVED FIXED

Status

RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 3 obsolete attachments)

(Assignee)

Description

a year ago
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)

Updated

a year ago
Depends on: 1360525

Comment 1

a year ago
Created attachment 8866986 [details] [diff] [review]
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

a year ago
Created attachment 8866993 [details] [diff] [review]
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
Attachment #8866986 - Attachment is obsolete: true

Comment 3

a year ago
Created attachment 8867360 [details] [diff] [review]
2_windows_repackage_task_config.diff

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

Updated

a year ago
Attachment #8866993 - Attachment is obsolete: true

Comment 5

a year ago
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.

Updated

a year ago
Attachment #8867360 - Attachment is obsolete: true

Comment 6

a year ago
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

a year ago
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

a year ago
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

a year ago
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

a year ago
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

a year ago
Created attachment 8869537 [details] [diff] [review]
1_generic_repackage_config
Assignee: nobody → aki
Attachment #8869537 - Flags: feedback?(mshal)
Attachment #8869537 - Flags: feedback?(kmoir)

Comment 12

a year ago
Created attachment 8869538 [details] [diff] [review]
2_win_mozconfig_mozharness_config
Attachment #8869538 - Flags: feedback?(mshal)
Attachment #8869538 - Flags: feedback?(kmoir)

Comment 13

a year ago
Created attachment 8869539 [details] [diff] [review]
3_enable_build_signing
Attachment #8869539 - Flags: feedback?(mshal)
Attachment #8869539 - Flags: feedback?(kmoir)

Comment 14

a year ago
Created attachment 8869540 [details] [diff] [review]
4_windows_repackage_task_config
Attachment #8869540 - Flags: feedback?(mshal)
Attachment #8869540 - Flags: feedback?(kmoir)

Comment 15

a year ago
Created attachment 8869541 [details] [diff] [review]
5_repackage_signing
Attachment #8869541 - Flags: feedback?(mshal)
Attachment #8869541 - Flags: feedback?(kmoir)

Comment 16

a year ago
Created attachment 8869542 [details] [diff] [review]
6_l10n
Attachment #8869542 - Flags: feedback?(mshal)
Attachment #8869542 - Flags: feedback?(kmoir)

Comment 17

a year ago
Comment on attachment 8869537 [details] [diff] [review]
1_generic_repackage_config

Whoever has time and headspace :)
Attachment #8869537 - Flags: feedback?(bugspam.Callek)

Updated

a year ago
Attachment #8869538 - Flags: feedback?(bugspam.Callek)

Updated

a year ago
Attachment #8869539 - Flags: feedback?(bugspam.Callek)

Updated

a year ago
Attachment #8869540 - Flags: feedback?(bugspam.Callek)

Updated

a year ago
Attachment #8869541 - Flags: feedback?(bugspam.Callek)

Updated

a year ago
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)

Comment 22

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

Comment 23

a year ago
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

a year ago
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

Updated

a year ago
Attachment #8869539 - Flags: feedback?(kmoir) → feedback+
(Assignee)

Comment 25

a year ago
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+
(Assignee)

Comment 26

a year ago
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+
(Assignee)

Comment 27

a year ago
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+
(Assignee)

Comment 28

a year ago
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+
(Assignee)

Comment 29

a year ago
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+
(Assignee)

Comment 30

a year ago
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)

Updated

a year ago
Attachment #8869538 - Flags: feedback?(kmoir)

Updated

a year ago
Attachment #8869540 - Flags: feedback?(kmoir)

Updated

a year ago
Attachment #8869541 - Flags: feedback?(kmoir)

Updated

a year ago
Attachment #8869542 - Flags: feedback?(kmoir)

Comment 32

a year ago
(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

a year ago
(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.

Updated

a year ago
Assignee: aki → bugspam.Callek
(Assignee)

Comment 35

a year ago
(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 37

a year ago
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

a year ago
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.

Updated

a year ago
Depends on: 1367939, 1367937

Comment 39

a year ago
(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.
(Assignee)

Updated

a year ago
Blocks: 1362534

Comment 40

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

Comment 41

a year ago
(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

a year ago
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.

Updated

a year ago
Blocks: 1370612
(Assignee)

Updated

a year ago
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Blocks: 1380293
Component: General Automation → General
Product: Release Engineering → Release Engineering
(Assignee)

Updated

2 months ago
Attachment #8869539 - Flags: feedback?
You need to log in before you can comment on or make changes to this bug.