511642 - use a single packaging manifest across all three platforms (with preprocessing)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

In bug 463605 I plan to make OS X use a packaging manifest like Windows and Linux. Once that's done, we should just combine all three packaging manifests into a single preprocessed file, so people don't have to go hunting for various packaging files.

Filing this as a followup to keep the scope of bug 463605 focused.

Comment 1

[(not currently active) Ted Mielczarek]

From bug 463605 comment 3:
 -------  Comment #3 From  Phil Ringnalda (:philor)   2008-11-23 23:47:15 PDT   (-) [reply] -------  
> 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.

I agree with this. I don't understand why we never dropped USE_SHORT_LIBNAME for Windows. I guess OS/2 still uses it, which is why we never dropped it completely.

Comment 2

[Peter Weilbacher]

Can't comment on Windows, but yes, OS/2 does still use USE_SHORT_LIBNAME. (The system cannot load DLLs with more than 8.3 chars in the filename, so no way around.)

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: unify packaging manifests

This works on mac, I need to push-to-try to check lin/win.

Comment 4

[(not currently active) Ted Mielczarek]

This breaks the Windows installer. Oops.

Comment 5

[(not currently active) Ted Mielczarek]

I think we might be able to just remove all references to packages-static in this makefile:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in#57

All the real magic happens in installer-stage, which is in packager.mk, so I don't think this makefile actually uses packages-static in a meaningful way.

Comment 6

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

Attachment: WinCE 'make package' output

Here's the log from testing on WinCE which you asked for Ted. The delta from a log without attachment 397123 [details] [diff] [review] is
* add '-DBINPATH=bin' when processing packages-manifest.in & removed-files.in
* s/packages-static/packages-manifest/g
* charset changes
 * add to package res/charsetData.properties, res/charsetalias.properties
 * start trying to package res/maccharset.properties
 * no longer package res/wincharset.properties
* stopped packaging LICENSE
* no more warnings trying to package Accessibility that wasn't built (woo!)
* failed to package components/proxyObjInst.xpt, should be components/proxyObject
* failed to package components/xpcom_threads.xpt, should be components/xpcom_thread.xpt
* Still warns about components/layout_printing.xpt, plugins/npnul32.dll, uninstall/helper.exe

proxyObject and xpcom_threads have platform specific name. Dunno what the charset stuff does.

Comment 8

[Phil Ringnalda (:philor)]

Dunno whether that ifdef SOLARIS is actually accurate for "32-bit OS_ARCH == SunOS, OS_TEST != 86" but I bet you'll be more successful at finding out by just landing it than I ever was by filing a bug and hoping for comments :)

Comment 9

[(not currently active) Ted Mielczarek]

Thanks for the info Nick! I'll clean that up in the next rev. Some of the platform differences here are bizarre. Definitely going to go through and fix most of those, perhaps after this patch.
Phil: yeah, I know that SOLARIS is at least sufficient ( http://mxr.mozilla.org/mozilla-central/source/configure.in#2544 ), and if it covers too many cases, well they can live with the packaging warnings or fix it themselves.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: rev 2, remove some installer/windows makefile bits

This just removes some bits of the installer makefiles. Rob: if you can run this through l10n repackaging, that would be cool. I think these bits are purely vestigal.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 397283 [details] [diff] [review]
rev 2, remove some installer/windows makefile bits

>diff --git a/browser/installer/windows/Makefile.in b/browser/installer/windows/Makefile.in
>--- a/browser/installer/windows/Makefile.in
>+++ b/browser/installer/windows/Makefile.in
>@@ -54,10 +54,6 @@
> PRE_RELEASE_SUFFIX := $(shell $(PYTHON) $(topsrcdir)/config/printprereleasesuffix.py $(APP_VERSION))
> DEFINES += -DPRE_RELEASE_SUFFIX="$(PRE_RELEASE_SUFFIX)"
> 
>-PP_LOCALIZED_FILES = \
>-	packages-static \
>-	$(NULL)
>-
> # All script and locale files used by the Unicode version of NSIS need to be
> # converted from UTF-8 to UTF-16LE
> INSTALLER_FILES_CONV = \
>@@ -112,9 +108,6 @@
> 	done
> 	$(INSTALL) $(addprefix $(DIST)/branding/,$(BRANDING_FILES)) $(CONFIG_DIR)
> 	$(EXIT_ON_ERROR) \
remove this as well

>-	for i in $(PP_LOCALIZED_FILES); do \
>-	  $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $(srcdir)/$$i > $(CONFIG_DIR)/$$i; \
>-	done
> 	$(PYTHON) $(topsrcdir)/config/Preprocessor.py -Fsubstitution $(DEFINES) $(ACDEFINES) \
> 	  $(srcdir)/nsis/defines.nsi.in | iconv -f UTF-8 -t UTF-16LE | \
> 	  cat $(MOZILLA_DIR)/toolkit/mozapps/installer/windows/nsis/utf16-le-bom.bin - > \
>@@ -138,9 +131,6 @@
> 	done
> 	$(INSTALL) $(addprefix $(DIST)/branding/,$(BRANDING_FILES)) $(CONFIG_DIR)
> 	$(EXIT_ON_ERROR) \
and this

>-	for i in $(PP_LOCALIZED_FILES); do \
>-	  $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) $(srcdir)/$$i > $(CONFIG_DIR)/$$i; \
>-	done
> ifeq ($(CONFIG_DIR),instgen)
> 	$(PERL) $(topsrcdir)/toolkit/mozapps/installer/windows/nsis/make-installremoves.pl \
> 	  ../removed-files > $(CONFIG_DIR)/removed-files.log

Comment 12

[Robert Strong (they/them - no direct email)]

I was able to create a localized de installer without any problems so besides comment #11 I think this is good to go.

Comment 13

[(not currently active) Ted Mielczarek]

Filed bug 514665 on fixing those stupid xpt filename differences.

Comment 14

[(not currently active) Ted Mielczarek]

Rob: I'm sorry, it's not completely clear to me which bits of code you're suggesting I remove. Can you quote a little more concisely?

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: rev 3, be more careful about platform differences etc

This should fix all of the warnings from Nick's log, several of which were actually correctness problems. (Oops.)

I'll update when Rob gets back to me.

Comment 16

[Robert Strong (they/them - no direct email)]

The $(EXIT_ON_ERROR) \ was for the code you removed which followed immediately afterward. I believe if you put this through the try with that there it would fail there as it did for me

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: rev 4, with makefile fixup

Ah, thanks! I totally missed that. I think this is good to go, but I'm pushing it to try just to double-check.

Comment 18

[Benjamin Smedberg]

Comment on attachment 398748 [details] [diff] [review]
rev 4, with makefile fixup

I'm not sure how I can systematically review the actual packages-static content, so I'm going to trust you on that part.

Comment 19

[(not currently active) Ted Mielczarek]

I've been over it a number of times, and Nick's wince build log was pretty helpful, but there's still a possibility for error, of course. I'll check the package step in the build logs, and I'll download packaged builds and sanity check them against the previous builds once this cycles.

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

Comment 20

[(not currently active) Ted Mielczarek]

Downloaded and diffed nightly builds vs. hourly builds with the patch, and pushed a followup to fix Linux bustage:
http://hg.mozilla.org/mozilla-central/rev/69f32850f635

I missed packaging the "firefox" script (oops), as well as chrome/icons/default. Otherwise the only difference is that with my patch, we now package the LICENSE file.

WinCE builds are showing the following warnings:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1252418777.1252421718.29075.gz&fulltext=1
Warning: package error or possible missing or unnecessary file: bin/res/charsetalias.properties (package-manifest, 305).
Warning: package error or possible missing or unnecessary file: bin/res/charsetData.properties (package-manifest, 306).
Warning: package error or possible missing or unnecessary file: bin/softokn3.chk (package-manifest, 326).
Warning: package error or possible missing or unnecessary file: bin/freebl3.chk (package-manifest, 328).
Warning: package error or possible missing or unnecessary file: bin/nssdbm3.dll (package-manifest, 331).
Warning: package error or possible missing or unnecessary file: bin/nssdbm3.chk (package-manifest, 332).
but diffing the nightly vs. an hourly shows no differences, which is good.

Linux builds also show a warning for the charsetalias.properties and charsetData.properties:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1252418777.1252422672.7456.gz&fulltext=1

Those two warnings are just a bad leftover from bug 508421, I'll fix that.

OS X builds show those two warnings plus:
Warning: package error or possible missing or unnecessary file: Minefield.app/Contents/MacOS/libsoftokn3.chk (package-manifest, 335).
Warning: package error or possible missing or unnecessary file: Minefield.app/Contents/MacOS/libfreebl3.chk (package-manifest, 337).
Warning: package error or possible missing or unnecessary file: Minefield.app/Contents/MacOS/libnssdbm3.chk (package-manifest, 341).

but the nightly vs. hourly differ only in that the hourly contains LICENSE.

Still waiting on a Win32 hourly to verify that.

Comment 21

[(not currently active) Ted Mielczarek]

Fixed taras' followup patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/dd26d29368ae

That will get rid of the {charsetalias,charsetData}.properties warnings.

Comment 22

[(not currently active) Ted Mielczarek]

Ah, the .chk stuff is just because we don't generate those in a cross-compile. The WinCE .chk warnings are...obvious, and the OS X ones are from the PPC half of the compile (the universal packaging goop regenerates new .chk files from the universal binaries).

The nssdbm stuff should be conditioned on NSS_DISABLE_DBM.

Comment 23

[(not currently active) Ted Mielczarek]

Followup fix to fix the .chk and nssdbm warnings:
http://hg.mozilla.org/mozilla-central/rev/fcf4388393f7

Noticed a typo in the Win32 build log:
Warning: package error or possible missing or unnecessary file: @BINPATH/AccessibleMarshal.dll (package-manifest, 53).
Pushed a fix for that:
http://hg.mozilla.org/mozilla-central/rev/cccf8041fc0b

Comment 24

[(not currently active) Ted Mielczarek]

(In reply to comment #23)
> Noticed a typo in the Win32 build log:
> Warning: package error or possible missing or unnecessary file:
> @BINPATH/AccessibleMarshal.dll (package-manifest, 53).
> Pushed a fix for that:
> http://hg.mozilla.org/mozilla-central/rev/cccf8041fc0b

Diffing the Win32 nightly and hourly builds shows that AccessibleMarshal.dll was the only difference, so they should be the same now with that fix.

Comment 25

[(not currently active) Ted Mielczarek]

Looking at recent build logs shows zero packager warnings on any platform. In addition, diffing the most recent hourlies vs. the nightlies from before landing my patch shows no unexpected differences.

Comment 26

[Benjamin Smedberg]

Quick, let's make packager warnings fatal...

Comment 27

[(not currently active) Ted Mielczarek]

Rolled up the original patch + followup fixes, and pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/60dda235c428

Comment 28

[(not currently active) Ted Mielczarek]

I had to port one line of a patch from bug 402892 to make this work right on branch:
configure.in
+            AC_DEFINE(MOZ_ENABLE_GNOMEVFS)

(Could have worked around that in the makefile, but that was the simplest solution.)