Closed Bug 367539 Opened 17 years ago Closed 14 years ago

When upgrading an existing install use the uninstall.log to uninstall the previous version before install.

Categories

(Firefox :: Installer, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .9-fixed
blocking1.9.1 --- needed
status1.9.1 --- .12-fixed

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(11 files, 7 obsolete files)

1.27 KB, patch
sipaq
: review+
Details | Diff | Splinter Review
5.61 KB, patch
jimm
: review+
Details | Diff | Splinter Review
27.72 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
787 bytes, patch
philor
: review+
Details | Diff | Splinter Review
781 bytes, patch
Details | Diff | Splinter Review
7.42 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
6.26 KB, patch
philor
: review+
Details | Diff | Splinter Review
4.44 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
5.98 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
1.05 KB, patch
mossop
: review+
Details | Diff | Splinter Review
28.00 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
This will allow the removal of reg keys when downgrading and is just safer overall.
Summary: When upgrading an existing install run uninstall silently prior to installing. → When upgrading an existing install use the uninstall.log to uninstall the previous version before install.
Nominating for 1.8.1.3 since bug 369102 made it into 1.8.0.10 and this will accomplish the same thing in a cleaner manner for 2.0.0.x.
Flags: blocking1.8.1.3?
Taking to get this on my radar
Assignee: nobody → robert.bugzilla
Not blocking ("stop everything and fix this"), but as with 369102 we might re-consider taking it if a fix gets implemented and tested in Firefox 3 first.
Flags: blocking1.8.1.4? → blocking1.8.1.4-
MSI is likely the better answer for this so reassigning back to default. If someone wants to take this on please do so
Assignee: robert.bugzilla → nobody
We're not going to get MSI's before 3.7 so re-taking since this is another one of those PITA bugs.

Basic process (iirc jar files were locked exclusively in 2.0 so this should work just fine)

Parse the uninstall log
Rename existing file if it exists from log
Delete the renamed file with the REBOOTOK flag
Use similar / same logic to require a reboot as currently exists (might need some refactoring).
Assignee: nobody → robert.bugzilla
Target Milestone: --- → mozilla1.9.3
Attached patch 2. SeaMonkey patch (obsolete) — Splinter Review
Attached patch 3. Thunderbird patch (obsolete) — Splinter Review
Attached patch 4. Sunbird patchSplinter Review
Simon, just in case someone compiles Sunbird's installer I'd like to remove the call for creating the removed-files.log since that will fail. If you would like me to I can also update the Sunbird installer as is being done for the other apps... it should only take a few minutes for me to create the patch.
Attachment #452544 - Flags: review?(bugzilla)
Comment on attachment 452544 [details] [diff] [review]
4. Sunbird patch

looks fine to me.
r=sipaq
Attachment #452544 - Flags: review?(bugzilla) → review+
Attachment #452541 - Attachment is obsolete: true
Attachment #452662 - Flags: review?(jmathies)
No longer blocks: 496207
Attached patch 3. Thunderbird patch (obsolete) — Splinter Review
Phil, this along with the mozilla-central patch removes the use of removed-files for figuring out which files need to be removed when installing and instead uninstalls existing files / dirs listed in the uninstall.log. This way installing an older version on top of a pre-existing install will just work.
Attachment #452543 - Attachment is obsolete: true
Attachment #452663 - Flags: review?(philringnalda)
Attached patch 2. SeaMonkey patch (obsolete) — Splinter Review
Callek, this along with the mozilla-central patch removes the use of
removed-files for figuring out which files need to be removed when installing
and instead uninstalls existing files / dirs listed in the uninstall.log. This
way installing an older version on top of a pre-existing install will just
work.
Attachment #452542 - Attachment is obsolete: true
Attachment #452664 - Flags: review?(bugspam.Callek)
Attachment #452546 - Flags: review?(jmathies)
Drivers, this removes the requirement to update a previous releases removed-files list to include the new files in the latest release which has been one of the easily forgotten todo items whenever we do a release... I would really, really, really like to get this on the branches.
Status: NEW → ASSIGNED
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
I do not understand how this addresses the bug description - "When upgrading an existing install use the uninstall.log to uninstall the previous version before install."

This patch removes the uninstall.log generation from build? Some context might help. The patch itself looks fine, but I don't understand why we are doing it.
The patch removed the removed-files.log generation during build which was used to figure out which files needed to be removed and instead uses the previous installation's uninstall.log to remove the files / directories (if empty) for the previous installation before installing the new installation.

Comment #15 describes the reason why
Some history... removed-files was created to remove files on app update for all platforms. To support removing files on pave over installs it was processed during build time to create a file that is read by the installer to know which files should not be present in the new installation. This works ok for upgrading to a newer version but fails badly when downgrading to a previous version and we mitigate that as described in comment #15.

With this patch the existing installation's uninstall.log which is created at install time is read and all files are either removed or if the file is in use
1. it renames the file then removes the file on OS reboot
2. if it can't be renamed
  a) if the file exists in the source left in place to be replaced on OS reboot
  b) if the file doesn't exist in the source removes the file on OS reboot

So, the existing installation's uninstall.log is used to remove the previous installation's files before install and the use of the removed-files.log is removed.
Attachment #452662 - Flags: review?(jmathies) → review+
Attachment #452546 - Flags: review?(jmathies) → review+
Comment on attachment 452662 [details] [diff] [review]
1. main patch for mozilla-central (checked in)

Patch 1 pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/29248f12faa7

After I get reviews for the comm-central patches I'll push them and the final mozilla-central patches
Attachment #452662 - Attachment description: 1. main patch for mozilla-central → 1. main patch for mozilla-central (checked in)
$R9 was already prefixed with a '\' so I removed the extra '\' in the IfFileExists checks as follows
      IfFileExists "$EXEDIR\nonlocalized$R9" end +1
      IfFileExists "$EXEDIR\localized$R9" end +1
      IfFileExists "$EXEDIR\optional$R9" end +1
Attachment #452662 - Attachment is obsolete: true
Attachment #452940 - Flags: review+
Comment on attachment 452663 [details] [diff] [review]
3. Thunderbird patch

No more putting a random assortment of new files in removed-files on stable branches, and feeling vaguely guilty about not caring enough to do others? DO WANT!
Attachment #452663 - Flags: review?(philringnalda) → review+
Turns out that Thunderbird was using GetSingleInstallPath in installer.nsi without declaring !insertmacro GetSingleInstallPath and instead relying on the same declaration in common.nsh
Comment on attachment 452956 [details] [diff] [review]
Thunderbird fix (checked in)

Oops, bad us.
Attachment #452956 - Flags: review+
Comment on attachment 452956 [details] [diff] [review]
Thunderbird fix (checked in)

Pushed Thunderbird fix to comm-central
http://hg.mozilla.org/comm-central/rev/0dc792d7dffa

I suspect the missing declaration was my doing. :(
Attachment #452956 - Attachment description: Thunderbird fix → Thunderbird fix (checked in)
Turns out the same is true for SeaMonkey... checked this in to fix the bustage
Includes the removal of !insertmacro GetSingleInstallPath added by attachment #452956 [details] [diff] [review]. Carrying forward r+
Attachment #452663 - Attachment is obsolete: true
Attachment #452965 - Flags: review+
Includes the removal of !insertmacro GetSingleInstallPath added by attachment #452961 [details] [diff] [review]
Attachment #452664 - Attachment is obsolete: true
Attachment #452966 - Flags: review?(bugspam.Callek)
Attachment #452664 - Flags: review?(bugspam.Callek)
Comment on attachment 452966 [details] [diff] [review]
2. SeaMonkey patch

also requesting review from Phil since Callek said he could review this over irc
Attachment #452966 - Attachment is patch: true
Attachment #452966 - Attachment mime type: application/octet-stream → text/plain
Attachment #452966 - Flags: review?(philringnalda)
Attachment #452940 - Attachment description: 1. mozilla-central main patch as pushed → 1. mozilla-central main patch as pushed (checked in)
Attached file 1. main patch for mozilla-1.9.2 (obsolete) —
This doesn't remove the following from PreDirectoryCommon to prevent consumers that don't have that in their installer.nsi from failing.
!ifndef NO_INSTDIR_FROM_REG
    !insertmacro GetSingleInstallPath
!endif

Other than that it is the same patch as attachment #452940 [details] [diff] [review] for 1.9.2
Attachment #452988 - Flags: review+
Not necessary to land with the main patch but it would be a good thing for SeaMonkey
Not necessary to land with the main patch but it would be a good thing for Thunderbird. This doesn't have the removal of the Makefile.in changes for the removed-files.log generation since the removal isn't necessary. Same goes for the SeaMonkey patch
Attachment #452993 - Flags: review+
The 1.9.2 patches also apply fine to 1.9.1
Jim, SeaMonkey and Thunderbird have code to handle MapiProxy_InUse.dll and mozMapi32_InUse.dll being in use so this code should just ignore these two files.
Attachment #453006 - Flags: review?(jmathies)
Attachment #453006 - Flags: review?(dtownsend)
Comment on attachment 453006 [details] [diff] [review]
let application's deal with MapiProxy_InUse.dll and mozMapi32_InUse.dll if present (checked in)

Seems straightforward
Attachment #453006 - Flags: review?(dtownsend) → review+
Attachment #453006 - Flags: review?(jmathies)
Comment on attachment 453006 [details] [diff] [review]
let application's deal with MapiProxy_InUse.dll and mozMapi32_InUse.dll if present (checked in)

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/bd08417f6869
Attachment #453006 - Attachment description: let application's deal with MapiProxy_InUse.dll and mozMapi32_InUse.dll if present → let application's deal with MapiProxy_InUse.dll and mozMapi32_InUse.dll if present (checked in)
The functionality is already landed and all that is left is the review of the SeaMonkey patch and then landing all of the comm-central patches along with the final mozilla-central patch. The functionality itself landed with the main patch and the rest are just cleanup.
Requesting branch approval for a rollup patch with minimal changes for 1.9.2 and 1.9.1
Attachment #453229 - Flags: review+
Attachment #453229 - Flags: approval1.9.2.6?
Attachment #453229 - Flags: approval1.9.1.11?
Attachment #452966 - Flags: review?(philringnalda)
Attachment #452966 - Flags: review?(bugspam.Callek)
Attachment #452966 - Flags: review+
Comment on attachment 452966 [details] [diff] [review]
2. SeaMonkey patch

LGTM.
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/4c7be0d2836d

Pushed to comm-central
Thunderbird
http://hg.mozilla.org/comm-central/rev/73d458b52ccb

SeaMonkey
http://hg.mozilla.org/comm-central/rev/2671e8da40f3

Sunbird
http://hg.mozilla.org/comm-central/rev/2fbc10fc82ef

I'm pretty sure there are existing litmus tests for the installer already which covers this about as well as we can.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Not "blocking" a branch release, marking "needed" so we see it again later but definitely not taking this in the waning days of 1.9.2.6/1.9.1.11
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
Attachment #453229 - Flags: approval1.9.2.7?
Attachment #453229 - Flags: approval1.9.2.6?
Attachment #453229 - Flags: approval1.9.1.12?
Attachment #453229 - Flags: approval1.9.1.11?
I think the last time I pushed drivers to accept a patch for the branch was probably around 4 years ago and I typically just have the attitude that if a patch isn't approved we'll just get to it the next time around instead of making a fuss. In this instance, the waning days of 1.9.2.6/1.9.1.11 equates to end of July with the understanding that we want to freeze early due to the summit. What ever the reason, that is around a month away for 1.9.2.6/1.9.1.11 and likely another month for 1.9.2.7/1.9.1.12 where this patch *might* be approved. Also, not accepting this patch can lead to some users ending up with a Firefox that doesn't work when downgrading without uninstalling first. I don't think this happens very often but this applies to all Windows users which is the vast majority of our user base. The patch itself uses a modified version of the code that does our uninstall and moves a couple of blocks of existing code to better check when the user should reboot before performing an install and files in use checks.
Comment on attachment 453229 [details] [diff] [review]
1. main patch for mozilla-1.9.2 and mozilla-1.9.1

Approved for 1.9.2.8 and 1.9.1.12, a=dveditz for release-drivers
Attachment #453229 - Flags: approval1.9.2.8?
Attachment #453229 - Flags: approval1.9.2.8+
Attachment #453229 - Flags: approval1.9.1.12?
Attachment #453229 - Flags: approval1.9.1.12+
Attachment #452988 - Attachment is obsolete: true
Attachment #452988 - Attachment mime type: application/octet-stream → text/plain
Attachment #452988 - Flags: review+
Comment on attachment 452993 [details] [diff] [review]
Thunderbird patch for mozilla-1.9.2

These aren't necessary to land the main patch but they are "nice to haves".
Attachment #452993 - Flags: approval1.9.2.8?
Attachment #452993 - Flags: approval1.9.1.12?
Comment on attachment 452992 [details] [diff] [review]
SeaMonkey patch for mozilla-1.9.2

These aren't necessary to land the main patch but they are "nice to haves".
Attachment #452992 - Flags: review+
Attachment #452992 - Flags: approval1.9.2.8?
Attachment #452992 - Flags: approval1.9.1.12?
Attachment #452992 - Flags: approval1.9.2.8?
This has a=beltzner for 1.9.2.9 - that flag just doesn't, um, exist yet.
Comment on attachment 453229 [details] [diff] [review]
1. main patch for mozilla-1.9.2 and mozilla-1.9.1

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers
Comment on attachment 452992 [details] [diff] [review]
SeaMonkey patch for mozilla-1.9.2

Approved for 1.9.1.12, a=dveditz for release-drivers
Attachment #452992 - Flags: approval1.9.1.12? → approval1.9.1.12+
Comment on attachment 452993 [details] [diff] [review]
Thunderbird patch for mozilla-1.9.2

Approved for 1.9.2.9 and 1.9.1.12, a=dveditz for release-drivers
Attachment #452993 - Flags: approval1.9.2.8?
Attachment #452993 - Flags: approval1.9.2.8+
Attachment #452993 - Flags: approval1.9.1.12?
Attachment #452993 - Flags: approval1.9.1.12+
Pushed to mozilla-1.9.2
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6f2999bb8d4b

Pushed Thunderbird patch to comm-1.9.2
http://hg.mozilla.org/releases/comm-1.9.2/rev/09f8d91be420

Looks like SeaMonkey doesn't need a comm-1.9.2 patch but if SeaMonkey wants it on 1.9.2 they can push the attachment #452992 [details] [diff] [review]
Depends on: 585649
Component: NSIS Installer → Installer
Product: Toolkit → Firefox
You need to log in before you can comment on or make changes to this bug.