We want to use NSIS for the Firefox installer on Windows (e.g. this bug is not for other platform / OS installers). Applications such as Thunderbird, XULRunner, etc. will be handled separately from this bug though the installer code will be written in a manner where applications other than Firefox will be able to use the same code as appropriate.

Should fix bug 322206 simultaneously.

Is there any particular reason that NSIS has been tagged to be used rather than an MSI installer (bug 231062) That would be a pretty major help for enterprise deployments.

As stated in bug 231062 there are a couple of reasons for having a non msi installer - one of these being the size of the file produced. As to why the msi package is not finished I would hope someone in bug 231062 could answer that for you.

Rob, are you thinking that most of the registry-munging is going to be carried out by the NSI scripts or by a separate program? i.e. 1) install Firefox files 2) call <bindir>/installregister.exe I see an opportunity here to unify the code in nsPostUpdateWin and the installer so that they can be maintained in one place.

I get questions about commonLocale.nsh. L10n builds don't have any concept of NPOB, too.

Axel, that's ridiculous, of course they have NPOB.

The file is not done and until it is it is rather pointless to get people started on something they will probably have to change again. I understand there was a question as to the encoding used. This can be found by downloading NSIS and looking at the encoding for the respective file for the translation... or you can wait until it is ready and I'll provide the details. I didn't know l10n doesn't understand NPOB... I suggest doing what you normally do under these circumstances. If the normal course of action is removing the file that's fine.

Benjamin, you wrote compare-locales.pl, which is part of our l10n build process and DOES NOT CARE about NPOB. Regarding the file itself, I'm operating under the impression of "no change for localizers", so I expect that new file format to go away anyway.

> Benjamin, you wrote compare-locales.pl, which is part of our l10n build process > and DOES NOT CARE about NPOB. Sure. It also doesn't particularly care about new files (it emits a warning, which is safely ignored and doesn't turn the tbox orange). > Regarding the file itself, I'm operating under the impression of "no change > for localizers", so I expect that new file format to go away anyway. What the hell does that mean?

mozilla/toolkit/mozapps/installer/packager.mk 1.37 mozilla/browser/installer/Makefile.in 1.23 After doing a build you can now do "make -C browser/installer installer-stage", which will put the nonlocalized files in <objdir>/installer-stage/nonlocalized and the localized files in <objdir>/installer-stage/localized

Retargetted for beta1

So I have been talking with bsmedberg and been told that the file format in http://lxr.mozilla.org/mozilla/source/toolkit/locales/en-US/installer/windows/commonLocale.nsh is supposed to be it. This is really a script file, right? Couldn't we just make a #defines file for l10n and preprocess a commonLocales.sh.in from generic instead? That's what we do for install.rdf, which IMHO is even less complex than what I see in the tree. We could even iconv the preprocessed file afterwards to abstract the encoding problems and to make checking those part of the build process and not a QA problem. What do we intend to do for those locales that don't have their langauge listed yet? Not that I have a concrete example apart from the indic languages, which may still be required to fallback to english due to encoding problems. Those language tags are hard to map to our iso codes :-/. Can they provide a localization or is that a restriction that we had to upstream to nsis?

There is an additional locale file in the NSIS installation for each locale. If there is a Firefox locale without a NSIS locale, they will need to translate this additional file (and we can send their translation upstream).

I'll see what can be done to simplify the format of the file that contains our additional strings.

The rest will follow shortly. All I did was integrate it with the existing installer packaging.

Comment on attachment 222845 [details] [diff] [review] packaging changes Hrm, all this perl stuff has got to go. I suppose that for the moment this is the simplest way to do stuff.

The remaining files. I screwed up with the line endings on the initial checkins so this patch has a bunch of line removals.

Comment on attachment 222912 [details] [diff] [review] installer files >Index: browser/installer/windows/nsis/7zipNSIS.bat >=================================================================== >RCS file: browser/installer/windows/nsis/7zipNSIS.bat >diff -N browser/installer/windows/nsis/7zipNSIS.bat >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ browser/installer/windows/nsis/7zipNSIS.bat 22 May 2006 19:43:52 -0000 >@@ -0,0 +1,4 @@ >+7z a -t7z app.7z setup.exe config ..\..\..\installer-stage\* -mx -m0=BCJ2 -m1=LZMA:d24 -m2=LZMA:d19 -m3=LZMA:d19 -mb0:1 -mb0s1:2 -mb0s2:3 I'm a little surprised this works and doesn't record ..\..\.. in the 7zip file. >+cd .. This shouldn't be necessary... batch files run in a subprocess. >Index: browser/installer/windows/nsis/SetProgramAccess.nsi >+# The Initial Developer of the Original Code is Robert Strong >+# Portions created by the Initial Developer are Copyright (C) 2006 >+# the Initial Developer. All Rights Reserved. Actually Mozilla Foundation owns the copyright. >+; Provides the Set Access portion of the "Set Program Access and Defaults" that >+; is available with Win2K Pro SP3 and WinXP SP1 (this does not specifically call >+; out Windows Vista which has not been released at the time of this comment or >+; any other future versions of Windows). >+ >+; !insertmacro EXIT_APP "${FileMainEXE}" "${BrandFullName}" "uninstall" I don't understand this comment, or why this call is commented out. (Or, if this does what it looks like it does, why we'd need/want to do this.) >Index: browser/installer/windows/nsis/defines.nsi.in >+!define AppVersion "@MOZ_APP_VERSION@" >+!define GREVersion @MOZILLA_VERSION@ not quoted? This could be an arbitrary-ish string. >+!define FileInstallerEXE "@PKG_BASENAME@.installer.exe" Do we want to name this differently from the xpinstall-based installer for the moment, so that we can build both in parallel? >Index: browser/installer/windows/nsis/instfiles-extra.nsi >@@ -0,0 +1,59 @@ >+; Only for Firefox and only if they don't already exist >+ ; Check if QuickTime is installed and copy the contents of its plugins >+ ; directory into the app's plugins directory. Previously only the >+ ; nsIQTScriptablePlugin.xpt files was copied which is not enough to enable >+ ; QuickTime as a plugin. Previously == in ff1.5? I hate this hack and would prefer to remove it if it wasn't working in ff1.5 >Index: toolkit/mozapps/installer/windows/nsis/installer.nsi >+; XXXrstrong - do we want to register IE as the default browser on uninstall Really we want to save the previous value and restore it. Please file a followup bug on this (I suppose there are older bugs on this already filed). >+ ; Register DLLs >+ ; XXXrstrong - AccessibleMarshal.dll can be used by multiple applications but >+ ; is only registered for the last application installed. When the last >+ ; application installed is uninstalled AccessibleMarshal.dll will no longer be >+ ; registered. File a bug on this? I need to investigate the activation methods for accessibility, but I really don't think we need to be using MS-COM in this context. >+ ${If} $InstallType != 4 >+ ${If} ${FileExists} "$INSTDIR\extensions\talkback@mozilla.org" >+ Call install_talkback >+ ${EndIf} >+ ${If} ${FileExists} "$INSTDIR\extensions\inspector@mozilla.org" >+ Call install_inspector >+ ${EndIf} >+ ${EndIf} I'm a bit lost at this point. Is this the default install path, forcing upgrade of installed DOMI/inspector? >+ ; The previous installer adds several regsitry values to both HKLM and HKCU. >+ ; Should we only add these values to HKCU if we don't have access to HKLM? Realistically we should be intelligently picking one or the other, but let's leave that one for the moment. We'll need to solve it for FF3+xulrunner though. >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Quick Launch Shortcut" $AddQuickLaunchSC >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Desktop Shortcut" $AddDesktopSC >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Start Menu Shortcut" $AddStartMenuSC Is it necessary to keep this hack around? The only reason these were set in the old installer was to communicate the settings to the install.js script, which I don't think is an issue any more (am I wrong?). >+ ; The Reinstall Command is defined at >+ ; http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/programmersguide/shell_adv/registeringapps.asp >+ StrCpy $0 "Software\Clients\StartMenuInternet\$R9\InstallInfo" >+ StrCpy $9 "$\"$INSTDIR\uninstall\uninstall.exe$\" /ua $\"${AppVersion} (${AB_CD})$\" /hs browser" Umm, do we have to support this command? Do we actually do it well? I'm not sure I understand this. >+ ; XXXrstrong - Should this be localized? Yeah... file followup? >+# XXXrstrong - if dir still exists at this point perhaps prompt user to ask if >+# they would like to delete it anyway with appropriate safe gaurds Talk to beltzner... sounds bad from a UI perspective, since most users will have no clue what the consequences of either option are (I don't really know myself). >Index: toolkit/mozapps/installer/windows/nsis/version.nsh >+VIAddVersionKey "FileVersion" "${AppVersion}" This is a bit iffy: FileVersion is really supposed to be a 16.16.16.16 number, and we really don't treat it like that at all. I'd prefer to leave out FileVersion altogether and use a custom MozVersion string field if this is simple to do. If not, a followup bug would be good (see bug 302046 for some background).

Reset default browser - Bug 255225 (already filed) Error during install registering AccessibleMarshal.dll (trunk only) - Bug 338807 (already filed) Only one install of AccessibleMarshal.dll can be registered - Bug 338878 (already filed) Writing to both HKLM and HKCU - Bug 338890 Localization of the shortcuts - Bug 338892

> I'm a little surprised this works and doesn't record ..\..\.. in the 7zip file. That only happens with Mac OS X > >+cd .. > This shouldn't be necessary... batch files run in a subprocess. Removed > Actually Mozilla Foundation owns the copyright. Fixed > >+; !insertmacro EXIT_APP "${FileMainEXE}" "${BrandFullName}" "uninstall" > > I don't understand this comment, or why this call is commented out. (Or, if > this does what it looks like it does, why we'd need/want to do this.) Removed > >+!define AppVersion "@MOZ_APP_VERSION@" > >+!define GREVersion @MOZILLA_VERSION@ > > not quoted? This could be an arbitrary-ish string. That particular one is already quoted while the others are not. File a bug? > >+!define FileInstallerEXE "@PKG_BASENAME@.installer.exe" > > Do we want to name this differently from the xpinstall-based installer for the > moment, so that we can build both in parallel? It is currently named in install_sub.pl. I changed it for now so it is name-nsis.exe > >@@ -0,0 +1,59 @@ > >+; Only for Firefox and only if they don't already exist > >+ ; Check if QuickTime is installed and copy the contents of its plugins > >+ ; directory into the app's plugins directory. Previously only the > >+ ; nsIQTScriptablePlugin.xpt files was copied which is not enough to enable > >+ ; QuickTime as a plugin. > > Previously == in ff1.5? I hate this hack and would prefer to remove it if it > wasn't working in ff1.5 It was kind of working in 1.5. I'll talk with jst about trying to get Apple on board to using the reg keys the same as flash and others. > >Index: toolkit/mozapps/installer/windows/nsis/installer.nsi > > >+; XXXrstrong - do we want to register IE as the default browser on uninstall > > Really we want to save the previous value and restore it. Please file a > followup bug on this (I suppose there are older bugs on this already filed). filed > >+ ; Register DLLs > >+ ; XXXrstrong - AccessibleMarshal.dll can be used by multiple applications but > >+ ; is only registered for the last application installed. When the last > >+ ; application installed is uninstalled AccessibleMarshal.dll will no longer be > >+ ; registered. > > File a bug on this? I need to investigate the activation methods for > accessibility, but I really don't think we need to be using MS-COM in this > context. filed > >+ ${If} $InstallType != 4 > >+ ${If} ${FileExists} "$INSTDIR\extensions\talkback@mozilla.org" > >+ Call install_talkback > >+ ${EndIf} > >+ ${If} ${FileExists} "$INSTDIR\extensions\inspector@mozilla.org" > >+ Call install_inspector > >+ ${EndIf} > >+ ${EndIf} > > I'm a bit lost at this point. Is this the default install path, forcing upgrade > of installed DOMI/inspector? This allows the installer to have a complete button along with the basic and advanced. I cleaned up the logic slightly here as well. > >+ ; The previous installer adds several regsitry values to both HKLM and HKCU. > >+ ; Should we only add these values to HKCU if we don't have access to HKLM? > > Realistically we should be intelligently picking one or the other, but let's > leave that one for the moment. We'll need to solve it for FF3+xulrunner though. filed > >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Quick Launch Shortcut" $AddQuickLaunchSC > >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Desktop Shortcut" $AddDesktopSC > >+ ${WriteRegDWORDHKLMandHKCU} "$0" "Create Start Menu Shortcut" $AddStartMenuSC > > Is it necessary to keep this hack around? The only reason these were set in the > old installer was to communicate the settings to the install.js script, which I > don't think is an issue any more (am I wrong?). At some point but for now I want to keep them. The logic used currently is confusing and cumbersome and I'd like to clean it all up at the same time. > >+ ; The Reinstall Command is defined at > >+ ; http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/programmersguide/shell_adv/registeringapps.asp > >+ StrCpy $0 "Software\Clients\StartMenuInternet\$R9\InstallInfo" > >+ StrCpy $9 "$\"$INSTDIR\uninstall\uninstall.exe$\" /ua $\"${AppVersion} (${AB_CD})$\" /hs browser" > > Umm, do we have to support this command? Do we actually do it well? I'm not > sure I understand this. It is standard browser app behavior and the previous installer supported it though it was somewhat broken. > >+ ; XXXrstrong - Should this be localized? > > Yeah... file followup? filed > >+# XXXrstrong - if dir still exists at this point perhaps prompt user to ask if > >+# they would like to delete it anyway with appropriate safe gaurds > > Talk to beltzner... sounds bad from a UI perspective, since most users will > have no clue what the consequences of either option are (I don't really know > myself). will do > >Index: toolkit/mozapps/installer/windows/nsis/version.nsh > > >+VIAddVersionKey "FileVersion" "${AppVersion}" > > This is a bit iffy: FileVersion is really supposed to be a 16.16.16.16 number, > and we really don't treat it like that at all. I'd prefer to leave out > FileVersion altogether and use a custom MozVersion string field if this is > simple to do. If not, a followup bug would be good (see bug 302046 for some > background). I'll look into it Thanks for taking on this painful review!

Also filed Bug 338901 to improve the progress text display

I had to change a few things to get it working with the build system including what is in this patch. One thing I saw in the build logs is that we are building the installer twice. :(

This 7zips directly from installer-stage, and fixes some other issues of system() calls that aren't checked.

Comment on attachment 223095 [details] [diff] [review] 7zip directly from installer-stage, rev. 1 [fixed on trunk] Looks good and thanks

Minefield. shortcut is not created by Standard/Complete install.

Benajamin, I found a copy of 7-Zip 3.12 which is the same version installed on pacifica and worked out compatible args for both that version and the newer versions of 7-Zip. I already checked this in in order to get things moving but I thought it best to let you know and review the patch.

The installer is finally being distributed to the mirrors... yah! For the time being it will be named similarly to the XPInstall based installer with -nsis added to the file name like so: XPInstall based installer: firefox-2.0a2.en-US.win32.installer.exe NSIS based installer: firefox-2.0a2.en-US.win32.installer-nsis.exe This will change in the near future if all goes well. To build you need to set in your environment both MOZ_PACKAGE_NSIS and MOZ_INSTALLER_USE_7ZIP. The reason for this is that the tinderbox scripts make the installer twice and it unsets MOZ_INSTALLER_USE_7ZIP to prevent the XPInstall based SEA installer from being built two times. For now NSIS is riding along with this to prevent it from being built a second time. This will hopefully change in the near future so it will only require MOZ_PACKAGE_NSIS. As with the previous installer you will need 7-Zip and you should also have UPX in your path when building. In addition, you will also need NSIS in your path. The latest released version works fine. http://nsis.sourceforge.net/ There are plenty of things left that need to be coded for this installer so file *new* bugs when you find problems. Also, I filed meta bug 339061 for tracking the NSIS installer work that is needed for Firefox 2.0. Besides searching, this will also be a good place to check if the bug you found has already been filed. To make it simpler to hack on the NSIS installer code we are using NSIS's Logic Library (e.g. LogicLib.nsh). This allows the use of syntax similar to other languages and will usually be simpler to understand for new and old hackers alike. Also, they have an excellent forum and their documentation is in very good shape (http://nsis.sourceforge.net/). As always, patches are welcome. The Installer is dead. Long live the Installer!

Have you guys considered upgrading 7-zip to the latest stable release, 4.42? Supposedly the compression ratio has been improved for the higher settings. It might be worth investigating anyway.

Comment on attachment 223140 [details] [diff] [review] Make 7-Zip args compatible with old and new versions Hrm, we don't have to specify which files ought to be zipped?

(In reply to comment #29) > Have you guys considered upgrading 7-zip to the latest stable release, 4.42? > Supposedly the compression ratio has been improved for the higher settings. It > might be worth investigating anyway. > Well, I went ahead and tried it myself using the tinderbox 7zip parameters. The difference between 3.12 and 4.42 was that 4.42 produced an archive which was actually 4K larger. I guess that answers that :-)

(In reply to comment #30) > (From update of attachment 223140 [details] [diff] [review] [edit]) > Hrm, we don't have to specify which files ought to be zipped? When nothing is specified for what to include it includes the contents of the cwd. The older version did strange things with directories when a wildcard was used which is fixed in the newer version.

I should have also stated that it did strange things with multiple things to include as well.

Hey Rob, when I was doing a test build of the new installer, I noticed that the build process is creating and copying files into a directory called instgen and it was putting that directory in my CVS tree: mozilla/mail/installer/windows/instgen. Should these installer build files get put in dist/instgen instead? Like the old installer did with dist\stage and dist\install?

Scott, the old installer used that directory and nsis is just going along for the ride by adding a couple of files. The installer build process needs cleaning up but that should be done separately.

(In reply to comment #14) > I'll see what can be done to simplify the format of the file that contains our > additional strings. > Does this need a separate bug?

(In reply to comment #36) > (In reply to comment #14) > > I'll see what can be done to simplify the format of the file that contains our > > additional strings. > > > > Does this need a separate bug? Filed bug 340173.