Status

()

Firefox
Installer
--
enhancement
7 years ago
3 days ago

People

(Reporter: khuey, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(21 attachments, 3 obsolete attachments)

3.34 KB, patch
Mitch
: review+
Details | Diff | Splinter Review
12.40 KB, patch
khuey
: review+
Details | Diff | Splinter Review
549 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
985 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
12.60 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
1.67 KB, patch
khuey
: review+
Details | Diff | Splinter Review
5.78 KB, patch
khuey
: review+
Details | Diff | Splinter Review
864 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
4.24 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.25 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
2.00 KB, patch
Mitch
: review+
Details | Diff | Splinter Review
7.30 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.70 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.35 KB, patch
ted
: review+
Details | Diff | Splinter Review
4.19 KB, patch
Pike
: review+
rstrong
: feedback+
Details | Diff | Splinter Review
1.01 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.40 KB, patch
Mitch
: review+
Details | Diff | Splinter Review
6.84 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.10 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
1.29 KB, patch
rstrong
: review+
nthomas
: feedback+
Details | Diff | Splinter Review
742 bytes, patch
ted
: review+
Details | Diff | Splinter Review
When I have a go at this again soon I'm going to use this bug rather than the ridiculously long/noisy Bug 231062.
Created attachment 487077 [details] [diff] [review]
Configure bits for MSI installer
Attachment #487077 - Flags: review?(mitchell.field)
Created attachment 487079 [details] [diff] [review]
Part 2: Provide branding info to WiX

This patch was r+'d by rs in Bug 231062.
Attachment #487079 - Flags: review+
Created attachment 487080 [details] [diff] [review]
Part 3: Generate Product Code at build time

This was also r+'d by rs in Bug 231062.
Attachment #487080 - Flags: review+
Created attachment 487081 [details] [diff] [review]
Part 4: Reflect build system variables into WiX

This patch was r+'d by jimm in Bug 231062.
Attachment #487081 - Flags: review+
Created attachment 487082 [details] [diff] [review]
Part 5: Implement a basic en-US Firefox MSI

Parts of this have been previously reviewed, but it's changed enough that I think we should start from scratch on this one.
Attachment #487082 - Flags: review?
Attachment #487082 - Flags: review? → review?(robert.bugzilla)
Created attachment 487083 [details] [diff] [review]
Part 6: Write the product code to disk for later use

This was r+'d by jimm in Bug 231062.
Attachment #487083 - Flags: review+
Created attachment 487084 [details] [diff] [review]
Part 7: Look pretty in Add/Remove Programs

Ditto
Attachment #487084 - Flags: review+
Attachment #487082 - Flags: feedback?(mitchell.field)
Created attachment 487087 [details] [diff] [review]
Part 8: Tell Windows we know what we're doing with UAC.
Attachment #487087 - Flags: review+
Created attachment 487096 [details] [diff] [review]
Part 9: Export brand.dtd to dist/branding

This was reviewed by ted and Pike in Bug 231062.
Attachment #487096 - Flags: review+
Created attachment 487097 [details] [diff] [review]
Part 10: Use BrandFullNameInternal to get the right name in official builds
Attachment #487097 - Flags: review?
Created attachment 487098 [details] [diff] [review]
Part 11: Calculate the firefox.exe Windows version from the app version, not the Gecko version
Attachment #487098 - Flags: review?(mitchell.field)
Attachment #487077 - Flags: review?(mitchell.field) → review+
Attachment #487098 - Flags: review?(mitchell.field) → review+
Attachment #487097 - Flags: review? → review?(robert.bugzilla)
Created attachment 487149 [details] [diff] [review]
Part 12: Add localized shorcuts

This patch has r=Mitch,ted,Pike in Bug 231062.
Attachment #487149 - Flags: review+
Created attachment 487150 [details] [diff] [review]
Part 13: Add a custom action DLL to replace AppAssocReg

This patch has r=jimm in Bug 231062.
Attachment #487150 - Flags: review+
Created attachment 487151 [details] [diff] [review]
Part 14: Integrate custom action DLL into the installer

Same as Part 13
Attachment #487151 - Flags: review+
Created attachment 487152 [details] [diff] [review]
Part 15: Registry/shell integration
Attachment #487152 - Flags: review?
Attachment #487152 - Flags: review? → review?(robert.bugzilla)
Created attachment 487153 [details] [diff] [review]
Part 16: Upload completed MSI
Attachment #487153 - Flags: review?(ted.mielczarek)
Created attachment 487161 [details] [diff] [review]
Part 17: Move all existing strings into the DTD for l10n
Attachment #487161 - Flags: review?(community)
Attachment #487161 - Flags: feedback?(robert.bugzilla)
Attachment #487161 - Flags: review?(community) → review?(l10n)
Created attachment 487164 [details] [diff] [review]
Part 18: Use a shortened product version to get the ProductCodes just right.

This has r=Mitch in Bug 231062.
Attachment #487164 - Flags: review+
Created attachment 487165 [details] [diff] [review]
Part 19: Teach the updater about MSI installations

Adds an 'msiexec' command to the updater that lets us pass an MSI file out to Windows Installer.
Attachment #487165 - Flags: review?(robert.bugzilla)
Created attachment 487174 [details] [diff] [review]
Part 20: Add patch tools to configure
Attachment #487174 - Flags: review?(mitchell.field)
I have a few concerns with making the updater binary part of the msi process. A few of the concerns off the top of my head are.

1. the updater requires that Firefox is not running and MSI's don't have that requirement.
2. It adds complexity to the updater which is unnecessary.
3. releng packaging will require creating the msi then creating a mar containing the msi. Though minor it is an additional step that should be unnecessary.
4. it locks us in to only applying the msi when the app is restarted which should not be necessary with an msi.

We already use the update service to distribute apk files for Android and I would think it would be possible to do the same for msi / msp files.
OK, I'll take a look at what we do for Android.
Attachment #487174 - Flags: review?(mitchell.field) → review+

Updated

7 years ago
Attachment #487161 - Flags: review?(l10n) → review+
Comment on attachment 487153 [details] [diff] [review]
Part 16: Upload completed MSI

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -669,16 +669,17 @@ QUOTED_WILDCARD = $(if $(wildcard $(subs
> CHECKSUM_ALGORITHM = 'sha512'
> 
> # This variable defines where the checksum file will be located
> CHECKSUM_FILE = "$(DIST)/$(PKG_PATH)/$(PKG_BASENAME).checksums"
> 
> UPLOAD_FILES= \
>   $(call QUOTED_WILDCARD,$(DIST)/$(PACKAGE)) \
>   $(call QUOTED_WILDCARD,$(INSTALLER_PACKAGE)) \
>+  $(call QUOTED_WILDCARD,$(DIST)/$(PKG_BASENAME).msi) \

If you use this in more than one place you might want to define MSI_PACKAGE or something like that.
Attachment #487153 - Flags: review?(ted.mielczarek) → review+
Attachment #487082 - Flags: review?(robert.bugzilla) → review+
Attachment #487097 - Flags: review?(robert.bugzilla) → review+
Attachment #487165 - Flags: review?(robert.bugzilla) → review-
Comment on attachment 487152 [details] [diff] [review]
Part 15: Registry/shell integration

Note: we will likely be removing the program directory from the start menu and only having a single shortcut in the root of the start menu. It will probably be easier if the msi did this from day one so cleanup doesn't have to happen when the change is made. There are a couple of bugs that need to be fixed for this to happen though.
Attachment #487152 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 487152 [details] [diff] [review]
Part 15: Registry/shell integration

btw: you named this patch "Part 15: Registry/shell integration" when all it really does is add the shortcuts. Is there already a patch for "Registry/shell integration"?
Attachment #487161 - Flags: feedback?(robert.bugzilla) → feedback+
(In reply to comment #25)
> Comment on attachment 487152 [details] [diff] [review]
> Part 15: Registry/shell integration
> 
> btw: you named this patch "Part 15: Registry/shell integration" when all it
> really does is add the shortcuts. Is there already a patch for "Registry/shell
> integration"?

Bah, I uploaded Part 12 as Part 12 and Part 15.
Comment on attachment 487152 [details] [diff] [review]
Part 15: Registry/shell integration

Minusing patch 15 due to comment #26 and comment #24 applies to patch 12 attachment #487149 [details] [diff] [review]
Attachment #487152 - Flags: review+ → review-
Regarding the Registry/shell integration we are planning on removing dde support in bug 491947. The one hassle is Win2K. I think we shouldn't support MSI's with Win2K and just disable it for MSI's.
Attachment #487152 - Attachment is obsolete: true
Attachment #487165 - Attachment is obsolete: true
Created attachment 521666 [details] [diff] [review]
Part 12: Add localized shortcut

Updated to the Firefox 4 behavior of one toplevel shortcut.
Attachment #487149 - Attachment is obsolete: true
Attachment #521666 - Flags: review+
Created attachment 521668 [details] [diff] [review]
Part 21: Omit CRT directives from the custom action DLL.

The custom action DLL cannot rely on an appropriate version of the CRT already being installed.
Attachment #521668 - Flags: review?
Attachment #521668 - Flags: review? → review?(robert.bugzilla)
Created attachment 521675 [details] [diff] [review]
Part 22: Bind files into the WiX output file

Asking for nthomas for feedback on the approach here, not the actual code.

There are two ways to do "partial" MSI updates in WiX.  You can generate the updates from a pair of the .msi packages themselves, or from the .wixpdb files that the toolset outputs.  If we use the .msi files, we have to do administrative installations to unpack them to generate the partials.  If we use the .wixpdbs we can avoid that, at the cost of having to have another type of file around.

It's basically a tradeoff between code complexity if we use .msis and some disk space on ftp if we use the .wixpdbs.  I'd like to use the wixpdbs, but I'd like nthomas's feedback on this.

If we use the wixpdbs, we need to bind the files into them (see http://windows-installer-xml-wix-toolset.687559.n2.nabble.com/error-PYRO0103-The-system-cannot-find-the-file-UI-Icons-appicon-ico-if-appicon-ico-is-in-my-latest-wb-td4600799.html for the type of error we hit otherwise)
Attachment #521675 - Flags: review?(robert.bugzilla)
Attachment #521675 - Flags: feedback?(nrthomas)
Created attachment 521678 [details] [diff] [review]
Part 23: Upload the wixpdb

If we go with the wixpdb method, we'll need to upload the wixpdbs.
Attachment #521678 - Flags: review?(ted.mielczarek)
Comment on attachment 521678 [details] [diff] [review]
Part 23: Upload the wixpdb

Fine with me.
Attachment #521678 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 521675 [details] [diff] [review]
Part 22: Bind files into the WiX output file

So we would need to retain the .wixpdb's between releases but not actually ship them to end users ? If so, we can keep them in firefox/nightly/$version-candidates/ on the ftp server, and avoid the mirrors having to carry that weight in firefox/releases/. That would be fine so long as the .wixpdb's are reasonably sized, so how large are they, and the msi's ?

Something we'll need to consider is signing the windows binaries for releases. The current process is 
 * build en-US, package
 * repack locales, package
 * sign files inside the installers and complete mars, and the installers themselves
 * generate partial mars
So we'd need to adjust the signing scripts to unpack the files in a msi, sign them or replace with signed files, and put them back together. Possibly sign the msi, and generate the partial msi updates afterwards. Bug 509158 may change that process, making signed bits available on the build machine - probably life much simpler for you - but there's isn't a timeline there yet.
Attachment #521675 - Flags: feedback?(nrthomas) → feedback+
Comment on attachment 521675 [details] [diff] [review]
Part 22: Bind files into the WiX output file

Oops, setting feedback? again until khuey answers.
Attachment #521675 - Flags: feedback+ → feedback?(nrthomas)
The MSI installer itself is ~25MB uncompressed (that'll drop significantly once we turn on compression) and the wixpdb is about 12-13 MB.  Note that we only have to keep the wixpdbs around for any binary we want to create a partial update to/from, we can always create full updates (the MSI itself is the full update)

(In reply to comment #34)
> Something we'll need to consider is signing the windows binaries for releases.
> The current process is 
>  * build en-US, package
>  * repack locales, package
>  * sign files inside the installers and complete mars, and the installers
> themselves
>  * generate partial mars
> So we'd need to adjust the signing scripts to unpack the files in a msi, sign
> them or replace with signed files, and put them back together. Possibly sign
> the msi, and generate the partial msi updates afterwards. Bug 509158 may change
> that process, making signed bits available on the build machine - probably life
> much simpler for you - but there's isn't a timeline there yet.

Yeah catlee and I have been talking about this.  He's rewriting a good chunk of our packaging code as part of that effort.
Attachment #521668 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 521675 [details] [diff] [review]
Part 22: Bind files into the WiX output file

I think wixpdb would be the way to go
Attachment #521675 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 521675 [details] [diff] [review]
Part 22: Bind files into the WiX output file

Another GB per set of release builds isn't going to break the bank.
Attachment #521675 - Flags: feedback?(nrthomas) → feedback+
Assignee: khuey → nobody
Status: ASSIGNED → NEW
Attachment #487082 - Flags: feedback?(mitchell.field)
You need to log in before you can comment on or make changes to this bug.