Closed Bug 392303 Opened 12 years ago Closed 12 years ago

Simplify installer changes

Categories

(Toolkit :: NSIS Installer, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: rstrong, Assigned: rstrong)

References

Details

Attachments

(1 file, 9 obsolete files)

193.51 KB, patch
Details | Diff | Splinter Review
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
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #277617 - Attachment description: patch in progress → patch part 1 - rev 1
Attachment #277617 - Flags: review?(sspitzer)
Comment on attachment 277617 [details] [diff] [review]
patch part 1 - rev 1 (checked in)

r=sspitzer
Attachment #277617 - Flags: review?(sspitzer) → review+
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.
Attachment #277903 - Flags: review?(sspitzer)
Comment on attachment 277903 [details] [diff] [review]
patch - fix includes for older versions of NSIS (checked in to trunk)

r=sspitzer
Attachment #277903 - Flags: review?(sspitzer) → review+
Attachment #277617 - Attachment description: patch part 1 - rev 1 → patch part 1 - rev 1 (checked in)
Attachment #277617 - Attachment is obsolete: true
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
Attachment #277903 - Attachment description: patch - fix includes for older versions of NSIS → patch - fix includes for older versions of NSIS (checked in to trunk)
Attachment #277903 - Attachment is obsolete: true
Tested against Firefox and Thunderbird using both NSIS 2.22 (e.g. newref) and NSIS 2.17 (e.g. community tinderboxen)
Attachment #277989 - Attachment is obsolete: true
Attachment #278115 - Flags: review?(sspitzer)
Comment on attachment 278115 [details] [diff] [review]
patch part 2 rev2 (not quite there)

Found a minor glitch when using the macros in uninstall
Attachment #278115 - Attachment description: patch part 2 rev2 → patch part 2 rev2 (not quite there)
Attachment #278115 - Flags: review?(sspitzer)
Attached patch patch part 2 rev3 (obsolete) — Splinter Review
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.
Attachment #278115 - Attachment is obsolete: true
Attachment #279713 - Flags: review?(benjamin)
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.
Target Milestone: --- → mozilla1.9 M9
Comment on attachment 279713 [details] [diff] [review]
patch part 2 rev3

will be meeting with robert soon to go over this.
Attachment #279713 - Flags: review?(sspitzer)
Attachment #279713 - Attachment is obsolete: true
Attachment #279713 - Flags: review?(sspitzer)
Attachment #279713 - Flags: review?(benjamin)
Attached patch patch part 2 rev4 (obsolete) — Splinter Review
I removed the changes to the progress bar to simplify the review. I'll fix that in a separate bug.
Attachment #280621 - Flags: review?(sspitzer)
Comment on attachment 280621 [details] [diff] [review]
patch part 2 rev4

some other changes I'm working on snuck in... will resubmit shortly
Attachment #280621 - Attachment is obsolete: true
Attachment #280621 - Flags: review?(sspitzer)
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.
Attached patch patch part 2 rev6 (obsolete) — Splinter Review
Attachment #280659 - Attachment is obsolete: true
Attachment #280670 - Flags: review?(sspitzer)
Attachment #280659 - Flags: review?(sspitzer)
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
Attachment #280670 - Flags: review?(sspitzer)
Attachment #280670 - Flags: review+
Attachment #280670 - Flags: approval1.9?
Comment on attachment 280853 [details] [diff] [review]
patch part 3 rev1

Seth, I'm going to go with this simple one as is.
Attachment #280853 - Attachment description: patch part 3 rev1 in progress → patch part 3 rev1
Attachment #280853 - Flags: review?(sspitzer)
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.
Attachment #280670 - Flags: approval1.9? → approval1.9+
Attachment #280670 - Attachment is obsolete: true
Attachment #280853 - Attachment is obsolete: true
Attachment #280853 - Flags: review?(sspitzer)
Comment on attachment 280853 [details] [diff] [review]
patch part 3 rev1

Actually, this patch is good to go
Attachment #280853 - Attachment is obsolete: false
Attachment #280853 - Flags: review?(sspitzer)
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.
Attachment #280853 - Attachment is obsolete: true
Attachment #280853 - Flags: review?(sspitzer)
Resolving fixed. I'll touch base with the toolkit apps when these common macros are available for use.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.