547599 - Port the comm-central "make package-compare"

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

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 package-manifest.in 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 no-package-manifest.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/Makefile.in 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.

Comment 1

[Phil Ringnalda (:philor)]

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

Comment 2

[Robert Kaiser]

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

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 428093 [details] [diff] [review]
Fix v.1

>diff --git a/browser/installer/Makefile.in b/browser/installer/Makefile.in
>--- a/browser/installer/Makefile.in
>+++ b/browser/installer/Makefile.in
>@@ -118,12 +118,38 @@ else
> DEFINES += -DBINPATH=bin
> endif
> 
> libs::
> 	$(MAKE) -C $(DEPTH)/browser/locales langpack PKG_LANGPACK_PATH=
> 
> UPLOAD_EXTRA_FILES += $(PKG_LANGPACK_BASENAME).xpi
> 
>+ifeq (Darwin, $(OS_ARCH))
>+BINPATH = $(_BINPATH)
>+DEFINES += -DAPPNAME=$(_APPNAME)
>+else
>+BINPATH = bin
>+endif
>+DEFINES += -DBINPATH=$(BINPATH)

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

>+
>+ifeq (WINNT,$(OS_ARCH))
>+PKGCOMP_FIND_OPTS =
>+else
>+PKGCOMP_FIND_OPTS = -L
>+endif
>+ifeq (Darwin, $(OS_ARCH))
>+FINDPATH = $(_APPNAME)/Contents/MacOS
>+else
>+FINDPATH=bin
>+endif
>+
>+package-compare::
>+ifdef MOZ_PKG_MANIFEST_P
>+	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
>+endif

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

Comment 4

[Robert Kaiser]

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

Comment 5

[Phil Ringnalda (:philor)]

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

Comment 6

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/e32cb0065bb7