463605 - make Mac OS X packaging use a packaging manifest (like Windows and Linux)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

The diff in attachment 346833 [details] on bug 460282 shows that if you --enable-tests, we package all kinds of crap on OS X. This is because the Mac packaging doesn't use a packaging manifest like Windows or Linux. I believe this is because the packaging process currently just does an rsync to get the contents of dist/bin into the app bundle. If we're going to ever enable tests on our build machines, we'll need to fix this somehow.

Comment 1

[Dave Townsend [:mossop]]

Do you think there is any value in changing to use a single packaging manifest that is preprocessed for all platforms?

Comment 2

[(not currently active) Ted Mielczarek]

Might make people's lives easier instead of hunting about for various packaging manifests.

Comment 3

[Phil Ringnalda (:philor)]

Having a single one would be a lot more pleasant if we drop USE_SHORT_LIBNAME for Windows, so we could have a single @DLL_PREFIX@browsercomps@DLL_SUFFIX@ instead of having to #ifdef XP_WIN brwsrcmp.dll #else.

Comment 4

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Ted: is this a "nice to have cleanup" or is this needed for us to be able to run-unittest-separate-from-builds?

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to comment #4)
> Ted: is this a "nice to have cleanup" or is this needed for us to be able to
> run-unittest-separate-from-builds?

It's necessary in order to be able to build our mac hourly/nightly builds with tests enabled, and run the tests on them. Otherwise the Mac build will package and ship a bunch of random test junk.

Comment 6

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Ted:  Do you have any ETA on this?

(Meanwhile, we'll start looking at doing this on linux, win32 in bug#457753, bug#486783. )

Comment 7

[Dietrich Ayala (:dietrich)]

marking [ts], since the resulting linkage of xpt files will help startup.

Comment 8

[(not currently active) Ted Mielczarek]

I talked with adw on IRC, and I think we can get the xpt linking with a quick hack of a patch instead of blocking on this. (and we could probably get that into 1.9.2, as well!)

Comment 9

[Drew Willcoxon :adw]

Thanks, Ted.  After talking with Dietrich I've filed bug 510309 for the quick patch.

Dietrich, I marked bug 510309 [ts] but have left this bug [ts] as well, since bug 510309 is a hack to ultimately be subsumed by this bug.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: first pass

Here's a first pass that seems to mostly work. I need to do a full rebuild to check some things out. Notably broken with this patch is universal build support, but that shouldn't be hard to fix. The packages-static file is just a copy of the unix one with some fiddling, so there may be some things missing there still.

Comment 11

[Phil Ringnalda (:philor)]

Don't forget to remove that lie about addDirectory in ab-CD.jst.

Comment 12

[(not currently active) Ted Mielczarek]

Should have this basically finished either today or early next week (travelling on Monday, so more likely Tuesday if not today).

Comment 13

[(not currently active) Ted Mielczarek]

For reference, there are a few files being shipped with current Mac builds that I intend to stop packaging, since we don't ship them with Linux or Windows builds:
dependentlibs.list (only useful for xulrunner, AFAIK)
firefox (the shell script)
run-mozilla.sh
components/nsProgressDialog.js (? not shipped in Win/Linux)

Comment 14

[(not currently active) Ted Mielczarek]

Attachment: use a packaging manifest

Running this through the try server to make sure Universal Builds work fine, as well as Windows builds. I tried this on a Mac x86 build as well as a Linux build, and they both seem ok.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 395915 [details] [diff] [review]
use a packaging manifest

Some notes to help a future review, since some of this code makes no sense:
+ifeq (Darwin, $(OS_ARCH))
+DEFINES += \
+  -DBINPATH=$(_BINPATH) \
+  -DAPPNAME=$(_APPNAME) \

$(_BINPATH) is the full path inside the bundle to where the binaries live. $(_APPNAME) is just the name of the bundle directory.

+++ b/browser/installer/removed-files.in
+components/firefox.xpt

The patch in bug 510309 made us link XPTs, but to "firefox.xpt". Every other platform links to "browser.xpt", since that's what the packaging manifest specifies.

+ifdef MOZ_PKG_MANIFEST
+	$(RM) -rf $(DIST)/xpt
+	$(call PACKAGER_COPY, "$(call core_abspath,$(DIST))",\
+		 "$(call core_abspath,$(DIST)/$(MOZ_PKG_DIR))", \
+		"$(MOZ_PKG_MANIFEST)", "$(PKGCP_OS)", 1, 0, 1)

Packager.pm says that the source and dest dirs are supposed to be absolute paths, so we were violating its assumptions here.

+	$(PERL) $(MOZILLA_DIR)/xpinstall/packager/xptlink.pl -s $(DIST) -d $(DIST)/xpt -f $(DIST)/$(MOZ_PKG_DIR)/$(_BINPATH)/components -v -x "$(XPIDL_LINK)" --debug=10

The $(_BINPATH) is to make sure the linked XPT winds up inside the bundle. The var will be empty on non-mac.

-      $destpathcomp = "$srcdir/xpt/$component";
+      $destpathcomp = "$srcdir/xpt/$component/components";
+      $altdest = "$srcname$srcsuffix";

Packager.pm special-cases XPT files in the components/ dir, and copies them to dist/xpt/$component. Except it also special cases them by stripping a leading "bin/" from their pathname, and preserves the rest of the path. This means that on non-mac, they wind up in dist/xpt/browser/components/, but on mac they were winding up in dist/xpt/browser/Minefield.app/Contents/MacOS/components, which then broke xptlink.pl. This change just ensures that they always wind up in dist/xpt/$component/components. (P.S. I hate all this Perl code and its pile of hacks.)

Comment 16

[(not currently active) Ted Mielczarek]

Also, if anyone wants to take a look at my packages-static and see if I'm missing (or including) anything obvious, please do. I sanity-checked (and got the list in comment 13) by downloading a nightly DMG, mounting it and then running:
diff -rq ../obj-firefox/dist/firefox/Minefield.app/ /Volumes/Minefield/Minefield.app/ | grep "Only in"

Comment 17

[Peter Weilbacher]

Comment on attachment 395915 [details] [diff] [review]
use a packaging manifest

Could you perhaps fold the $(PERL) line in packager.mk?

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 395915 [details] [diff] [review]
use a packaging manifest

Ok, try server builds look good.

Comment 19

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/0bd17bd1cbaf

Comment 20

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

ted: any chance of this on 1.9.2 and 1.9.1 also? We've need builds/unittests on those branches for a while.

Comment 21

[(not currently active) Ted Mielczarek]

This is a pretty small patch, it's not unreasonable.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

Attachment: Followup fixes

rs=ted on irc

http://hg.mozilla.org/mozilla-central/rev/5baf439a991a

Comment 23

[Nick Thomas [:nthomas] (UTC+12)]

Forgot to add the rationale

From the make package log:
* left alone
** Normal - defaults/existing-profile-defaults.js
** Disabled on mac - components/accessibility.xpt
* Removed from osx/packages-static
** Not built for mac: components/filepicker.xpt, components/nsFilePicker.js
   http://mxr.mozilla.org/mozilla-central/source/toolkit/components/filepicker/Makefile.in#47
** Not built for mac: components/toolkitremote.xpt (see configure.in)
** Removed by bug 221602: chrome/comm.jar, chrome/comm.manifest
** Removed by bug 499123: components/aboutPrivateBrowsing.js, aboutSessionRestore.js, aboutRights.js, aboutRobots.js, aboutCertError.js
* Remove accidental '--debug=10' from toolkit/mozapps/installer/packager.mk from patch testing

From diffing builds before and after checkin:
* added to removed-files.in
** components/firefox.xpt (from bug 510309) became components/browser.xpt
** Unneeded files no longer shipped - run-mozilla.sh, firefox, dependentlibs.list
** Shit that embedding dumped in dist/ - nsProgressDialog.js
* added to osx/packages-static
** Only built shared on mac - components/libalerts_s.dylib
   http://mxr.mozilla.org/mozilla-central/source/toolkit/components/alerts/src/mac/Makefile.in#45

Comment 24

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #15)
> +++ b/browser/installer/removed-files.in
> +components/firefox.xpt

Ted and I were wondering why I made the same change in the "Followup fixes" patch, yet there's only one firefox.xpt in removed-files.in. Looks like it was just an omission on landing as it's not in 
 http://hg.mozilla.org/mozilla-central/rev/0bd17bd1cbaf

Comment 25

[(not currently active) Ted Mielczarek]

Must have been a bad merge or got lost along the way somehow. Glad you caught it!

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 395915 [details] [diff] [review]
use a packaging manifest

RelEng would like to get this on the branches so we can run unit tests on nightly/release builds there as well. Looking for approval for this along with the followup patch here + the patch from bug 513067.

These patches are basically Mac-only and just change our packaging on Mac to use a manifest file like every other platform. There are no substantial changes to the bits we ship.

Comment 27

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This would also be nice for Calendar (and probably Thunderbird too), since it also fixes packaging directories recursively, i.e adding bin\modules\* to packages-static. This will make it much easier to package calendar try server builds and also simplify the standard packages-static.

Comment 28

[Benjamin Smedberg]

Comment on attachment 395915 [details] [diff] [review]
use a packaging manifest

a=me for the bits you need on the branch, but I'd like if we just took the unified packaging manifest there as well.

Comment 29

[(not currently active) Ted Mielczarek]

Pushed to 1.9.2 (rolled up the original + followup):
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/77d59f33be5a

And yes, I pushed the unified packaging manifest patch after that.

Comment 30

[(not currently active) Ted Mielczarek]

Attachment: stupid followup, don't re-run packager::Copy when packaging a universal binary

I guess I missed this, but since I landed this patch, we've been doing extra work when packaging universal binaries. Specifically, we've been running packager::Copy on the pre-staged universal bits even though those are produced by unifying two arch-specific dirs which were produced by packager::Copy. The old non-package-manifest code path just skipped copying entirely in a UNIVERSAL_BINARY build, and doing the same here works just fine.

Comment 31

[(not currently active) Ted Mielczarek]

Comment on attachment 406724 [details] [diff] [review]
stupid followup, don't re-run packager::Copy when packaging a universal binary

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/fff8cd985e80

Should probably take this on 1.9.2 as well, just because it's wasteful to leave it like this, and the previous patches landed there.

Comment 32

[Serge Gautherie (:sgautherie)]

http://hg.mozilla.org/comm-central/rev/a09a7d1c82d7
Trunk packager bustage fix

Comment 33

[Justin Wood (:Callek)]

(In reply to comment #23)
> ** Shit that embedding dumped in dist/ - nsProgressDialog.js

From my memory, this is cruft leftover from CVS when suite used the xpfe Download Manager. nsProgressDialog.js (and related files in embedding/ui) were actually for the xpfe download stuff. I'm unsure if any of this is used or needed in "today"s world, but I doubt it.