CPack/NSIS: generated package fails when uninstalling old package

I've noticed on a new(ish) development machine that packages I generate fail when trying to uninstall old versions of themselves (ie. CPACK_NSIS_ENABLE_UNINSTALL_BEFORE_INSTALL). You can however uninstall them manually through Windows Add/Remove programs, but that defeats the purpose and convenience.

I've made up a fairly minimal test case by just removing my actual project and leaving the CPACK related stuff.

What I've noticed is that by downgrading (I had an old copy of CMake 3.5 x86 lying) around, the problem went away. However on 3.22.0 and 3.22.1 I see the problem. I tried both 64bit and 32bit installers of 3.22.1 (thinking it might shake something loose as only 32bit installs were available back in the CMake 3.5 days, but to no avail).

My environment is Windows 10, NSIS 3.08 (and also 3.05 prior to upgrading it), and CMake 3.22.1.

mytest.cpp

CMakeLists.txt

changed title from CPack generated NSIS package fails when uninstalling old package to CPack/NSIS: generated package fails when uninstalling old package

added area:cpack label

Cc: @justinclift @craig.scott @petitlapin @sgmarkus @lassoan

CPack/NSIS uninstall-related changes since CMake 3.5 include:

• commit 11768733
• !127 (merged)
• !3945 (merged)
• !6050 (merged)
• !6517 (merged)

Cc: @mc

• !7096 (merged)

This is yet another quoted string/space in path issue.

Uninstall fails due to CMake passing a quoted path to Uninstall.exe's _?= argument. It looks like the NSIS Uninstall.exe was implemented poorly (it did not support quoted paths) and developers "fixed" the problem by documentation: _?= ... must be the last parameter used in the command line and must not contain any quotes, even if the path contains spaces. (source).

Windows requires UninstallString registry value to be quoted (otherwise uninstall using "Apps & Features" fails) and the CMake-generated installers set this key correctly now (see this fix). However, the CMake-generated uninstall code in project.nsi does not strip the quotes and also adds extra quotes in ExecWait:

uninst:
  ClearErrors
  StrLen $2 "\Uninstall.exe"
  StrCpy $3 $0 -$2 # remove "\Uninstall.exe" from UninstallString to get path
  ExecWait '"$0" /S _?=$3' ;Do not copy the uninstaller to a temp file

If the UninstallString is quoted (in all new installations) then this code works correctly (it strips quotes when setting $3 and does not add quotes in ExecWait):

uninst:
  ClearErrors
  StrLen $2 "\Uninstall.exe$\""
  StrCpy $3 $0 -$2 1 # remove "\Uninstall.exe" from UninstallString to get path
  ExecWait '$0 /S _?=$3' ;Do not copy the uninstaller to a temp file

If you want to make the script work with both quoted (current) and unquoted (legacy) uninstall strings then you could add a check if the string is quoted and if not then add quotes at the beginning of uninst.

This patch seems to work, but I'm by no means NSIS competent. It just removes the quotes (conditionally in case of legacy Uninstall.exe) and lets the ExecWait add them back in. I believe ExecWait should still quote $0, according to this.

remove-quotes.patch

diff -Naur a/cmake-3.22.1/Modules/Internal/CPack/NSIS.template.in b/cmake-3.22.1/Modules/Internal/CPack/NSIS.template.in
--- a/cmake-3.22.1/Modules/Internal/CPack/NSIS.template.in      2021-12-07 11:44:21.000000000 -0400
+++ b/cmake-3.22.1/Modules/Internal/CPack/NSIS.template.in      2021-12-11 10:12:25.422489734 -0400
@@ -932,6 +932,20 @@
 ;Run the uninstaller
 uninst:
   ClearErrors
+
+  # $0 should _always_ be quoted, however older versions of CMake did not
+  # do this.  We'll conditionally remove the begin/end quotes.
+
+  # Remove first char if quote
+  StrCpy $2 $0 1 1      # copy first char
+  StrCmp $2 "$\"" 0 +2  # if char is quote
+    StrCpy $0 $0 1      # remove first char
+
+  # Remove last char if quote
+  StrCpy $2 $0 1 -1     # copy last char
+  StrCmp $2 "$\"" 0 +2  # if char is quote
+    StrCpy $0 $0 -1     # remove last char
+
   StrLen $2 "\@CPACK_NSIS_UNINSTALL_NAME@.exe"
   StrCpy $3 $0 -$2 # remove "\@CPACK_NSIS_UNINSTALL_NAME@.exe" from UninstallString to get path
   ExecWait '"$0" /S _?=$3' ;Do not copy the uninstaller to a temp file

mentioned in commit xpxaxsxi/cmake@2e3dc814

mentioned in merge request !7716 (closed)

on 3.22.0 and 3.22.1 I see the problem.

I think that !6517 (merged) introduced this problem while trying to fix support for spaces in the path to the uninstaller.

Then !7096 (merged) attempted to fix it by adding a check for existing double quotes before adding them.

Now !7716 (closed) revises that check to work on a host platform where it did not before.

I have been working with this (the !7716 (closed) was my first failed attempt) problem and I have a working solution, at least with 64-bit executables and cross-compiling in Linux to Windows, maybe this is a special case, but you may want check out this

I could not figure out why, but NSIS thinks that it is making an 32-bit executable and writing to wrong registry places.

I could not find a better solution and decided to add a new option CPACK_NSIS_ONINIT_REGVIEW to NSIS.template.in and set that to 'SetRegView 64'.

This works for me now: cpack -D CPACK_NSIS_EXTRA_UNINSTALL_COMMANDS='SetRegView 64' -D CPACK_NSIS_ONINIT_REGVIEW='SetRegView 64'

There is also an executable if you want to check

Since !7716 (closed) is closed, is there any news on this? Is the patch above the recommended workaround?

We have been bitten hard by this issue. The symptom for us is that the installer does not wait for the uninstaller to finish. Therefore the uninstaller and the installer both run in parallel leaving the installation directory a complete mess...

!7096 (merged) added quoting. What goes wrong now?

I think this is a different issue, and it's caused by b795c967

It seems the uninstaller works differently with _? and without.

• With _? the uninstaller (Uninstall.exe) works as intended and the installer waits for it to finish.
• Without _?, the uninstaller (Uninstall.exe) actually spawns a new Un_A.exe and exits immediately, so that the installer thinks the uninstallation is done, while the spawned child doing the uninstallation is actually still running...

This seems to work for me (fixed version of the patch above):

diff --git a/CMake/NSIS.template.in b/CMake/NSIS.template.in
index 42a44d96c..21753afe7 100644
--- a/CMake/NSIS.template.in
+++ b/CMake/NSIS.template.in
@@ -931,11 +931,20 @@ Function .onInit
 ;Run the uninstaller
 uninst:
   ClearErrors
-  StrCpy $2 $0 1
-  StrCmp '"' $2 0 +3 ; checks if string is quoted (CPack before v3.20.6 did not quote it)
-  ExecWait '$0 /S'
-  Goto +2
-  ExecWait '"$0" /S'
+  # $0 should _always_ be quoted, however older versions of CMake did not
+  # do this.  We'll conditionally remove the begin/end quotes.
+  # Remove first char if quote
+  StrCpy $2 $0 1 0      # copy first char
+  StrCmp $2 "$\"" 0 +2  # if char is quote
+  StrCpy $0 $0 "" 1     # remove first char
+  # Remove last char if quote
+  StrCpy $2 $0 1 -1     # copy last char
+  StrCmp $2 "$\"" 0 +2  # if char is quote
+  StrCpy $0 $0 -1       # remove last char
+
+  StrLen $2 "\@CPACK_NSIS_UNINSTALL_NAME@.exe"
+  StrCpy $3 $0 -$2 # remove "\@CPACK_NSIS_UNINSTALL_NAME@.exe" from UninstallString to get path
+  ExecWait '"$0" /S _?=$3' ;Do not copy the uninstaller to a temp file

   IfErrors uninst_failed inst
 uninst_failed:

Yes, the docs state (emphasis mine):

_?= sets $INSTDIR. It also stops the uninstaller from copying itself to the temporary directory and running from there. It can be used along with ExecWait to wait for the uninstaller to finish. It must be the last parameter used in the command line and must not contain any quotes, even if the path contains spaces.

So I really think _? is needed and it was problematic to remove it.

If it's a different issue, please open a new issue for that and include your explanation, thanks.

A merge request to put things in your proposed state will also be appreciated, thanks.

@brad.king See #24041 (closed) & !7774 (merged)