Closed Bug 547599 Opened 10 years ago Closed 10 years ago

Port the comm-central "make package-compare"


(Firefox Build System :: General, defect)

Not set


(Not tracked)



(Reporter: philor, Assigned: philor)




(2 files)

Attached patch Fix v.1Splinter Review
It's not very pretty, since it still doesn't know to expand directories or wildcards, and it still doesn't know about files that you know you don't want to package (particularly ugly for Fx, where pretty much every build has tests enabled and thus builds a ton of files you don't want to package, many of them poorly named), but for all that, I know that Thunderbird and SeaMonkey are trying to package contentprefs.xpt when it isn't there anymore and you don't (though you probably could from "possible missing or unnecessary"), and they are packaging widget_cocoa.xpt because they know it's there, and you don't.

The two biggest benefits are when it's added as a build step on tryserver (I review patches to and patches that add or remove files by pushing them to the MoMo tryserver and looking at the package-compare step on all three platforms), and when someone finally gets around to teaching it to ignore things listed in, so it can turn the tree orange when someone does anything to add or remove a file without saying what they want done with it, but even just having it exist, so I don't have to remember to copy-modify-paste the commands out of mail/installer/ when I want to see what's going wrong in Firefox packaging, is worth having, and also might get someone to work on improving it.
Attachment #428093 - Flags: review?(ted.mielczarek)
Attached file Sample Mac output
Note both the ton o' noise, and that you're trying to package .autoreg when you maybe need to be doing something to create it if you want to ship it, and that libbrowserdirprovider.dylib isn't there to package anymore, and that if you don't want to package nsProgressDialog.js you should maybe look into not building it, either, and that it's a bit odd that parentalcontrols.xpt is built on all platforms, but isn't in the manifest for any.
I originally wrote this as a quick hack so that we notice when we miss something in packaging, and a number of comm-central users seem to really like it despite its slight shortcomings ;-)
Blocks: 547915
Comment on attachment 428093 [details] [diff] [review]
Fix v.1

>diff --git a/browser/installer/ b/browser/installer/
>--- a/browser/installer/
>+++ b/browser/installer/
>@@ -118,12 +118,38 @@ else
> endif
> libs::
> 	$(MAKE) -C $(DEPTH)/browser/locales langpack PKG_LANGPACK_PATH=
>+ifeq (Darwin, $(OS_ARCH))
>+BINPATH = bin

Just combine this with the earlier block that deals with -DBINPATH. (You're double-defining it here currently.)

>+ifeq (WINNT,$(OS_ARCH))
>+ifeq (Darwin, $(OS_ARCH))
>+FINDPATH = $(_APPNAME)/Contents/MacOS
>+	cd $(DIST); find $(PKGCOMP_FIND_OPTS) $(FINDPATH) -type f | sort > bin-list.txt
>+	grep "^$(BINPATH)" $(MOZ_PKG_MANIFEST) | sed -e 's/^\///' | sort > $(DIST)/pack-list.txt
>+	-diff -u $(DIST)/pack-list.txt $(DIST)/bin-list.txt

This rule needs to depend on $(MOZ_PKG_MANIFEST), in case it gets run without having previously run "make package".

You should probably rm bin-list.txt and pack-list.txt at the end of this rule for cleanliness' sake.

r=me with those changes.

As for cleaning up the output, instead of having a list of things to ignore, I'd actually prefer if we fixed the build system to not put stuff we don't intend to ship into dist/bin. That should reduce the differences between "running a build from dist/bin" and "running from a package", which I know causes problems even to this day (although at least we mostly catch them in unittests now). For example, we can make CPP_UNIT_TESTS install to $(DIST)/test/bin, and always build stuff like xpt_link etc into $(DIST)/host/bin. (But those are followup bugs, of course.)
Attachment #428093 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #3)
> This rule needs to depend on $(MOZ_PKG_MANIFEST), in case it gets run without
> having previously run "make package".

Interesting thought, I guess we should backport that change to the comm-central apps as well, then.

Ted, I fully agree with you on the cleaning of the output. Turning on tests in all builds has made that output quite a bit noisier, but it's still quite helpful and I'm happy that Firefox will have it as well in the future.
Phil, thanks for caring about it and porting it over, I wanted to see this happen ever since I brought the first version of it to SeaMonkey and Thunderbird!
After _far_ too long trying to remember what I was thinking redefining BINPATH, I finally realized that I copy-pasted from Fx to define it for Tb, and then copy-pasted it back again :(
Blocks: 551182
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a3
Blocks: 551638
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 3.7a3 → mozilla3.7a3
You need to log in before you can comment on or make changes to this bug.