304160 - Fix XULRunner mac tinderbox and build process

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect, P1)

Description

[Benjamin Smedberg]

Currently the mac xulrunner tbox is red. There are two reasons for this:

1) the tinderbox config is looking for
dist/XULRunner.app/Contents/MacOS/xulrunner-bin when it should be looking for
dist/XUL.framework/Versions/Current/xulrunner-bin

2) We need to ship the XUL framework as an installer.pkg, not an app bundle
(still inside a DMG).

step 1 is for chase/coop, step 2 is for me.

Step 2) unfortunately involves some hackery with commands executed as root: in
one part of the install process, it is necessary to chmod -R root:admin <a
directory> which requires root privs (and it would be good to "undo" this task
after packaging is finished). To do this automatically from a makefile probably
involves putting a suid script somewhere.

Comment 1

[Benjamin Smedberg]

Attachment: Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]

Comment 2

[Benjamin Smedberg]

Attachment: The source I'm using for setuid chown_root

This or something like it will need to be installed as a setuid program (owned
by root) on the tinderbox build machine.

Comment 3

[Benjamin Smedberg]

Attachment: The source I'm using for setuid chown_revert

Same same, for chown_revert

Comment 4

[Mark Mentovai]

pax files, as in *.pkg/Contents/Archive.pax.gz are just cpio archives.  How do
you feel about fixing up the uid and gid of each entry in the cpio header after
running PackageMaker?  It can be done by pipeline in a trivial script.  I think
I prefer that to suid wrappers, and it can be integrated into the build.  We'd
need to re-run mkbom to get a proper Archive.bom afterwards.  No biggie.

Comment 5

[Benjamin Smedberg]

Mark, that sounds fine, if you can help write it, at least providing significant
examples... I don't know how. Does mkbom take the CPIO file as input?

Comment 6

[Mark Mentovai]

I thought it did, but it seems to only operate on expanded directories.  I'll
see if it's trivial to transform the bom file too - if not, I guess there's no
choice other than making Chase install setuid helpers.

The scripts would be a piece of cake, if it's possible, I'll smack 'em together
later.

Comment 7

[Mark Mentovai]

Unfortunately, the bom file format isn't publicly documented, nor are the APIs
into the bom framework.  I can see the proper fields in the bom file, but I
don't think it's wise to go poking around like this, and I don't think
reverse-engineering it is an effective use of time.

Apple really isn't leaving us with much of a choice here.  Thanks, Apple.

Comment 8

[Chris Cooper [:coop] (he/him)]

(In reply to comment #0)
> Currently the mac xulrunner tbox is red. There are two reasons for this:
> 
> 1) the tinderbox config is looking for
> dist/XULRunner.app/Contents/MacOS/xulrunner-bin when it should be looking for
> dist/XUL.framework/Versions/Current/xulrunner-bin
> 
> step 1 is for chase/coop, step 2 is for me.

Made some intial changes to the build scripts, and we've gone green:

http://tinderbox.mozilla.org/showlog.cgi?log=XULRunner/1123854540.12639.gz&fulltext=1

Still some packaging problems at the end of the process that I'm looking into:

building file list ... link_stat
"/builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/Mozilla.app"
failed: No such file or directory

Comment 9

[Chris Cooper [:coop] (he/him)]

(In reply to comment #8)
> Still some packaging problems at the end of the process that I'm looking into:
> 
> building file list ... link_stat
> "/builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/Mozilla.app"
> failed: No such file or directory

This will apparently be fixed by step 2.

Comment 10

[Chris Cooper [:coop] (he/him)]

Attachment: tinderbox build script changes for XULRunner/MacOS [checked in]

This patch does the following:

* changes $DistBin for XULRunner on MacOS; 
* puts /sw/bin at the front of $PATH on MacOS;
* fixes perl function warning in encode_log

Comment 11

[Benjamin Smedberg]

Mark, unless you're holding out hope for some sort of BOM manipulation can you
review the patch I've got?

Comment 12

[Mark Mentovai]

Comment on attachment 192388 [details] [diff] [review]
Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]

So, .bom is a bomb.

This is good for now.  Eventually, more of the .pkg making logic should
probably be integrated into packager.mk, the .pkg will be a better place for
the EULA than the .dmg, etc.

Notes on this patch: You've got to chmod package-stage so that it's
world-readable and executable where appropriate.  Permissive umasks aren't
guaranteed, and the user that builds the .pkg needs to be able to get at the
files and directories owned by root:admin.  Doing this before $(CHOWN_ROOT) is
good:

chmod -R a+rX,u+w,go-w $(DIST)/package-stage && \

You probably also want to clear the s/t bits.  It would look really bad to
accidentally ship something that installs setuid programs when not necessary
(although a few major software publishers do it and apparently nobody has
noticed or cares.)

This error mesage and the one for CHOWN_REVERT:

+CHOWN_ROOT ?= $(error CHOWN_ROOT must be set to a setuid script.)

are misleading because of the use of the word "script."

I'm not sure if you meant to seek review of two utilities, my r+ only applies
to this patch file with the above changes.

I do have concerns about the implementation of chown_root and chown_revert. 
I'd be frightened as hell to have either installed setuid on my system, even if
restricted to a sandboxed build user.  chown_root, because it doesn't clear the
s-bits.  The safest way to go would be to have chown_root do its own recursion,
chowning with fchown, and clearing any s-bits with fchmod.  chown_revert should
at least check ownership of the parent directory.

Building and installing them obviously can't be part of the build, so I don't
know if you plan on putting either utility's source in cvs.  I think it's a
good idea for them to be in the tree, but there's no way they could be added in
their current states.

As for not integrating them into the tree and putting them on the
tinderbuilders manually, I guess that's Chase's call.

Comment 13

[Mark Mentovai]

P.S. that's a capital X in my chmod.

Comment 14

[Benjamin Smedberg]

Somebody is welcome to make chown_root/chown_revert a lot safer; unix system
security is not my specialty. It should probably only work on specified
hardcoded paths (make sure that the argument starts with /builds or /tinderbox
or something). Adding a chmod -s (or C equivalent) sounds like a good idea as well.

Comment 15

[Benjamin Smedberg]

Comment on attachment 192388 [details] [diff] [review]
Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]

This patch is checked in.

Comment 16

[Benjamin Smedberg]

Coop, this is all yours now ;-)

The tinderbox variables need to be updated so that instead of building
xpinstall/packager they build xulrunner/installer (just like browser/installer
or mail/installer)... and you need to set CHOWN_ROOT and REVERT_ROOT in the
environment and compile/install them as setuid root programs.

Comment 17

[Chase Phillips]

columbia's config has been updated and these apps installed.  It should cycle
shortly.

Comment 18

[Chase Phillips]

(In reply to comment #16)
> Coop, this is all yours now ;-)
> 
> The tinderbox variables need to be updated so that instead of building
> xpinstall/packager they build xulrunner/installer (just like browser/installer
> or mail/installer)... and you need to set CHOWN_ROOT and REVERT_ROOT in the
> environment and compile/install them as setuid root programs.

s/REVERT_ROOT/CHOWN_REVERT/

Comment 19

[Chase Phillips]

The build appears to have failed with the following error:

...
exit $SAVED
2005-08-22 12:51:35.820 PackageMaker[14120] ** building a package at
/builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/xulrunner-1.9a1.en-US.mac.pkg
**
dyld:
/Developer/Applications/Utilities/PackageMaker.app/Contents/MacOS/PackageMaker
malformed library:
/Developer/SDKs/MacOSX10.2.8.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/CarbonCore
(not a Mach-O library file, bad filetype value)
make[2]: *** [libs] Error 133
make[1]: *** [libs] Error 2
make: *** [all] Error 2
No files to copy
...

The full build log can be seen at:
http://tinderbox.mozilla.org/showlog.cgi?log=XULRunner/1124738640.16774.gz&fulltext=1

Let me know if you'd like more info from the system.

Comment 20

[Mark Mentovai]

Some versions of Apple's tools are a little overzealous about using $NEXT_ROOT.
 The libraries in the SDK are only stubs and are intended for linking only, not
loading.

This needs:

 unset NEXT_ROOT; \

before $(CHOWN_ROOT)

Comment 21

[Chase Phillips]

Mark, the change you suggest is for mozilla/, not the build scripts, right? 
Handing back to [e-mail] who is handling the mozilla/ side of
things on this one.

Please reassign to me if the requested change needs to happen on the build
system instead.

Comment 22

[Mark Mentovai]

Chase, the unset NEXT_ROOT fix is for the tree.  :bs landed it shortly after I
suggested it.  I assume this is still open for 1.8 branch lovin' - but if
there's no branch xulrunner tinderbuilder, I guess there's no build script work
to be done.

Your reassignment seemed not to stick, I'm trying again.

Comment 23

[Benjamin Smedberg]

Fixed on 1.8branch as well (with the NEXT_ROOT patch).

Comment 24

[Benjamin Smedberg]

I have checked in the source of chown_root.c and chown_revert.c into mozilla/build/macosx/permissions