Closed Bug 313956 Opened 14 years ago Closed 9 years ago

expand installer .exe contents to make complete mar

Categories

(Release Engineering :: General, defect, P4)

x86
Windows XP
defect

Tracking

(firefox5 fixed)

RESOLVED FIXED
Tracking Status
firefox5 --- fixed

People

(Reporter: chase, Assigned: rail)

References

Details

(Whiteboard: [updates])

Attachments

(3 files, 9 obsolete files)

During the Windows build process we first create a .exe, then a .zip, then a .mar.  The .exe is created out of XPI packages and setup files, and then wrapped in a 7z invocation layer.  The .zip and .mar are made from a standard dist/firefox directory.  Occasionally small differences between the dist/firefox directory and the unpackaged installer may creep in, leading to difficulty in field deployments of the app when it is time to create a partial patch from that build's .mar file.

We need to be proactive in verifying that a .exe's installation directory will look *exactly* like its accompanying .mar file.  While we can easily unpack and test the .mar, no utility exists to unpack the .exe.  We should create such a utility.
Blocks: 313827
Here's a rough way to do this, using 7zip to extract the constituents of the installer, then unzip on the xpi's. As usual, the devil is in the details:
* each xpi has an install.js, so I used unzip -n to not ask questions, and not overwrite files. Prompting is safer but not very suitable for automatic checking
* UninstallFirefox.zip is ignored, see below for diff between unpacked installer and executed installer. The other setup files are also ignored.

Sample output (for 20051026 nightly):
 Difference between zip/ and installer/:
 Only in zip: .autoreg
 Only in installer: README.txt
 Only in zip: readme.txt
 Only in zip: removed-files

The readme's are actually the same content. Also interesting is a diff between the unpacked installed (installer/) and the result of executing one (installed/, before running Firefox for the first time):
 Difference between installer/ and installed/:
 Only in installed: .autoreg
 Only in installer: README.txt
 Only in installed/defaults: shortcuts
 Only in installed: install_status.log
 Only in installed: install_wizard.log
 Only in installed/plugins: nsIQTScriptablePlugin.xpt
 Only in installed: readme.txt
 Only in installed: uninstall

nsIQTScriptablePlugin.xpt has a 20050917 modification date, so is presumably Quicktime running in the background watching for Firefox installs.

For completeness:
 Difference between zip/ and installed/:
 Only in installed/defaults: shortcuts
 Only in installed: install_status.log
 Only in installed: install_wizard.log
 Only in installed/plugins: nsIQTScriptablePlugin.xpt
 Only in zip: removed-files
 Only in installed: uninstall
That's excellent. Chase is probably going to want to pass the installer filename on the shell command line, but that's easy to hack in. We not only want to compare the existence of files, but actually check to make sure we're shipping the exact same files (see bug 313827 where one of the XPT files was actually different between installer and MAR).
Assignee: nobody → chase
Component: Build Config → Build & Release
Product: Firefox → mozilla.org
QA Contact: build.config → chase
Version: unspecified → other
Mass reassign of open bugs for chase@mozilla.org to build@mozilla-org.bugs.
Assignee: chase → build
Over to me during triage
Assignee: build → nrthomas
Priority: -- → P3
[ Meanwhile, we moved the installer over to NSIS ]

Benjamin, do you think this is still an issue ? It looks to me that the exe and zip/mar uses the same manifest, even if they end up in different directories (see below).

Assorted checks I did:

For Firefox 2.0.0.4, the differences are:
  Only in complete_update/: .autoreg
  Only in complete_update/: remove-files
  Only in complete_udpate/: update.manifest
  Only in installer/combined_files/defaults/pref: channel-prefs.js
  Only in installer/: removed-files.log
  Only in installer/: setup.exe
(where I've swept all of nonlocalized, localized, and optional into combined_files/). Except for .autoreg I'd expect all those differences. 

Update checking/QA as part of the release process is also greatly beefed up, ie  we explicitly check that 
 installed 2.0.0.x + applied partial/complete update == installed 2.0.0.x+1

As for what the tinderbox does (for 2.0.0.x again) the sequence is

* Make the installer:
  * "make -C mozilla/browser/installer installer" into mozilla/installer-stage/
  * copy the exe from mozilla/dist/install/sea/ to mozilla/dist/install/2007-06-20-03-mozilla1.8/
* Make another installer:
  * "make -C mozilla/browser/installer installer" into mozilla/installer-stage/ again, but with MOZ_PACKAGE_MSI and MOZ_INSTALLER_USE_7ZIP unset
  * fail to copy the non-existent xpi's
  * we can probably unset $push_raw_xpis in the tinder-config.pl, and get rid of this leftover
* Make zip:
  * "make -C mozilla/browser/installer" into mozilla/dist/firefox
  * copy dist/*.zip to mozilla/dist/install/2007-06-20-03-mozilla1.8/
* Make complete mar:
  * build the mar using dist/firefox
  * copy mar to mozilla/dist/install/2007-06-20-03-mozilla1.8/
* upload everything in mozilla/dist/install/2007-06-20-03-mozilla1.8/ (via another dir)

So we don't run the risk of running xpt_link twice (as in bug 313827).
Well, hrm. We've talked about generating the MAR directly by unpacking the .exe instead of doing it from the "zip" build process, but most of the problems here are fixed. We only doing staging xpt_link once, and we create both the installer and the zip/mar from those files, AFAIK.

FWIW "a tool to unpack NSIS installer.exes" is the 7zip command-line.
QA Contact: chase → build
Handing back to the Build bugpool for now. We could easily make this sort of change in a post-tinderbox setup - it'd be a bit harder to wrangle tinderbox into it.
Assignee: nrthomas → nobody
Summary: expand installer .exe contents into installation directory → expand installer .exe contents to make complete mar
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
No longer blocks: 470811
Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering
Whiteboard: [updates]
Priority: P3 → P5
Assignee: nobody → rail
Attached patch Unpack EXE (obsolete) — Splinter Review
Looks like a potential bug for Core/Build Config.

Attached is a not tested patch.
Attachment #468640 - Flags: feedback?(nrthomas)
Comment on attachment 468640 [details] [diff] [review]
Unpack EXE

>diff --git a/tools/update-packaging/Makefile.in b/tools/update-packaging/Makefile.in
> # input location for the build, usually $(DIST)
> # set this to $(DIST)/l10n-stage per override for L10n builds
> PACKAGE_BASE_DIR	= $(DIST)
...
>+ifeq ($(OS_TARGET), WINNT)
>+PACKAGE_DIR     = $(DIST)/unpacked_installer
>+endif

Might have to use PACKAGE_BASE_DIR to satisfy l10n, would need testing.
 
> complete-patch::
>+ifeq ($(OS_TARGET), WINNT)
>+	rm -rf "$(PACKAGE_DIR)" \
>+	&& mkdir -p "$(PACKAGE_DIR)" \
>+	&& $(CYGWIN_WRAPPER) 7z x "$(INSTALLER_PACKAGE)" core -o"$(PACKAGE_DIR)" \
>+	&& mv "$(PACKAGE_DIR)"/core/* "$(PACKAGE_DIR)"/ \
>+	rmdir "$(PACKAGE_DIR)"/core
>+endif

Why do this as a long command ? Mozilla style is to run these sequentially when they're not being assigned to a VAR. Worth trying to use INNER_UNMAKE_PACKAGE from packager.mk ?

We own this code (http://www.mozilla.org/about/owners.html#build-and-release-tools) so we can do review as normal for RelEng changes. Only thing I can think of to add would be a timestamp check to verify the exe is fresh, ie newer than installer-stage and dist/bin and whatever. I always find our packaging a maze of twisty packages, all alike.
Attachment #468640 - Flags: feedback?(nrthomas) → feedback+
Attached patch Unpack EXE (obsolete) — Splinter Review
I tested the attached patch for nightlies.

* toolkit/mozapps/installer/packager.mk (I need it for $(INSTALLER_PACKAGE)) requires $(MOZILLA_DIR) variable which is defined in config/rules.mk
* toolkit/mozapps/installer/package-name.mk is included by toolkit/mozapps/installer/packager.mk, so I removed it
* Instead of using INNER_UNMAKE_PACKAGE which depends on MOZ_PKG_FORMAT, I'd prefer to use a command which explicitly uses exe file (INSTALLER_PACKAGE)

I unpacked exe and mar files and the diff lists only 2 files:
* core/defaults/pref/channel-prefs.js
* update.manifest
Attachment #468640 - Attachment is obsolete: true
Attachment #477188 - Flags: review?(nrthomas)
Comment on attachment 477188 [details] [diff] [review]
Unpack EXE

Ted, could you also take a look at this patch?
Attachment #477188 - Flags: review?(ted.mielczarek)
Comment on attachment 477188 [details] [diff] [review]
Unpack EXE

>diff --git a/tools/update-packaging/Makefile.in b/tools/update-packaging/Makefile.in
>--- a/tools/update-packaging/Makefile.in
>+++ b/tools/update-packaging/Makefile.in
>@@ -38,18 +38,19 @@
> # ***** END LICENSE BLOCK *****
> 
> DEPTH		= ../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
>+include $(topsrcdir)/config/rules.mk

You're double-including rules.mk here. (Why'd you add this up here anyway?)

> complete-patch::
>+ifeq ($(OS_TARGET), WINNT)
>+	rm -rf "$(PACKAGE_DIR)"
>+	$(CYGWIN_WRAPPER) 7z x $(INSTALLER_PACKAGE) -o"$(PACKAGE_DIR)"
>+endif

You should probably sanity check that $(INSTALLER_PACKAGE) exists first, otherwise this will rm the package directory and then fail to extract the nonexistent installer.

r=me with that fixed.
Attachment #477188 - Flags: review?(ted.mielczarek) → review+
Attached patch Unpack EXE (obsolete) — Splinter Review
(In reply to comment #13)
> You're double-including rules.mk here. (Why'd you add this up here anyway?)

Missed the second inclusion. I moved the inclusion of packager.mk to the end.
 
> > complete-patch::
> >+ifeq ($(OS_TARGET), WINNT)
> >+	rm -rf "$(PACKAGE_DIR)"
> >+	$(CYGWIN_WRAPPER) 7z x $(INSTALLER_PACKAGE) -o"$(PACKAGE_DIR)"
> >+endif
> 
> You should probably sanity check that $(INSTALLER_PACKAGE) exists first,
> otherwise this will rm the package directory and then fail to extract the
> nonexistent installer.

I added test -f "$(INSTALLER_PACKAGE)".

Thanks to bhearsum, who pointed me to the fact that we need to reproduce "package" file layout in MARs, not "installer", so I added mv/rmdir commands to arrange the files.

The patch creates a MAR file with expected content, but I'd double check and run some additional tests before we land this to m-c.
Attachment #477188 - Attachment is obsolete: true
Attachment #482527 - Flags: review?(ted.mielczarek)
Attachment #477188 - Flags: review?(nrthomas)
Priority: P5 → P2
Comment on attachment 482527 [details] [diff] [review]
Unpack EXE

>diff --git a/tools/update-packaging/Makefile.in b/tools/update-packaging/Makefile.in
>--- a/tools/update-packaging/Makefile.in
>+++ b/tools/update-packaging/Makefile.in
>@@ -39,18 +39,16 @@
> 
> complete-patch::
>+ifeq ($(OS_TARGET), WINNT)
>+	test -f "$(INSTALLER_PACKAGE)"
>+	rm -rf "$(PACKAGE_DIR)"
>+	$(CYGWIN_WRAPPER) 7z x "$(INSTALLER_PACKAGE)" -o"$(PACKAGE_DIR)" core
>+	mv "$(INSTALLER_PACKAGE)/core/"* "$(INSTALLER_PACKAGE)/"
>+	rmdir "$(INSTALLER_PACKAGE)/core"

I'd just use $(RM) -rf instead of rmdir here. Is there any way to convince that 7z commandline to extract the contents of "core" into the dir you want without extracting the "core" directory itself?

Also, are we guaranteed that we're not going to change the layout of files inside the installer? Hardcoding things like this in separate directories always makes me nervous. (although the likelihood of failure is pretty small here.)
Attachment #482527 - Flags: review?(ted.mielczarek) → review+
Blocks: 404340
Priority: P2 → P4
Attached patch Unpack EXE (obsolete) — Splinter Review
This is my final patch which doesn't use any hardcoded macros. It changes INNER_UNMAKE_PACKAGE macro and unpacks only content of the "core" directory. In this case it doesn't warn when unpacks the package second time (it used to be asking if you want to override setup.exe and other top level files).

There is a problem when I ran it for l10n repacks with MOZ_PKG_PRETTYNAMES=1:

sh -c make installers-be LOCALE_MERGEDIR=$PWD/merged
 in dir e:\builds\moz2_slave\win32_repack\build/mozilla-central/browser/locales (timeout 1200 secs)
 watching logfiles {}
 argv: ['sh', '-c', 'make installers-be LOCALE_MERGEDIR=$PWD/merged']
 environment:
....
  MOZ_MAKE_COMPLETE_MAR=1
  MOZ_PKG_PRETTYNAMES=1
  MOZ_PKG_VERSION=4.0b7
.....
cd ../../dist/l10n-stage && \
	  unzip e:\builds\moz2_slave\win32_repack\build\mozilla-central/firefox.zip && (cd firefox && d:/mozilla-build/python25/python2.5.exe /e/builds/moz2_slave/win32_repack/build/mozilla-central/browser/locales/../../config/optimizejars.py --deoptimize /e/builds/moz2_slave/win32_repack/build/mozilla-central/browser/locales/../../dist/jarlog/ ./ ./ && unzip -o omni.jar && rm -f components/binary.manifest && sed -e 's/^#binary-component/binary-component/' components/components.manifest > components.manifest && mv components.manifest components)
unzip:  cannot find either e:buildsmoz2_slavewin32_repackbuildmozilla-central/firefox.zip or e:buildsmoz2_slavewin32_repackbuildmozilla-central/firefox.zip.zip.
make: *** [/e/builds/moz2_slave/win32_repack/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox] Error 9

The full log is here: http://pastebin.mozilla.org/858524

Any idea how to fix this bug? For other 3 cases (usual build and l10n repack, build with MOZ_PKG_PRETTYNAMES set) the build worked fine... :(
Help is very appreciated.
Attachment #482527 - Attachment is obsolete: true
Attachment #491795 - Flags: feedback?(ted.mielczarek)
Attachment #491795 - Flags: feedback?(nrthomas)
Comment on attachment 491795 [details] [diff] [review]
Unpack EXE

Since you're breaking in l10n repackaging, Pike would be a lot more useful here than I would be.
Attachment #491795 - Flags: feedback?(l10n)
Comment on attachment 491795 [details] [diff] [review]
Unpack EXE

pretty names are in hard work in bug 525438 to work at all, still need to test all code paths on too many platforms to wrap that one up.

Until then, no idea how pretty names will affect this patch, as that's using new code concepts by Kyle with space escaping.
Comment on attachment 491795 [details] [diff] [review]
Unpack EXE

Like all our packaging code I find this pretty opaque, but it seems fairly sensible to me.

The error might just be a cmd vs msys slash-direction issues. You have
 ZIP_IN=e:\builds\moz2_slave\win32_repack\build\mozilla-central/firefox.zip
in the environment with that last slash the other direction, where we have
 'ZIP_IN': '/e/builds/moz2_slave/release-mozilla-1.9.2-win32_repack_1/build/firefox.zip'
for the call to the script factory in the 3.6.13build1 logs.
Attachment #491795 - Flags: feedback?(nrthomas) → feedback+
Oh. Looks like _ABS_DIST macro [1] redefinition used for l10n repacks [2] introduces this.

Axel, do you know the reason why l10n uses its own _ABS_DIST?

1. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/packager.mk#94

2. http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#66
Hysterical raisons, l10n.mk was there first.

If there difference matters, no idea.
Attached patch Unpack EXE (obsolete) — Splinter Review
I'm testing the attached patch right now using different scenarios (nightly builds/repacks, release builds/repacks)

I've removed _ABS_DIST definition from toolkit/locales/l10n.mk, it should use _ABS_DIST defined in packager.mk.
Attachment #491795 - Attachment is obsolete: true
Attachment #491795 - Flags: feedback?(ted.mielczarek)
Attachment #491795 - Flags: feedback?(l10n)
Attached patch Unpack EXE (obsolete) — Splinter Review
Thanks to Ben, I finally managed to found why l10n repacks were failing for release builds, when MOZ_PKG_PRETTYNAMES is set. During l10n repacks update-packaging code is being called twice (see bug 617927), once for repackage-win32-installer and second time for repackage-zip. One of them has pretty package name, and another hasn't.

The attached patch should also fix bug 617927.

Some comments:

> +ifdef MOZ_MAKE_COMPLETE_MAR
> +MAKE_COMPLETE_MAR=1
> +ifeq ($(OS_ARCH), WINNT)
> +ifneq ($(MOZ_PKG_FORMAT), SFX7Z)
> +MAKE_COMPLETE_MAR=0
> +endif
> +endif
> +endif

This added to fix bug 617927 and prevent running update-packaging code twice.

> +ifdef AB_CD
> +ifdef MOZ_PKG_PRETTYNAMES
> +UNPACKAGE      = "$(PACKAGE_BASE_DIR)/$(PACKAGE)"
> +endif
> +endif

This is a special case for release l10n repacks. When update-packaging code is being called, the installer package isn't renamed.
Attachment #493782 - Attachment is obsolete: true
Attachment #497194 - Flags: feedback?(nrthomas)
Attachment #497194 - Flags: feedback?(community)
Attachment #497194 - Flags: feedback?(bhearsum)
Attachment #497194 - Flags: feedback?(community) → feedback?(l10n)
Comment on attachment 497194 [details] [diff] [review]
Unpack EXE

Sorry, but I'm not going to be able to provide meaningful feedback without spending a lot of time figuring out what all the code paths and variables are in this code.
Attachment #497194 - Flags: feedback?(nrthomas)
Comment on attachment 497194 [details] [diff] [review]
Unpack EXE

Do you have links to any logs and/or builds generated with this patch?
Comment on attachment 497194 [details] [diff] [review]
Unpack EXE

Can

+ifeq ($(MAKE_COMPLETE_MAR), 1)

be an ifdef, too? That's a review comment that ted put on me in bug 525438. I'm tempted to want this land after bug 525438, though I don't see a real merge conflict in the patches.

Probably wants a real review by ted or kyle.
Attachment #497194 - Flags: feedback?(l10n) → feedback+
I think this bug should be moved to toolkit build config, too.
Comment on attachment 497194 [details] [diff] [review]
Unpack EXE

I can't really provide a real review, but this looks sensible.
Attachment #497194 - Flags: feedback?(bhearsum) → feedback+
Attached patch Unpack EXE (obsolete) — Splinter Review
Used ifdef instead of ifeq.
Attachment #497194 - Attachment is obsolete: true
Attachment #504393 - Flags: review?(ted.mielczarek)
Attachment #504393 - Flags: review?(ted.mielczarek) → review+
Attachment #504393 - Flags: approval2.0?
Comment on attachment 504393 [details] [diff] [review]
Unpack EXE

This is nominated for approval2.0 but no risk/reward assessment has been provided. I'm going to clear the nom for now, and you can renom if there is a compelling reason we need to take this now instead of the next release cycle. Please provide a full risk/reward assessment.
Attachment #504393 - Flags: approval2.0?
Ted, do you think that it's worth to try landing this patch via build-system branch?
You are always welcome to land build system changes on the build-system branch, provided it's green.
Attached patch Unpack EXE (obsolete) — Splinter Review
Refreshed patch to match the current context. Keeping ted's r+.
Attachment #504393 - Attachment is obsolete: true
Attachment #522735 - Flags: review+
Comment on attachment 522735 [details] [diff] [review]
Unpack EXE

changeset:   64168:f4da8c0c48d9
Attachment #522735 - Flags: checked-in+
Blocks: 646309
Somehow the patch has broken nightly l10n repacks.

In the current state l10n repacked installer is moved from dist/l10n-stage to dist/install/sea, but the update packaging target is run just before this mv happens. So the installer file is "$(PACKAGE_BASE_DIR)/$(PACKAGE)" for both l10n repack case.

The landed patch has a special case for l10n repacks with pretty names set on. Now we need this special case for regular l10n repacks too.

The attached patch fixes nightly l10n repack case.

I tested the patch for the following cases:

1. Release build (pretty name on)
2. Release repacks (pretty name on)
3. Nightly build. It also runs additional target for update packaging with pretty names set on.
4. Nightly l10n repacks (the most important part which was broken). 

All of the tests have been passed. See also some snippets from the build logs in the next attachment.
Attachment #523327 - Flags: review?(ted.mielczarek)
Attached patch build snippetsSplinter Review
Attachment #523327 - Flags: review?(ted.mielczarek) → review+
Attachment #522735 - Flags: checked-in+ → checked-in-
Attached patch Unpack EXESplinter Review
Combined patch of attachment 522735 [details] [diff] [review] and attachment 523327 [details] [diff] [review]. Keeping r+
Attachment #522735 - Attachment is obsolete: true
Attachment #523327 - Attachment is obsolete: true
Attachment #524208 - Flags: review+
Comment on attachment 524208 [details] [diff] [review]
Unpack EXE

I re-landed this today: http://hg.mozilla.org/mozilla-central/rev/991132479b7a

Once we have a build I'll push through some repacks.
Attachment #524208 - Flags: checked-in+
Turns out that I can't force repacks to test this, because they update to the revision they find in their build, which is yesterday's nightly. We'll have to wait for the next nightlies to know if this worked.

Builds/tests all passed, though.
Yay! Finally. :)

Tested using today's build and repacks. The following bash script downloads en-US build and localized repacks + complete MARs, unpacks exe and mar, compares exe and mar directories, adds changes files to expand.diff, then lists changed files regardless of locale:
======================================================
MAR="python /home/rail/work/mozilla/build/tools/buildfarm/utils/mar.py"

wget -c -q http://hg.mozilla.org/mozilla-central/raw-file/tip/browser/locales/shipped-locales
wget -c -q http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-6.0a1.en-US.win32.installer.exe
wget -c -q http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/firefox-6.0a1.en-US.win32.complete.mar

for l in $(grep -v en-US shipped-locales); do
    wget -c -q http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/firefox-6.0a1.$l.win32.installer.exe
    wget -c -q http://stage.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central-l10n/firefox-6.0a1.$l.win32.complete.mar
done

for l in $(cat shipped-locales); do
    # ignore failed repacks (all of them died due to hg timeout)
    test -f firefox-6.0a1.$l.win32.installer.exe && test -f firefox-6.0a1.$l.win32.complete.mar || continue 
    7z x firefox-6.0a1.$l.win32.installer.exe core >/dev/null
    mv core $l.exe
    mkdir $l.mar
    $MAR -x --bzip2 -C $l.mar firefox-6.0a1.$l.win32.complete.mar
    diff -Nrua  $l.exe $l.mar | lsdiff >> expand.diff
    rm -rf $l.exe $l.mar
done
awk -F/ '{print $2}' expand.diff | sort -u
======================================================

Output:
=============
update.manifest
updatev2.manifest
=============
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Was fixed in time for the merge to aurora for Firefox 5. This is also totally a Core::Build Config bug.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.