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.

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

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

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.

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

(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.

Ted: Do you have any ETA on this? (Meanwhile, we'll start looking at doing this on linux, win32 in bug#457753, bug#486783. )

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

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!)

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.

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.

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

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

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)

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 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.)

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 on attachment 395915 [details] [diff] [review] use a packaging manifest Could you perhaps fold the $(PERL) line in packager.mk?

Comment on attachment 395915 [details] [diff] [review] use a packaging manifest Ok, try server builds look good.

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

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.

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

rs=ted on irc http://hg.mozilla.org/mozilla-central/rev/5baf439a991a

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

(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

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

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.

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 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.

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.

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 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.

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

(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.