392303 - Simplify installer changes

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

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

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.

Comment 1

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

Attachment: patch part 1 - rev 1 (checked in)

I need to test but this looks hopeful

Comment 2

[(not reading, please use [e-mail] instead)]

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

r=sspitzer

Comment 3

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

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

Comment 4

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

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

Comment 5

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

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

Comment 6

[Robert Kaiser]

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

Comment 7

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

Attachment: patch - fix includes for older versions of NSIS (checked in to trunk)

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

Comment 8

[(not reading, please use [e-mail] instead)]

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

r=sspitzer

Comment 9

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

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

Comment 10

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

Attachment: patch part 2 rev1

Comment 11

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

Attachment: patch part 2 rev2 (not quite there)

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

Comment 12

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

Comment on attachment 278115 [details] [diff] [review]
patch part 2 rev2 (not quite there)

Found a minor glitch when using the macros in uninstall

Comment 13

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

Attachment: patch part 2 rev3

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.

Comment 14

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

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 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 279713 [details] [diff] [review]
patch part 2 rev3

will be meeting with robert soon to go over this.

Comment 16

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

Attachment: patch part 2 rev4

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

Comment 17

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

Comment on attachment 280621 [details] [diff] [review]
patch part 2 rev4

some other changes I'm working on snuck in... will resubmit shortly

Comment 18

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

Attachment: patch part 2 rev5

Comment 19

[(not reading, please use [e-mail] instead)]

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 20

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

Attachment: patch part 2 rev6

Comment 21

[(not reading, please use [e-mail] instead)]

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 22

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

Attachment: patch part 3 rev1

Comment 23

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

Comment on attachment 280853 [details] [diff] [review]
patch part 3 rev1

Seth, I'm going to go with this simple one as is.

Comment 24

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

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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?

Comment 26

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

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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 28

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

Attachment: Part 2 as checked in

Comment 29

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

Comment on attachment 280853 [details] [diff] [review]
patch part 3 rev1

Actually, this patch is good to go

Comment 30

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

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 31

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

Comment on attachment 280853 [details] [diff] [review]
patch part 3 rev1

I'm going to close this and finish the rest in other bugs.

Comment 32

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

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