Closed Bug 463605 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox Build System :: General, defect)

x86
macOS
defect
Not set

Tracking

(status1.9.2 beta1-fixed)

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: ted, Assigned: ted)

References

Details

(Whiteboard: [ts])

Attachments

(3 files, 1 obsolete file)

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.
Blocks: 421611
No longer blocks: 463455
No longer blocks: 421611
Blocks: 372581
Blocks: 383136
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.
Blocks: 486783
No longer blocks: 383136
Assignee: nobody → ted.mielczarek
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.
Whiteboard: [ts]
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.
Status: NEW → ASSIGNED
Blocks: 511642
Attached patch first pass (obsolete) — Splinter Review
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.
Attachment #395650 - Attachment is obsolete: true
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.
Attachment #395915 - Flags: review?(benjamin)
Attachment #395915 - Flags: review?(benjamin) → review+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/0bd17bd1cbaf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
Target Milestone: --- → mozilla1.9.3a1
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.
Attachment #395915 - Flags: approval1.9.2?
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.
Attachment #395915 - Flags: approval1.9.2? → approval1.9.2+
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.
Attachment #406724 - Flags: review?(benjamin)
Attachment #406724 - Attachment is patch: true
Attachment #406724 - Attachment mime type: application/octet-stream → text/plain
Attachment #406724 - Flags: review?(benjamin) → review+
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.
Attachment #406724 - Flags: approval1.9.2?
Attachment #406724 - Flags: approval1.9.2? → approval1.9.2+
Depends on: 542004
(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.
Depends on: 564656
Blocks: 564656
No longer depends on: 564656
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.