Fix XULRunner mac tinderbox and build process

RESOLVED FIXED

Status

Toolkit Graveyard
XULRunner
P1
normal
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

({fixed1.8})

unspecified
x86
Windows XP
fixed1.8
Bug Flags:
blocking1.8b5 +

Details

Attachments

(4 attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 192388 [details] [diff] [review]
Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]
Attachment #192388 - Flags: second-review?(chase)
Attachment #192388 - Flags: first-review?(mark)
(Assignee)

Comment 2

13 years ago
Created attachment 192390 [details]
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.
(Assignee)

Comment 3

13 years ago
Created attachment 192391 [details]
The source I'm using for setuid chown_revert

Same same, for chown_revert

Comment 4

13 years ago
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.
(Assignee)

Comment 5

13 years ago
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

13 years ago
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

13 years ago
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

13 years ago
(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

13 years ago
(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

13 years ago
Created attachment 192528 [details] [diff] [review]
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
Attachment #192528 - Flags: first-review?(chase)
(Assignee)

Comment 11

12 years ago
Mark, unless you're holding out hope for some sort of BOM manipulation can you
review the patch I've got?
Priority: -- → P1

Comment 12

12 years ago
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.
Attachment #192388 - Flags: first-review?(mark) → first-review+

Comment 13

12 years ago
P.S. that's a capital X in my chmod.
(Assignee)

Comment 14

12 years ago
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.

Updated

12 years ago
Attachment #192388 - Flags: second-review?(chase) → second-review+

Updated

12 years ago
Attachment #192528 - Flags: first-review?(chase) → first-review+
(Assignee)

Comment 15

12 years ago
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.
Attachment #192388 - Attachment description: Make a XULRunner installer .pkg and then DMG it, rev. 1 → Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]
(Assignee)

Updated

12 years ago
Attachment #192388 - Flags: approval1.8b4?
(Assignee)

Comment 16

12 years ago
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.
Assignee: benjamin → ccooper

Updated

12 years ago
Attachment #192528 - Attachment description: tinderbox build script changes for XULRunner/MacOS → tinderbox build script changes for XULRunner/MacOS [checked in]

Comment 17

12 years ago
columbia's config has been updated and these apps installed.  It should cycle
shortly.
Assignee: ccooper → chase

Comment 18

12 years ago
(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

12 years ago
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.
Status: NEW → ASSIGNED

Comment 20

12 years ago
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)

Updated

12 years ago
Flags: blocking1.8b4+

Updated

12 years ago
Attachment #192388 - Flags: approval1.8b4? → approval1.8b4+

Comment 21

12 years ago
Mark, the change you suggest is for mozilla/, not the build scripts, right? 
Handing back to benjamin@smedbergs.us 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

12 years ago
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.
Assignee: chase → benjamin
Status: ASSIGNED → NEW
(Assignee)

Comment 23

12 years ago
Fixed on 1.8branch as well (with the NEXT_ROOT patch).
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
(Assignee)

Comment 24

11 years ago
I have checked in the source of chown_root.c and chown_revert.c into mozilla/build/macosx/permissions
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.