1347576 - Create ./mach repackage that takes a tarball and returns a .dmg to support OSX signing

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Justin Wood (:Callek)]

Mike is working on this

Comment 1

[Michael Shal [:mshal]]

I have some test patches that are now on date. Once we can test them out I'll post them for review.

They either need a configure run with a mozconfig like this:

export MKFSHFS=$topsrcdir/hfsplus-tools/newfs_hfs
export DMG_TOOL=$topsrcdir/dmg/dmg
export HFS_TOOL=$topsrcdir/dmg/hfsplus
ac_add_options --disable-compile-environment

Or, configure can be skipped if MKFSHFS, DMG_TOOL, and HFS_TOOL are specified in the environment. Eg:

MKFSHFS=/home/mshal/mozilla-central-git/hfsplus-tools/newfs_hfs DMG_TOOL=/home/mshal/mozilla-central-git/dmg/dmg HFS_TOOL=/home/mshal/mozilla-central-git/dmg/hfsplus ./mach repackage -i mac-signed-branding.tar.gz -o mydmg.dmg

The input tar.gz is expected to have the extra files like .DS_Store, background.png, and the icons that come from the normal make_dmg action. Assuming the input to the signing server is a dmg from the build system, this should be fine.

Comment 2

[Kim Moir [:kmoir] ET]

mshal: does it make sense to ask for feedback on these patches so the review process is faster or is the better path to test it first?

Comment 3

[Michael Shal [:mshal]]

Good idea. I was hoping to test it first in case there were any showstoppers but we could start to review now and update it if necessary after we test it on the date branch.

Comment 4

[Michael Shal [:mshal]]

Attachment: Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

This is the initial support of 'mach repackage', which can take an
existing tarball and create a relevant package format for a target
system. Currently the intended use is to repackage a signed tarball into
an OSX .dmg file, though we can expand it to support other package types
in the future (marked by the TODO in the mach command).

Review commit: https://reviewboard.mozilla.org/r/122434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122434/

Comment 5

[Michael Shal [:mshal]]

Attachment: Bug 1347576 - Allow 'mach repackage' to work from environment variables;

The cross Linux->OSX dmg creator needs some tools that are specified by
configure variables. However, this means we would need to run configure
before repackaging a signed tarball into a dmg. Instead we can
optionally allow the tools to be passed in via environment variables and
only import buildconfig if necessary.

Review commit: https://reviewboard.mozilla.org/r/122436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122436/

Comment 6

[Michael Shal [:mshal]]

We had a few fixups from testing:

1) The os.remove() on the /Applications symlink shouldn't fail the whole thing if it already doesn't exist
2) The dmg tool silently fails if the output directory doesn't exist. We can ensure it's created, but we should also think about following up by making the dmg tool less verbose and removing the stdout=/dev/null hack that we have to silence it.

Comment 7

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122434/diff/1-2/

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 8849643 [details]
Bug 1347576 - Allow 'mach repackage' to work from environment variables;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122436/diff/1-2/

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review124706

Is there somewhere I can go for an overview of where this fits into a larger process? Thanks.

::: python/mozbuild/mozpack/dmg.py:136
(Diff revision 2)
> +    try:
> +        os.remove(mozpath.join(source_directory, ' '))

Can you point me towards an explanation of this? Looks pretty odd to me.

::: python/mozbuild/mozpack/dmg.py:150
(Diff revision 2)
> +    else:
> +        raise Exception("Input package does not contain an application.ini file")

`for...else` has always thrown me for a, um, loop.

Can we just `raise` if `volume_name is None`?

::: python/mozbuild/mozpack/unarchive.py:9
(Diff revision 2)
> +def unpack_archive(filename, outputdir):
> +    if not tarfile.is_tarfile(filename):
> +        raise Exception("File %s is not a valid tarfile." % filename)
> +

Someone more familiar with the packager might have an idea of where this should go under mozpack... alternatively, I might say the `tarfile` api isn't really worth wrapping like this, at least for what we're doing here.

::: python/mozbuild/mozpack/unarchive.py:13
(Diff revision 2)
> +    tar = tarfile.open(filename)
> +    tar.extractall(path=outputdir)
> +    tar.close()

Can be `with tarfile.open(filename) as tar:` etc.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849643 [details]
Bug 1347576 - Allow 'mach repackage' to work from environment variables;

https://reviewboard.mozilla.org/r/122436/#review124714

::: commit-message-99eea:4
(Diff revision 2)
> +configure variables. However, this means we would need to run configure
> +before repackaging a signed tarball into a dmg. Instead we can

Why don't we want to run configure? Let's mention that in the commit message if we're sure this is how we want to do it.

::: python/mozbuild/mozpack/dmg.py:125
(Diff revision 2)
> +        path = os.environ.get(tool)
> +        if not path:
> +            import buildconfig
> -        path = buildconfig.substs[tool]
> +            path = buildconfig.substs[tool]

The precedence here seems weird, shouldn't we try to start by importing buildconfig and fall back to environment variables if that doesn't work out?

::: python/mozbuild/mozpack/dmg.py:135
(Diff revision 2)
>              raise Exception('Required tool "%s" at path "%s" is not executable' % (tool, path))
> +        cross_tools[tool] = path

A "check" function that populates a global seems weird (although I can see this will work fine if we go through `create_dmg` is it is today).

We could replace individual checks to `substs` as they are now with a newly invented `get_tool` function that would do this dance for us at each callsite.

Comment 11

[Justin Wood (:Callek)]

(In reply to Chris Manchester (:chmanchester) from comment #9)
> Comment on attachment 8849642 [details]
> Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;
> 
> https://reviewboard.mozilla.org/r/122434/#review124706
> 
> Is there somewhere I can go for an overview of where this fits into a larger
> process? Thanks.

Basically https://bugzilla.mozilla.org/attachment.cgi?id=8841727 (From Bug 1318505) see also comment 12 from that bug.

To restate here, the idea is:

* Build task, creates an unsigned .dmg (unaffected by this patch)
* Taskcluster's taskgraph hands that .dmg off to the releng controlled signing worker, the worker then uploads a tarball of the dmg's contents, signed.
* Taskclusters taskgraph then hands that tarball off to a mozharness script I wrote, in a seperate task, that has a full source checkout and can run the commands necessary to repack a .dmg from that tarball.

This mach command is implementing the latter part, since it needs to use the build system.

Any Questions?

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Nope, I think that covers it. Thanks!

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review124706

> Can you point me towards an explanation of this? Looks pretty odd to me.

The ' ' (single space) symlink to /Applications was added in bug 320155 so that when you open the .dmg, you can easily drag the Firefox app into the Applications folder. This was then moved to python in bug 1190522. Though looking at this again, it would probably be more straightforward to simply skip creating the symlink if it already exists?

> Someone more familiar with the packager might have an idea of where this should go under mozpack... alternatively, I might say the `tarfile` api isn't really worth wrapping like this, at least for what we're doing here.

I thought this was a reasonable place since archive.py exists in the same directory. The wrapper function was so we could fairly easily plug different unpacking / repacking functions into repackage() based on things like the file extension (or command-line argument, etc), though that would be more apparent once we add Windows support. However, maybe it makes more sense to just have 'mach repackage' call out directly to repackage_dmg (or in the future, repackage_windows_installer), and do the untar logic there.

What do you think of moving most of the repackage() and unpack_archive() functions inside repackage_dmg? Then repackage() can delegate to repackage_dmg if passed a --dmg flag or something (which would be the only valid flag until Windows is added).

Comment 14

[Michael Shal [:mshal]]

chmanchester: Looking for feedback on #c13 - thanks!

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review124706

> The ' ' (single space) symlink to /Applications was added in bug 320155 so that when you open the .dmg, you can easily drag the Firefox app into the Applications folder. This was then moved to python in bug 1190522. Though looking at this again, it would probably be more straightforward to simply skip creating the symlink if it already exists?

FWIW, the symlink filename is a space because if we made it named 'Applications' it wouldn't be properly localized on non-en-US systems.

Comment 16

[Michael Shal [:mshal]]

Comment on attachment 8849643 [details]
Bug 1347576 - Allow 'mach repackage' to work from environment variables;

https://reviewboard.mozilla.org/r/122436/#review126498

::: commit-message-99eea:4
(Diff revision 2)
> +configure variables. However, this means we would need to run configure
> +before repackaging a signed tarball into a dmg. Instead we can

I've updated the comment, though I'm not totally tied to adding environment support. Mostly it just seemed a bit silly to me to run configure only to get some environment settings, which we could just as easily put into the task definition instead of a mozconfig. If you think this adds unnecessary complexity, we can drop it.

::: python/mozbuild/mozpack/dmg.py:135
(Diff revision 2)
>              raise Exception('Required tool "%s" at path "%s" is not executable' % (tool, path))
> +        cross_tools[tool] = path

Not sure I follow - are you saying to remove check_tools() and then instead of cross_tools['DMG_TOOL'] we could do get_tool('DMG_TOOL') where get_tool() looks at buildconfig or the environment?

I think the benefit of checking all the tools early is so that it fails before trying to do anything, but I agree that it's now weird for a "check" function to set some global stuff. If I understood you above, I think I can make the check_tools() back into only doing checks, and have get_tool() just return the subst or env variable as appropriate.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review124706

> I thought this was a reasonable place since archive.py exists in the same directory. The wrapper function was so we could fairly easily plug different unpacking / repacking functions into repackage() based on things like the file extension (or command-line argument, etc), though that would be more apparent once we add Windows support. However, maybe it makes more sense to just have 'mach repackage' call out directly to repackage_dmg (or in the future, repackage_windows_installer), and do the untar logic there.
> 
> What do you think of moving most of the repackage() and unpack_archive() functions inside repackage_dmg? Then repackage() can delegate to repackage_dmg if passed a --dmg flag or something (which would be the only valid flag until Windows is added).

Moving things inside `repackage_dmg` does seem cleaner for the moment.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849643 [details]
Bug 1347576 - Allow 'mach repackage' to work from environment variables;

https://reviewboard.mozilla.org/r/122436/#review126498

> Not sure I follow - are you saying to remove check_tools() and then instead of cross_tools['DMG_TOOL'] we could do get_tool('DMG_TOOL') where get_tool() looks at buildconfig or the environment?
> 
> I think the benefit of checking all the tools early is so that it fails before trying to do anything, but I agree that it's now weird for a "check" function to set some global stuff. If I understood you above, I think I can make the check_tools() back into only doing checks, and have get_tool() just return the subst or env variable as appropriate.

Yes, that's the idea. The idea of doing the checks early and failing before trying to do anything sounds like the start of an argument for leaving this to configure!

I'm not particularly opinionated on this point, it seems like a straightforward job to do either way.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Responded in mozreview.

Comment 20

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review126840

::: python/mozbuild/mozpack/dmg.py:136
(Diff revision 2)
> +    try:
> +        os.remove(mozpath.join(source_directory, ' '))

Actually, I forgot it's really the rsync command that breaks (as mentioned in the comment) rather than the part that always creates the symlink. So we can't just check if the symlink exists and not re-create it, and I'm not sure if fiddling with the rsync command will break other things.

Comment 21

[Michael Shal [:mshal]]

(In reply to Chris Manchester (:chmanchester) from comment #18)
> Yes, that's the idea. The idea of doing the checks early and failing before
> trying to do anything sounds like the start of an argument for leaving this
> to configure!

Excellent point :). Callek and I are testing out using configure. Configure would be required anyway if we ended up doing the repackaging on a mac, so it makes more sense to just always use it. I'll abandon the 2nd patch when the new review request is posted.

Comment 22

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122434/diff/2-3/

Comment 23

[Justin Wood (:Callek)]

(In reply to Michael Shal [:mshal] from comment #21)
> Excellent point :). Callek and I are testing out using configure. Configure
> would be required anyway if we ended up doing the repackaging on a mac, so
> it makes more sense to just always use it. I'll abandon the 2nd patch when
> the new review request is posted.

To be clear, this configure test has passed, now awaiting this review.

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review127390

Just nits here, thanks!

::: python/mozbuild/mozbuild/mach_commands.py:1598
(Diff revision 3)
> +             description='Repackage artifacts into different formats.')
> +    @CommandArgument('--input', '-i', type=str, required=True,
> +        help='Input filename')
> +    @CommandArgument('--output', '-o', type=str, required=True,
> +        help='Output filename')
> +    def repackage(self, input, output):

I think it's considered bad style to shadow a builtin, which I just realized `input` is.

::: python/mozbuild/mozbuild/repackage.py:13
(Diff revision 3)
> +import shutil
> +import ConfigParser
> +import mozpack.path as mozpath
> +
> +def repackage_dmg(input, output):
> +    from mozpack.dmg import create_dmg

Any reason to defer this import?

::: python/mozbuild/mozbuild/repackage.py:22
(Diff revision 3)
> +
> +    tmpdir = tempfile.mkdtemp()
> +    try:
> +        with tarfile.open(input) as tar:
> +            tar.extractall(path=tmpdir)
> +            tar.close()

The context manager should close the tarfile for us.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review127392

::: python/mozbuild/mozbuild/repackage.py:28
(Diff revision 3)
> +
> +        # Remove the /Applications symlink. If we don't, an rsync command in
> +        # create_dmg() will break, and create_dmg() re-creates the symlink anyway.
> +        try:
> +            os.remove(mozpath.join(tmpdir, ' '))
> +        except OSError, e:

PEP 8 prefers `except OSError as e:` here.

Comment 26

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review127390

> Any reason to defer this import?

dmg.py imports buildconfig, so I didn't want to force repackage.py and/or mach_commands.py to try to possibly raise an error if configure hasn't been run yet. That way we can check for its existence and give a more reasonable error message. Is there a better way to handle that?

Comment 27

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122434/diff/3-4/

Comment 28

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fb3be69091b9 -d b37fc7dc0311: rebasing 385993:fb3be69091b9 "Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support; r=chmanchester" (tip)
merging python/mozbuild/mozbuild/mach_commands.py
warning: conflicts while merging python/mozbuild/mozbuild/mach_commands.py! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 29

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

https://reviewboard.mozilla.org/r/122434/#review127390

> dmg.py imports buildconfig, so I didn't want to force repackage.py and/or mach_commands.py to try to possibly raise an error if configure hasn't been run yet. That way we can check for its existence and give a more reasonable error message. Is there a better way to handle that?

Meant to respond here earlier. An option would be to put the import of `repackage` in the body of the mach command after our check for a build environment there. We're supposed to keep as many imports as we can out of the top level of `mach_commands.py` files anyway.

Comment 30

[Michael Shal [:mshal]]

(In reply to Chris Manchester (:chmanchester) from comment #29)
> Meant to respond here earlier. An option would be to put the import of
> `repackage` in the body of the mach command after our check for a build
> environment there. We're supposed to keep as many imports as we can out of
> the top level of `mach_commands.py` files anyway.

Fortunately the autoland bounced, so I'll fix this before trying again :)

Comment 31

[Michael Shal [:mshal]]

Comment on attachment 8849642 [details]
Bug 1347576 - Add a 'mach repackage' command, with OSX dmg support;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122434/diff/4-5/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7049cb5450d8
Add a 'mach repackage' command, with OSX dmg support; r=chmanchester

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7049cb5450d8