498500 - Mac DMG unpackaging is unreliable

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert Kaiser]

The Mac DMG unpackaging script/macro in packager.mk is unreliable as it tries to get deterministic output from hdiutil, while that tool does actually perform asychronously (grr, how I hate async commandline tools). This gets especiall visible on the virtualized Macs we have at SeaMonkey now, nightly L10n repacks fail sometimes, the release L10n repacks failed consistently when they ran with the default unpack command.
I adopted a script catlee wrote for talos, and the rewritten unpack performed nicely in testruns. We'll need this in 1.9.1 ASAP if we want to use release automation for SeaMonkey 2.0 Beta 1.

Patch coming as soon as I'm back on my normal machine that has it.

Comment 1

[Robert Kaiser]

Attachment: make it safer, use catlee's script

This patch uses a variation of catlee's script and has run reliably through SeaMonkey release automation testing.
I tried to keep changes to the packager.mk script small, putting more into the script would be possible but it would mean pushing a larger number of makefile variables to the script.

The main changes here are to 1) specify a mount point so that we don't depend on hdiutils output, 2) wait for the actual mount to finish with a rather ugly while loop as hdiutil isn't synchronous.

The talos script from catlee is at http://hg.mozilla.org/build/tools/file/b680aba8e49a/buildfarm/utils/installdmg.sh - I basically just took it and adopted it to fit into the main build process.

Comment 2

[Robert Kaiser]

This could possibly be regarded NPOTB by the way, as we're not actually doing unpack during normal builds. AFAIK, we're only using it for L10n repackaging.

Comment 3

[Robert Kaiser]

Attachment: v1.1: deal with spaces in package names, remove obsolete .ex file

Here's a small update to the earlier patch, quoting the DMG name when handing it to hdiutil so that unpackaging a name with spaces works fine (we're producing those with "pretty names" for releases), and also removing the now obsolete .ex file.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 384404 [details] [diff] [review]
v1.1: deal with spaces in package names, remove obsolete .ex file

+# How long to wait before giving up waiting for the mount to fininsh

I assume this is in seconds? Could you explicitly state that? Also, typo: "fininsh".

+while [ "$(echo $MOUNTPOINT/*)" == "$MOUNTPOINT/*" ]; do

This seems a little weird. Could you grep the output of "mount" or something instead? Not going to r- on it, but it looks odd.

+# Sleep for a bit to let messages from diskimage-helper go away

What does this mean exactly?

+_ABS_DIST = $(shell cd $(DIST) && pwd)
You can use:
_ABS_DIST = $(call core_abspath,$(DIST))
nowadays, which doesn't result in a shell spawning. (Not sure if that landed on 1.9.1, so double-check if you go to land this there!)

Comment 5

[Robert Kaiser]

I've asked catlee to comment on your comments, as the shell script is actually his work, so he knows the details behind the while and the sleep comment better than me.

What I can confirm though is that the core_abspath patch has only landed in config.mk on trunk, on 1.9.1 I only find that identifier directly defined in some makefiles that use it. Should I do that for a 1.9.1 patch as well and define it in packager.mk there?

Comment 6

[Chris AtLee [:catlee]]

(In reply to comment #4)
> (From update of attachment 384404 [details] [diff] [review])
> +# How long to wait before giving up waiting for the mount to fininsh
> 
> I assume this is in seconds? Could you explicitly state that? Also, typo:
> "fininsh".

Yes, in seconds.

> +while [ "$(echo $MOUNTPOINT/*)" == "$MOUNTPOINT/*" ]; do
> 
> This seems a little weird. Could you grep the output of "mount" or something
> instead? Not going to r- on it, but it looks odd.

This seemed like the safest thing to do, since our rsync later on was dying because files the glob didn't expand to anything.  I'm a bit worried that grepping the output of 'mount' could have similar timing issues where the image is mounted, but the helper process hasn't finished whatever it's doing.


> +# Sleep for a bit to let messages from diskimage-helper go away
> 
> What does this mean exactly?

After the image is unmounted, diskimage-helper prints a few lines to stdout.  This also happens asynchronously, so if the current build step completes and another starts up immediately afterwards, the output from diskimage-helper can appear in the stdout of the next command somehow (buildbot is probably sharing a single PTY for all commands, and diskimage-helper is sending output to that PTY?), which breaks things if you're trying to parse the next command's output.

Comment 7

[(not currently active) Ted Mielczarek]

Ok, those are reasonable. Perhaps a few more comments in the script to clarify? Clearly this is just a set of workarounds anyway, so it can't hurt to explain them as much as possible.

Comment 8

[Robert Kaiser]

Pushed to trunk as http://hg.mozilla.org/mozilla-central/rev/33475a1b5746 - will create a 1.9.1 patch including all the comment fixes (but excluding the call_abspath as that's not in config.mk on branch).

Comment 9

[Robert Kaiser]

Attachment: 1.9.1 version of the patch

Forwarding ted's review from the trunk patch to this and requesting approval.

The patch could be considered NPOTB as it's only about (un)packaging scripts but as all builds on tinderbox run packager.mk in some way and L10n repacks actually run the unpackaging, even on 1.9.1, so going through approval is probably a good idea.

Comment 10

[Robert Kaiser]

As a note, Firefox Mac L10n repacks on mozilla-central succeeded for today's builds where this patch was already in, so that verifies that nothing breaks and things work fine with it.

Comment 11

[Robert Kaiser]

Localizers are getting nervous as they have red trees for SeaMonkey repackaging and I'm getting nervous as this blocks using release automation for SeaMonkey 2.0 Beta 1 which should be built this week from 1.9.1 - is there any chance to get this approval very soon?

Comment 12

[Rail Aliiev [:rail]]

Add CC

Comment 13

[Robert Kaiser]

Comment on attachment 386998 [details] [diff] [review]
1.9.1 version of the patch

1.9.1.1 has been cut, request 1.9.1.2 approval instead

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 386998 [details] [diff] [review]
1.9.1 version of the patch

a1912=beltzner

Thanks for your patience Kairo.

Comment 15

[Robert Kaiser]

Thanks Mike, pushed to 1.9.1 as http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1e7c406956d3

Comment 16

[Tony Chung [:tchung]]

qa is going through bug verifications for 3.5.2, and unsure how to verify this one.  Was this bug in any relation to the mac dmg packaging issue to https://bugzilla.mozilla.org/show_bug.cgi?id=489313#c9?  

Chris, Kairo, If this is fixed for 3.5.2 builds, please help us by marking it verified1.9.1 in the keyword.  thanks

Comment 17

[[e-mail] (:bhearsum)]

(In reply to comment #16)
> qa is going through bug verifications for 3.5.2, and unsure how to verify this
> one.  Was this bug in any relation to the mac dmg packaging issue to
> https://bugzilla.mozilla.org/show_bug.cgi?id=489313#c9?  
> 
> Chris, Kairo, If this is fixed for 3.5.2 builds, please help us by marking it
> verified1.9.1 in the keyword.  thanks

I don't think we really hit this for Firefox in the first place, but we didn't hit it in 3.5.2, that's for sure.

Comment 18

[Robert Kaiser]

(In reply to comment #16)
> Chris, Kairo, If this is fixed for 3.5.2 builds, please help us by marking it
> verified1.9.1 in the keyword.  thanks

The patched code is not part of the builds themselves, it's part of the build system. I can verify that SeaMonkey L10n repacks on the 1.9.1 branch, which failed before, did start working as soon as this patch was in, but for verifying this with Firefox you'd need to find a machine with slow I/O on which this failed before and have the build system on it and use it to unpack a DMG with "make unpack".

In any case, as I saw the SeaMonkey boxes that showed the problem start to work with the 1.9.1 build system as soon as this patch was in, I'll mark it verified.