As it stands now if a change has to be made to the NSIS installer it almost always requires touching each app. To make this easier to maintain and get reviews I am going to move many of the common sections shared by installers into common.nsh which will allow apps to either continue using the common code or they can choose to just not call the common code as is the case for common.nsh today. After that is completed I am going to implement something along the lines of a NSIS define that app's can use to use the macros in common.nsh which would perform all of the required insertmacro statements. Something like this is desirable since macros that are inserted and not used increase the installer size.

I need to test but this looks hopeful

Comment on attachment 277617 [details] [diff] [review] patch part 1 - rev 1 (checked in) r=sspitzer

Part 1 checked in to trunk Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v <-- common.nsh new revision: 1.22; previous revision: 1.21 done Checking in mozilla/toolkit/mozapps/installer/windows/nsis/overrides.nsh; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/overrides.nsh,v <-- overrides.nsh new revision: 1.2; previous revision: 1.1 done Checking in mozilla/browser/installer/windows/nsis/installer.nsi; /cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v <-- installer.nsi new revision: 1.32; previous revision: 1.31 done Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi; /cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v <-- uninstaller.nsi new revision: 1.9; previous revision: 1.8 done I am going to do some app installer specific cleanup in this bug so I am leaving it open

Broke win32 Thunderbird and Seamonkey (probably Calendar as well). Working on a fix

I commented out the following two lines in common.nsh since TextFunc.nsh and FileFunc.nsh that comes with the version of NSIS (2.17) on the non newref win32 tbox doesn't have the define to prevent loading these files a second time. + !include TextFunc.nsh + !include FileFunc.nsh

BTW, the bugs for upgrading the Thunderbird and community tinderboxen to the newref image are marked as dependencies to bug 384624

Seth, I verified this fix compiling both Firefox and Thunderbird using NSIS 2.17 which is used prior to the MozillaBuild env.

Comment on attachment 277903 [details] [diff] [review] patch - fix includes for older versions of NSIS (checked in to trunk) r=sspitzer

Comment on attachment 277903 [details] [diff] [review] patch - fix includes for older versions of NSIS (checked in to trunk) Checked in to trunk Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v <-- common.nsh new revision: 1.24; previous revision: 1.23 done Checking in mozilla/toolkit/mozapps/installer/windows/nsis/overrides.nsh; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/overrides.nsh,v <-- overrides.nsh new revision: 1.6; previous revision: 1.5 done

Tested against Firefox and Thunderbird using both NSIS 2.22 (e.g. newref) and NSIS 2.17 (e.g. community tinderboxen)

Comment on attachment 278115 [details] [diff] [review] patch part 2 rev2 (not quite there) Found a minor glitch when using the macros in uninstall

These changes have been heavily tested with all toolkit apps using both NSIS 2.17 (e.g. Community tinderbox) and NSIS 2.22 (e.g. Firefox tinderbox). There is a lot of moving of code and code cleanup in this patch. The main functional differences are: The macros that would overwrite registers no longer do GetSingleInstallPath no longer returns a path ending with a \ RegCleanUninstall now uses SHCTX. This prepares the installer to be able to write to HKCU uninstall keys for installing as a user without admin rights. These changes make it a lot easier to fix bugs in the installer for all toolkit apps that choose to use the common macros and should make it easier for toolkit apps to maintain the installer by lessening the amount of common code they have to maintain. Sunbird is a prime example of a toolkit app that will benefit from these changes. I patched Thunderbird to use one common macro since it was the only other call site for GetSingleInstallPath and this will automatically fix the change in behaviour... I've discussed these changes with mscott and he is all for these changes. The remaining changes to the toolkit app installers will be made in separate bugs per app and it is opt in for each app. There is more to do but this is a good point to get these changes landed.

btw: this patch includes the fix for bug 352510 but it doesn't include the locale and defines.nsi.in changes... the patch in bug 352510 has those changes.

Comment on attachment 279713 [details] [diff] [review] patch part 2 rev3 will be meeting with robert soon to go over this.

I removed the changes to the progress bar to simplify the review. I'll fix that in a separate bug.

Comment on attachment 280621 [details] [diff] [review] patch part 2 rev4 some other changes I'm working on snuck in... will resubmit shortly

update, rstrong sat down and walked me through the patch. in doing so, he found about 2 or 3 things he wanted to either verify or clean up. once he does, he'll be uploading a new patch.

Comment on attachment 280670 [details] [diff] [review] patch part 2 rev6 r=sspitzer, thanks for walking me through this review. seeking a= (on robert's behalf) for m9

Comment on attachment 280853 [details] [diff] [review] patch part 3 rev1 Seth, I'm going to go with this simple one as is.

Comment on attachment 280853 [details] [diff] [review] patch part 3 rev1 >Index: toolkit/mozapps/installer/windows/nsis/common.nsh >=================================================================== ... >+ !ifdef MUI_INNERTEXT_LICENSE_BOTTOM I changed this in my tree to MUI_INNERTEXT_LICENSE_BOTTOM_CHECKBOX >+ !insertmacro MUI_LANGUAGEFILE_LANGSTRING_PAGE LICENSE "MUI_INNERTEXT_LICENSE_BOTTOM_CHECKBOX" >+ !endif

This looks like a fairly big change (and the fact that the reviewer needed walking through it doesn't make me particularly sanguine either). What's the risk assessment here? And why wasn't it made when requesting approval, for that matter?

The vast majority of this patch is moving code around so when a change is needed that applies to the installers for all toolkit app's I can make the change in one place and thereby not have to get reviews, etc. from the owner's of each app. The reason I walked the reviewer through the changes is that the syntax for NSIS installers is not easy to understand especially when the reviewer seldom works on NSIS code. This is due to my being the the only one actively patching the toolkit installer code and most of the toolkit apps installers. Risk of landing: as with all patches the risk is that there will be regressions with the caveat that the installer has been "well owned" and I will fix any regressions that occur. Risk of not landing: I will most likely not have the time to patch other toolkit app's installers (see Sunbird's installer which is currently VERY out of date). This applies to both the 1.9 release and security / bugfix releases of 1.9. Sorry about not providing a risk assessment. I just noticed that the toolkit NSIS Installer has two sets of area drivers ( http://wiki.mozilla.org/Firefox3/Drivers#Firefox ) and I only saw that mconnor and beltzner were the area drivers since they were at the top of the page and I have discussed the above issues with mconnor previously... still though, when Seth made the a1.9 request I should have jumped in and provided a risk assessment.

> I just noticed that the toolkit NSIS Installer has two sets of area drivers Uh... indeed. That would be a screwup. Beltzner or mconnor are probably the right folks to be approving this, in fact. I'll get that page (and more importantly the bugzilla queries) adjusted.

Comment on attachment 280853 [details] [diff] [review] patch part 3 rev1 Actually, this patch is good to go

Comment on attachment 281267 [details] [diff] [review] Part 2 as checked in Part 2 checked in to trunk Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v <-- common.nsh new revision: 1.26; previous revision: 1.25 done Checking in mozilla/toolkit/mozapps/installer/windows/nsis/makensis.mk; /cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/makensis.mk,v <-- makensis.mk new revision: 1.12; previous revision: 1.11 done Checking in mozilla/browser/installer/windows/nsis/installer.nsi; /cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v <-- installer.nsi new revision: 1.33; previous revision: 1.32 done Checking in mozilla/browser/installer/windows/nsis/shared.nsh; /cvsroot/mozilla/browser/installer/windows/nsis/shared.nsh,v <-- shared.nsh new revision: 1.14; previous revision: 1.13 done Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi; /cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v <-- uninstaller.nsi new revision: 1.10; previous revision: 1.9 done Checking in mozilla/calendar/installer/windows/nsis/installer.nsi; /cvsroot/mozilla/calendar/installer/windows/nsis/installer.nsi,v <-- installer.nsi new revision: 1.11; previous revision: 1.10 done Checking in mozilla/calendar/installer/windows/nsis/uninstaller.nsi; /cvsroot/mozilla/calendar/installer/windows/nsis/uninstaller.nsi,v <-- uninstaller.nsi new revision: 1.4; previous revision: 1.3 done Checking in mozilla/mail/installer/windows/nsis/installer.nsi; /cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v <-- installer.nsi new revision: 1.17; previous revision: 1.16 done Checking in mozilla/mail/installer/windows/nsis/shared.nsh; /cvsroot/mozilla/mail/installer/windows/nsis/shared.nsh,v <-- shared.nsh new revision: 1.9; previous revision: 1.8 done Checking in mozilla/mail/installer/windows/nsis/uninstaller.nsi; /cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v <-- uninstaller.nsi new revision: 1.8; previous revision: 1.7 done Checking in mozilla/suite/installer/windows/nsis/installer.nsi; /cvsroot/mozilla/suite/installer/windows/nsis/installer.nsi,v <-- installer.nsi new revision: 1.7; previous revision: 1.6 done Checking in mozilla/suite/installer/windows/nsis/shared.nsh; /cvsroot/mozilla/suite/installer/windows/nsis/shared.nsh,v <-- shared.nsh new revision: 1.3; previous revision: 1.2 done Checking in mozilla/suite/installer/windows/nsis/uninstaller.nsi; /cvsroot/mozilla/suite/installer/windows/nsis/uninstaller.nsi,v <-- uninstaller.nsi new revision: 1.3; previous revision: 1.2 done

Comment on attachment 280853 [details] [diff] [review] patch part 3 rev1 I'm going to close this and finish the rest in other bugs.

Resolving fixed. I'll touch base with the toolkit apps when these common macros are available for use.