Closed Bug 1347576 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Callek, Assigned: mshal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Mike is working on this
Blocks: 1347579
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(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 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.
Attachment #8849642 - Flags: review?(cmanchester)
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.
Attachment #8849643 - Flags: review?(cmanchester)
(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?
Flags: needinfo?(cmanchester)
Nope, I think that covers it. Thanks!
Flags: needinfo?(cmanchester)
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).
chmanchester: Looking for feedback on #c13 - thanks!
Flags: needinfo?(cmanchester)
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 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 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 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.
Responded in mozreview.
Flags: needinfo?(cmanchester)
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.
(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.
Attachment #8849643 - Attachment is obsolete: true
(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 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.
Attachment #8849642 - Flags: review?(cmanchester) → review+
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 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?
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 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.
(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 :)
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7049cb5450d8
Add a 'mach repackage' command, with OSX dmg support; r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/7049cb5450d8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.