Closed Bug 1360525 Opened 7 years ago Closed 7 years ago

add mach repackage so windows builds can be signed as separate step from build with move to taskcluster

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: Callek, Assigned: mshal)

References

Details

Attachments

(4 files)

Bug 1360525 - Separate repackage out into subcommands;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1360525 - 'mach repackage' for installer & stub installer;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1360525 - Add 'mach repackage mar';
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1360525 - Upload setup.exe / setup-stub.exe for repackage task;
59 bytes, text/x-review-board-request
chmanchester
: review+
Details 
This is to make sure that ./mach repackage as it was implemented for OSX cross compile can be utilized for Windows needs.

+++ This bug was initially created as a clone of Bug #1287881 +++

From bug 1277591 re creating a windows signing worker (aki's notes)

https://bugzilla.mozilla.org/show_bug.cgi?id=1277591#c4

To be explicit:
* `mach repackage`, or something like it, will be needed to do windows signing as a separate step from builds.
** unsigned windows packages need to be exploded, all the internal binaries signed separately, then repackaged, and the external package also signed.
* `mach repackage` is not written yet
* I'm not certain who is assigned the task of writing `mach repackage`.  Its absence blocks this bug.  In my mind, the actual windows signingscript + scriptworker implementations/deployment are relatively easy once linux signing scriptworker is deployed.  The main difference is `mach repackage`.
Component: mach → Build Config
I landed a WIP on date that hopefully is functional enough to support testing the signing tasks. Previously we had 'mach repackage' for dmgs like so:

./mach repackage -i tarball.tar.gz -o output.dmg

This is now:

./mach repackage dmg -i tarball.tar.gz -o output.dmg

(ie: 'repackage dmg' is the command)

The following commands show a Windows full installer, stub installer, and complete mar:

 1) ./mach repackage installer --package path/to/firefox-55.0a1.en-US.win32.zip --tag browser/installer/windows/app.tag --setupexe path/to/setup.exe -o installer.exe
 2) ./mach repackage installer --tag browser/installer/windows/stub.tag --setupexe path/to/setup-stub.exe -o installer-stub.exe
 3) ./mach repackage mar -i path/to/firefox-55.0a1.en-US.win32.zip --mar path/to/mar.exe -o win32.complete.mar

The installer needs both a package .zip and the setup.exe from the signing task, while the stub installer just needs setup-stub.exe from the signing task (the *.tag files are in-tree). For the complete mar we need the package from the signing task, as well as the obj-firefox/dist/host/bin/mar.exe file from the build task.

kmoir, aki - can you try to wire this into the tasks and let me know how it goes? I still have some cleanup to do before getting reviews, but I'd like to know what roadblocks there are.
Flags: needinfo?(kmoir)
Flags: needinfo?(aki)
(In reply to Michael Shal [:mshal] from comment #1)
> kmoir, aki - can you try to wire this into the tasks and let me know how it
> goes? I still have some cleanup to do before getting reviews, but I'd like
> to know what roadblocks there are.

I was under the impression you wanted us to wire this into the OSX dmg repackage command; on re-reading it appears you want the full windows task graph?
(In reply to Aki Sasaki [:aki] from comment #2)
> I was under the impression you wanted us to wire this into the OSX dmg
> repackage command; on re-reading it appears you want the full windows task
> graph?

I'm not sure I follow - what does this have to do with OSX? I added 'mach repackage' support for the Windows installer and complete mar (on the date branch for now), and was hoping it could be tested with the Windows signing tasks.

I'm on PTO for a bit, so we can chat on Tuesday at the migration meeting.
Blocks: 1361402
Blocks: 1362534
To answer the ni, either aki or I will look at this, depending who can get to it first.
Flags: needinfo?(kmoir)
Blocks: 1362489
Sorry for the delay; currently working on beta update test bustage, scriptworker claimWork issues, and helping dhouse with new signing servers.

Aiui, the request is currently for someone to take and fix bug 1362489 since Callek is out.  Is that accurate?
Working on 1362489 as time allows.
Flags: needinfo?(aki)
Depends on: 1365722
Mike, testing on date showed we are not currently uploading setup[-stub].exe in the actual build/Nightly, can you rectify that?  Should we have it be a part of this bug?

(:aki went to manually sign, to get this testing out of the way, since we have other blocking issues we discovered, but without the setup.exe's we couldn't do that)
Flags: needinfo?(mshal)
We chatted in IRC, but for bug continuity purposes there's a test patch on date that should upload these artifacts from the build task.
Flags: needinfo?(mshal)
Comment on attachment 8875452 [details]
Bug 1360525 - Separate repackage out into subcommands;

https://reviewboard.mozilla.org/r/146894/#review152592

::: python/mozbuild/mozbuild/mach_commands.py
(Diff revision 1)
>          if not os.path.exists(os.path.join(self.topobjdir, 'config.status')):
>              print('config.status not found.  Please run |mach configure| '
>                    'prior to |mach repackage|.')
>              return 1
>  
> -        if output.endswith('.dmg'):

Should we error out here if the output doesn't end in '.dmg'?
Attachment #8875452 - Flags: review?(cmanchester) → review+
Comment on attachment 8875453 [details]
Bug 1360525 - 'mach repackage' for installer & stub installer;

https://reviewboard.mozilla.org/r/146896/#review152596

::: python/mozbuild/mozbuild/action/exe_7z_archive.py:15
(Diff revision 1)
>  import subprocess
>  import tempfile
>  import mozpack.path as mozpath
>  
>  def archive_exe(pkg_dir, tagfile, sfx_package, package):
> -    tmpdir = tempfile.mkdtemp(prefix='tmp')
> +    def zip_package(pkg_dir, tmpdir):

Maybe I'll see the use for this later in the patch series, but this nested function seems to just make things harder to understand. For instance, `pkg_dir` doesn't need to be a parameter here, it's always the same as what was passed to archive_exe as `pkg_dir`.

Is this just to decrease the indentation level?

::: python/mozbuild/mozbuild/repackaging/installer.py:23
(Diff revision 1)
> +    tag = mozpath.realpath(tag)
> +    output = mozpath.realpath(output)
> +    ensureParentDir(output)
> +
> +    tmpdir = tempfile.mkdtemp()
> +    saved_dir = os.getcwd()

I might call this 'old_cwd'.
Attachment #8875453 - Flags: review?(cmanchester) → review+
Comment on attachment 8875454 [details]
Bug 1360525 - Add 'mach repackage mar';

https://reviewboard.mozilla.org/r/146898/#review152606

::: python/mozbuild/mozbuild/repackaging/mar.py:29
(Diff revision 1)
> +        filelist = z.namelist()
> +        z.close()
> +
> +        # Make sure the .zip file just contains a directory like 'firefox/' at
> +        # the top, and find out what it is called.
> +        toplevel_dirs = set([mozpath.split(f)[0] for f in filelist])

I don't think I'd ever noticed how different `os.path.split` and `mozpath.split` are, that's a little odd.
Attachment #8875454 - Flags: review?(cmanchester) → review+
Comment on attachment 8875455 [details]
Bug 1360525 - Upload setup.exe / setup-stub.exe for repackage task;

https://reviewboard.mozilla.org/r/146900/#review152608

::: commit-message-c2f07:6
(Diff revision 1)
> +Bug 1360525 - Upload setup.exe / setup-stub.exe for repackage task; r?chmanchester
> +
> +The Windows repackaging tasks need the setup.exe / setup-stub.exe files
> +from the installer / stub-installer build in order to regenerate a new
> +installer. In the future, the build can be updated to only produce the
> +setup.exe / setup-stub.exe files (instead of a full installer).

That is good news, building the installer still takes a few minutes in automation.
Attachment #8875455 - Flags: review?(cmanchester) → review+
Comment on attachment 8875452 [details]
Bug 1360525 - Separate repackage out into subcommands;

https://reviewboard.mozilla.org/r/146894/#review152592

> Should we error out here if the output doesn't end in '.dmg'?

I don't think we need to. This if-statement was there originally just as a way of selecting which repackager to use, but then I realized that the file extension wasn't really a good way of doing that.
Comment on attachment 8875453 [details]
Bug 1360525 - 'mach repackage' for installer & stub installer;

https://reviewboard.mozilla.org/r/146896/#review152596

> Maybe I'll see the use for this later in the patch series, but this nested function seems to just make things harder to understand. For instance, `pkg_dir` doesn't need to be a parameter here, it's always the same as what was passed to archive_exe as `pkg_dir`.
> 
> Is this just to decrease the indentation level?

I don't know why I did it like this, to be honest. I think it is simpler to just put everything in the one try/finally block and avoid the separate function. I'll ask for re-review for this chunk.

> I might call this 'old_cwd'.

Fixed.
Comment on attachment 8875453 [details]
Bug 1360525 - 'mach repackage' for installer & stub installer;

Asking for re-review of archive_exe()
Attachment #8875453 - Flags: review+ → review?(cmanchester)
Comment on attachment 8875453 [details]
Bug 1360525 - 'mach repackage' for installer & stub installer;

https://reviewboard.mozilla.org/r/146896/#review153180
Attachment #8875453 - Flags: review?(cmanchester) → review+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91b810a40547
Separate repackage out into subcommands; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a6430edd16b0
'mach repackage' for installer & stub installer; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a5c9ab8ea451
Add 'mach repackage mar'; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/76acaa56c314
Upload setup.exe / setup-stub.exe for repackage task; r=chmanchester
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: