[Windows] Wait for uninstaller, rework .dll bundling, add logging (#7407)

* [Windows] Purge every runtime DLL on upgrade instead of a named few

A user updating from 3.0.2 to 3.1 got "abseil_dll.dll is either not designed
to run on Windows or it contains an error" and could not start the client.

abseil_dll.dll comes from vcpkg, not Qt. It rides into the install directory
on the "*.dll" glob in cockatrice/CMakeLists.txt, alongside libprotobuf.dll,
liblzma.dll and z.dll, and none of those were named in the upgrade purge. So
on an upgrade the old uninstaller's RMDir /r skipped whichever of them was
locked, the purge did not look for them, HasStaleRuntimeFiles did not look
for them either, and the installer reported success - having written the new
executables next to the previous version's protobuf runtime. Windows then
refused to load it.

The mask list was also already wrong where it existed: it matches zlib*.dll
while vcpkg ships z.dll.

Delete every top-level DLL instead of naming library families. The payload is
written after the purge, so a DLL the new build still needs is put straight
back; what the purge has to guarantee is only that nothing from the previous
build is left to be loaded next to the new executables. Enumerated families
cannot do that - every transitive dependency that arrives between two
releases is one more name to forget, and the three previous rounds of this
work each reasoned about Qt because that is what the reports named at the
time.

HasStaleRuntimeFiles collapses to the matching *.dll probe, so the abort
fires on any survivor rather than only on the families it was told about.

IsExistingCockatriceInstall stays on Qt6*.dll on purpose: it decides whether a
directory is ours at all, and a folder that merely holds some DLLs of its own
must not be purged. Qt6*.dll is enough even when only the protobuf runtime
survived, because the whole Qt set is mapped by the same process that maps
abseil_dll.dll and so is locked or free with it.

* [CI] Assert the whole Windows runtime, not just the Qt modules

required-qt-runtime.txt listed only the Qt modules COCKATRICE_QT_MODULES
names, so it could not see any other runtime go missing. The three previous
rounds of installer work all reasoned about Qt*.dll because that was all the
build knew how to ask about, and the one runtime that actually broke the
reported upgrade - abseil_dll.dll - was outside the set entirely.

- Rename it to required-runtime.txt and take the third party runtime names
  from the vcpkg bin directory the app-local copies come from, so a
  dependency bump cannot leave the list behind and a versioned name such as
  libcrypto-3-x64.dll becoming libcrypto-4-x64.dll cannot either
- Union the three applications' module lists rather than the client's alone:
  Servatrice contributes Qt6Sql.dll, servatrice.exe is installed next to
  cockatrice.exe and cannot start without it, and nothing asserted it
- Add Core to the client's module list. It is the one module nothing can do
  without and it only ever arrived transitively, so Qt6Core.dll was shipped
  and never checked - the new unknown-runtime check flagged it on the first
  run
- Fail the build on a missing runtime, as before, and warn on a runtime
  nothing requires. Only the test framework is a hard failure there: "no
  shipped executable links gtest" is a fact, whereas a runtime missing from
  the manifest may be one the manifest failed to derive, and a check that has
  never run before should not be able to block a release

The warning is meant to be read. Qt6::Quick pulls in Qml, QmlModels,
QmlWorkerScript, QuickBasic, QuickTemplates2 and OpenGL transitively and none
are named, so the manifest is known to be a partial list; the first Windows
run reports the real one, which is evidence rather than a guess about what Qt
links. Fold that back in when it lands.

* [CI] Stop deriving the required runtime from the vcpkg bin directory

The first Windows run of this check failed, and the seven files it named as
missing are not runtime at all: gmock.dll, gmock_main.dll, legacy.dll,
libprotobuf-lite.dll, libprotoc.dll and pthreadVC3/VCE3/VSE3.dll. None of them
is linked by any shipped executable, so a correct package leaves them out.

vcpkg_installed/x64-windows/bin is everything vcpkg installed, not everything
the applications import. The protoc compiler, the GoogleMock library, a port
tool and three CMake FindThreads probes sit beside the genuine runtimes, and a
directory listing cannot tell them apart. Asserting it fails the build over a
package that is right - which is what happened, and it is the same mistake as
the one that produced this bug report: reasoning about a list of names instead
of about the build.

The real source of truth is the three executables' import tables, which
fixup_bundle already walks to decide what to copy. Until CMake can read that
result, the Qt half is what can be derived honestly, from the same module lists
the targets are built against. So assert that, and report the rest rather than
require it:

- Drop the vcpkg bin glob, its gtest filter and the vcpkg-runtime-known marker.
  All three existed only to support the glob.
- Report top level DLLs the manifest does not name without failing. These are
  expected: protobuf, Abseil, OpenSSL, liblzma, ffmpeg and the Qt debug DLLs all
  arrive on the build output glob because nothing in the build declares them by
  name. The list is a report of what the package carries, not a verdict, and it
  is how the missing Qt6Qml/Quick/OpenGL entries above became visible.
- Report a leaked gtest or gmock instead of failing on it. "No shipped
  executable links this" is a fact, but a check that has never passed on a real
  package has not earned the right to block a release, and the first run of an
  enforcing version is exactly when its own wrong assumptions show up.
- Skip DLLs under a subdirectory, which are platform plugins rather than
  imported runtimes, and the NSIS plugins, which extract to $PLUGINSDIR and so
  reach the comparison looking like payload.

Verified against the paths 7-Zip actually listed in the failing package: the
check passes on it, still fails when Qt6Sql.dll or the qwindows plugin is
removed, and still reports gtest.dll and gmock.dll if either is added.

The purge in cmake/NSIS.template.in covers the runtimes the manifest does not
name. That fix is independent of any list and is what makes the reported update
failure go away.

* [Installer] Fix the /R upgrade aborting before it does anything

WaitForLauncherToExit read the wrong registers. System.dll writes its
.rN outputs to $N and its .RN outputs to $RN, but the wait used
p.r0/i.r1 while testing $R1. $R1 therefore stayed empty, and the "did
not exit" test compared "" with "0" - true every time. Every in-app
update passes /PID=, so every upgrade hit AbortInstall before writing
anything; because /R also sets silent, the override just vanished with
exit code 1 and no message. The bug came in with #7372 and is inherited
by this branch, not introduced by it.

Use the uppercase .RN forms, and treat a null or plugin-failed
OpenProcess as the launcher already being gone - the case the old
${Errors} check never caught, because System.dll does not set the NSIS
error flag from the callee's return value.

Also restore the abort dialog for the in-app updater: AbortInstall and
the two inline warnings gated their message box on ${IfNot} ${Silent},
which is false for a /R upgrade, so any abort on that path was equally
silent. The unattended case is a /S install without /R, not /R, so show
the box for $ReinstallMode = 1 or an interactive install. The
stale-runtime abort now goes through AbortInstall so that decision
lives in one place.

* [Installer] Log every upgrade step to %TEMP%\Cockatrice-installer.log

* [Installer] Actually append to the upgrade log (FileSeek END)

* [Installer] Wait for the old uninstaller before replacing files

An NSIS uninstaller copies itself to %TEMP% and runs from there so that it
can delete its own directory; the process the installer started exits
immediately. A plain ExecWait therefore returns before the uninstall has
run, and the temp copy's "RMDir /r $INSTDIR" then executes concurrently
with the install section, deleting files the payload just wrote. That is
why an upgrade could leave Qt6Core/Gui/Multimedia and the other
alphabetically-first DLLs missing while the tail of the directory survived:
a recursive delete marching from the front while the final listing ran.

Pass _?=$INSTDIR to the old uninstaller so it runs in place and ExecWait
actually blocks until the uninstall completes. _?= must be the last
argument and unquoted, even for paths with spaces. Applied to both the
silent /R path (AutoUninstallIfNeeded) and the interactive path
(componentsPagePre).

* [Installer] Tighten the wording of the upgrade comments

Drop the dash-heavy phrasing from the comments this PR adds, and say plainly
in the _?= comment that ExecWait returns before the old uninstaller has run
and the temp copy then deletes the payload the section is still writing.

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

.ci/compile.sh

@@ -353,15 +353,21 @@ if [[ $MAKE_PACKAGE ]]; then
       fi
       echo "Installer content is clean"
 
-      # Fail the build if the installer is missing a Qt runtime the applications
-      # link against. The DLLs reach the package by globbing every *.dll out of
-      # the build output directories, so a module the client links statically
-      # (Qt6Multimedia.dll, Qt6QuickWidgets.dll) can be left out without anything
-      # else noticing, and the only symptom is a client that refuses to start
-      # after an update. required-qt-runtime.txt is written by the top level
-      # CMakeLists.txt from the same module list the targets are built against.
-      if [[ ! -f required-qt-runtime.txt ]]; then
-        echo "::error file=$0::required-qt-runtime.txt not found in the build dir, cannot verify the Qt runtime"
+      # Fail the build if the installer does not carry the runtime the
+      # applications link against - a file that must be there, or a DLL that must
+      # not be. The runtime DLLs reach the package by globbing every *.dll out of
+      # the build output directories, so anything that build produced can ride
+      # along and nothing else in the build objects to it: a Qt module the client
+      # links statically (Qt6Multimedia.dll, Qt6QuickWidgets.dll) can be left out,
+      # and the test framework's gtest.dll can be shipped. Either only surfaces on
+      # a user's machine after an update has already replaced their install, as
+      # "The code execution cannot proceed because Qt6Multimedia.dll was not
+      # found" or "abseil_dll.dll is either not designed to run on Windows or it
+      # contains an error". required-runtime.txt is written by the top level
+      # CMakeLists.txt from the same module lists the targets are built against
+      # plus the vcpkg bin directory the app-local copies come from.
+      if [[ ! -f required-runtime.txt ]]; then
+        echo "::error file=$0::required-runtime.txt not found in the build dir, cannot verify the runtime"
         exit 1
       fi
       # 7-Zip writes CRLF on Windows, so the \r has to come off before any
@@ -371,7 +377,7 @@ if [[ $MAKE_PACKAGE ]]; then
       # being a wildcard; -i because that is how the installed tree behaves -
       # the uninstaller clears both $INSTDIR\plugins and $INSTDIR\Plugins.
       # The archive side and the manifest side have to agree on line endings.
-      # CMake writes required-qt-runtime.txt with the platform's line ending and
+      # CMake writes required-runtime.txt with the platform's line ending and
       # 7-Zip writes CRLF on Windows, so a trailing \r survives on $entry and
       # defeats the whole-line match - every entry then reads as missing even
       # though the listing holds it verbatim.
@@ -380,7 +386,7 @@ if [[ $MAKE_PACKAGE ]]; then
       installer_paths="$("$seven_zip" l -slt "$package" | tr -d '\r' |
         sed -n 's/^Path = //p' |
         sed -e 's|\\|/|g' -e 's|^[$]INSTDIR/||' -e 's|^[$]OUTDIR/||' -e 's|^[$]PLUGINSDIR/||' -e 's|^\./||' -e 's|^/||')"
-      manifest="$(tr -d '\r' <required-qt-runtime.txt)"
+      manifest="$(tr -d '\r' <required-runtime.txt)"
       missing=""
       checked=0
       while IFS= read -r entry; do
@@ -393,11 +399,11 @@ if [[ $MAKE_PACKAGE ]]; then
         fi
       done <<<"$manifest"
       if [[ -n $missing ]]; then
-        echo "::error file=$0::Installer is missing required Qt runtime files:$missing"
+        echo "::error file=$0::Installer is missing required runtime files:$missing"
         # Print both sides of the comparison. A mismatch here has twice been
         # caused by the shape of 7-Zip's output rather than by a missing file,
         # and a bare "missing" line cannot tell those apart.
-        echo "required-qt-runtime.txt asked for $checked path(s); 7-Zip listed $(grep -c '' <<<"$installer_paths") path(s)"
+        echo "required-runtime.txt asked for $checked path(s); 7-Zip listed $(grep -c '' <<<"$installer_paths") path(s)"
         # Indent with parameter expansion rather than sed 's/^/  /', which the
         # linter reports as SC2001.
         echo "--- required ---"
@@ -407,7 +413,66 @@ if [[ $MAKE_PACKAGE ]]; then
         echo "${listed_head//$'\n'/$'\n  '}"
         exit 1
       fi
-      echo "Installer contains the full Qt runtime ($checked files)"
+      echo "Installer carries the Qt runtime of every module the three applications link ($checked files)"
+
+      # Now the other direction: a top level DLL in the package that the manifest
+      # does not name. The missing check above cannot see a runtime that was
+      # never expected.
+      #
+      # Two things are reported here and neither may fail the build.
+      #
+      # The test framework is a fact rather than a guess - no shipped executable
+      # links gtest or gmock, and the build output directories the "*.dll" glob
+      # reads are shared with the test targets, so gtest.dll and gmock.dll are
+      # one filter away from shipping in every release. Even so it is reported
+      # instead of enforced: a check that has never passed on a real package has
+      # not earned the right to block a release, and the first Windows run of an
+      # enforcing version is exactly when its own wrong assumptions show up.
+      #
+      # Everything else is expected to be there. required-runtime.txt names the
+      # Qt modules the three applications link, and the protobuf, Abseil,
+      # OpenSSL, liblzma, ffmpeg and Qt debug DLLs in the package come in on the
+      # build output glob instead, because nothing in the build declares them by
+      # name. So this list is a report of what the package carries, not a verdict.
+      # Read it when a dependency is added; do not fail on it.
+      forbidden=""
+      unexpected=""
+      while IFS= read -r entry; do
+        [[ -z $entry ]] && continue
+        # Top level only. A DLL under Plugins/ or a nested image format plugin is
+        # a plugin the platform loads by name, not a runtime the executable
+        # imports, and requiring one here would list all of them every run.
+        [[ $entry == */* ]] && continue
+        if grep -qxiF "$entry" <<<"$manifest"; then
+          continue
+        fi
+        # NSIS extracts its plugins to $PLUGINSDIR, which the path normalization
+        # above strips, so they arrive here looking like payload files. They are
+        # part of the installer, not of the installed tree.
+        if [[ $entry == [Nn][Ss][Dd][Ii][Aa][Ll][Oo][Gg][Ss].dll ||
+          $entry == [Nn][Ss][Ee][Xx][Ee][Cc].dll ||
+          $entry == [Ss][Yy][Ss][Tt][Ee][Mm].dll ||
+          $entry == [Uu][Ss][Ee][Rr][Ii][Nn][Ff][Oo].dll ||
+          $entry == [Ii][Nn][Ee][Tt].dll ]]; then
+          continue
+        fi
+        if [[ $entry == [Gg][Tt][Ee][Ss][Tt]* || $entry == [Gg][Mm][Oo][Cc][Kk]* ]]; then
+          forbidden+=" $entry"
+        else
+          unexpected+=" $entry"
+        fi
+      done < <(grep -i '\.dll$' <<<"$installer_paths")
+      if [[ -n $forbidden ]]; then
+        echo "::warning file=$0::Installer contains test framework DLLs that no shipped executable links:$forbidden"
+        echo "A build output directory shared with the test targets has leaked into the package."
+      fi
+      if [[ -n $unexpected ]]; then
+        echo "Top level DLLs in the package beyond the linked Qt modules:$unexpected"
+        echo "These arrive on the build output glob, not from required-runtime.txt, so they are reported and not required."
+      fi
+      if [[ -z $forbidden && -z $unexpected ]]; then
+        echo "Installer carries no runtime files beyond the linked Qt modules"
+      fi
     fi
     echo "::endgroup::"
   fi

CMakeLists.txt

@@ -326,38 +326,54 @@ elseif(WIN32)
   # Configure file with custom definitions for NSIS.
   configure_file("${COCKATRICE_CMAKE_PATH}/NSIS.definitions.nsh.in" "${PROJECT_BINARY_DIR}/NSIS.definitions.nsh")
 
-  # Every Qt module the client links against, as the DLL names a Windows install
-  # has to contain, plus the platform plugin without which the client cannot
-  # start. .ci/compile.sh reads this and fails the build when the packaged
-  # installer is missing one of them.
+  # Every runtime DLL a Windows install has to contain, as paths relative to the
+  # install root. .ci/compile.sh reads this and fails the build when the packaged
+  # installer is missing one of them, or carries a runtime it does not know about.
   #
-  # Only the client is packaged on Windows, so this is the client's module list
-  # rather than the union across server, client and Oracle: demanding Oracle's or
-  # Servatrice's modules of the client installer checks nothing about the client.
+  # All three applications are packaged into the same directory on Windows, so
+  # this is the union of their Qt module lists, not just the client's: Servatrice
+  # contributes Qt6Sql.dll and Oracle contributes nothing the client lacks, and a
+  # client-only manifest leaves Qt6Sql.dll unasserted even though servatrice.exe
+  # is installed next to cockatrice.exe and cannot start without it.
   #
   # A statically linked Qt module ships no runtime DLL, so requiring one would
   # fail the package check on a static build for no reason. cockatrice/CMakeLists.txt
   # installs the runtime of every module listed here, and skips static ones the
   # same way, so the two can never disagree.
   #
-  # A DLL missing from the package only surfaces on a user's machine, after an
-  # update has already replaced their install, as "The code execution cannot
-  # proceed because Qt6Multimedia.dll was not found" or as a missing procedure
-  # entry point in a Qt6QuickWidgets.dll left over from the previous version.
-  # Assert it while the installer is still being built instead.
-  set(_required_qt_runtime "")
-  foreach(_qt_module IN LISTS COCKATRICE_QT_MODULES)
+  # Only the Qt modules, and that is deliberate. protobuf, Abseil, liblzma, zlib
+  # and OpenSSL also ride in on the "*.dll" glob, and a client whose runtime is
+  # absent, stale or of the wrong architecture fails to start with "abseil_dll.dll
+  # is either not designed to run on Windows or it contains an error", so they
+  # are exactly the files worth asserting.
+  #
+  # Their names cannot be derived from the vcpkg bin directory, which is what a
+  # first attempt did and what a Windows build immediately refuted: bin holds
+  # everything vcpkg installed, not everything the applications import. It also
+  # contains gmock.dll, libprotobuf-lite.dll, libprotoc.dll, legacy.dll and three
+  # pthreadVC*.dll FindThreads probes, none of which any shipped executable
+  # links, so requiring them failed a build over a package that was correct.
+  #
+  # Deriving them from the executables' own import tables is the real answer,
+  # and fixup_bundle already walks it to decide what to copy. Until that result
+  # is available to CMake, requiring the vcpkg half would mean guessing names
+  # again, which is what produced this bug report in the first place. So assert
+  # what is derived from the build's own link lists and leave the rest to the
+  # purge in cmake/NSIS.template.in, which removes every DLL regardless of name.
+  set(_required_runtime "")
+  foreach(_qt_module IN LISTS COCKATRICE_QT_MODULES SERVATRICE_QT_MODULES ORACLE_QT_MODULES)
     get_target_property(_qt_module_type ${_qt_module} TYPE)
     if(_qt_module_type STREQUAL "SHARED_LIBRARY")
       string(REGEX REPLACE "^Qt6::" "" _qt_component "${_qt_module}")
-      list(APPEND _required_qt_runtime "Qt6${_qt_component}.dll")
+      list(APPEND _required_runtime "Qt6${_qt_component}.dll")
     endif()
   endforeach()
-  list(APPEND _required_qt_runtime "Plugins/platforms/qwindows.dll")
-  list(REMOVE_DUPLICATES _required_qt_runtime)
-  list(SORT _required_qt_runtime)
-  string(REPLACE ";" "\n" _required_qt_runtime "${_required_qt_runtime}")
-  file(WRITE "${PROJECT_BINARY_DIR}/required-qt-runtime.txt" "${_required_qt_runtime}\n")
+  list(APPEND _required_runtime "Plugins/platforms/qwindows.dll")
+
+  list(REMOVE_DUPLICATES _required_runtime)
+  list(SORT _required_runtime)
+  string(REPLACE ";" "\n" _required_runtime "${_required_runtime}")
+  file(WRITE "${PROJECT_BINARY_DIR}/required-runtime.txt" "${_required_runtime}\n")
 
   # include vcredist into the package; NSIS will take care of running it
   if(VCREDISTRUNTIME_FOUND)

cmake/FindQtRuntime.cmake

@@ -7,6 +7,12 @@
 # Output: ORACLE_QT_MODULES
 # Output: TEST_QT_MODULES
 
+# Every Qt module an application links against, as the DLL names a Windows
+# install has to contain. Core is listed explicitly rather than left to arrive
+# transitively: it is the one module nothing can do without, and required-runtime.txt
+# is built from these lists, so a module missing here is a runtime the package
+# check never asserts. That is how Qt6Core.dll went unasserted while every other
+# Qt module was checked.
 set(REQUIRED_QT_COMPONENTS Core)
 if(WITH_SERVER)
   set(_SERVATRICE_NEEDED Network Sql WebSockets)
@@ -14,6 +20,7 @@ endif()
 if(WITH_CLIENT)
   set(_COCKATRICE_NEEDED
       Concurrent
+      Core
       Gui
       Multimedia
       Network

cmake/NSIS.template.in

@@ -21,6 +21,13 @@ Var IsUpgrade
 Var AbortReason
 Var WaitPid
 Var LauncherExited
+Var InstallLogPath
+Var LogHandle
+Var LogMessage
+Var LogFindHandle
+Var LogFindName
+Var LogMatchLabel
+Var LogMatchPattern
 
 !include LogicLib.nsh
 !include FileFunc.nsh
@@ -64,8 +71,67 @@ Page Custom PortableModePageCreate PortableModePageLeave
 
 !insertmacro MUI_LANGUAGE English
 
+; Installer log
+; Every run appends a step-by-step trace to a file so a failed upgrade on a
+; user's machine can be diagnosed after the fact. Kept in $TEMP rather than in
+; $INSTDIR because the previous version's uninstaller removes the whole install
+; directory part-way through an upgrade. Read it back with:
+;   notepad %TEMP%\Cockatrice-installer.log
+!macro CockLog text
+    Push "${text}"
+    Call CockLogWrite
+!macroend
+
+Function CockLogWrite
+    Pop $LogMessage
+    FileOpen $LogHandle "$InstallLogPath" a
+    ${If} $LogHandle == ""
+        Return
+    ${EndIf}
+    ; FileOpen places the pointer at the start of the file in every mode, so
+    ; 'a' alone would overwrite the log from the beginning on each line. Seek
+    ; past the existing content first to actually append.
+    FileSeek $LogHandle 0 END
+    FileWrite $LogHandle "$LogMessage$\r$\n"
+    FileClose $LogHandle
+FunctionEnd
+
+; Logs every file matching $INSTDIR\<mask>. Used to record the DLL inventory
+; before and after the upgrade purge, which is what shows whether Qt6Gui.dll was
+; on disk and what (if anything) survived the deletes.
+!macro CockLogListing label mask
+    Push "${label}"
+    Push "$INSTDIR\${mask}"
+    Call CockLogListing
+!macroend
+
+Function CockLogListing
+    Pop $LogMatchPattern
+    Pop $LogMatchLabel
+    !insertmacro CockLog "listing [$LogMatchLabel] pattern=$LogMatchPattern"
+    FindFirst $LogFindHandle $LogFindName "$LogMatchPattern"
+    ${If} $LogFindHandle == ""
+        !insertmacro CockLog "listing [$LogMatchLabel] (no matches)"
+        Return
+    ${EndIf}
+    log_listing_loop:
+        !insertmacro CockLog "listing [$LogMatchLabel] $LogFindName"
+        ClearErrors
+        FindNext $LogFindHandle $LogFindName
+        IfErrors 0 log_listing_loop
+    FindClose $LogFindHandle
+FunctionEnd
+
 Function .onInit
 
+StrCpy $InstallLogPath "$TEMP\Cockatrice-installer.log"
+; Start each run from a clean file so the log that gets sent back is the last
+; run, not every past run glued together.
+FileOpen $LogHandle "$InstallLogPath" w
+FileClose $LogHandle
+!insertmacro CockLog "=================================================================="
+!insertmacro CockLog "installer start: @CPACK_PACKAGE_NAME@ @CPACK_PACKAGE_VERSION@"
+
 ${If} ${NSIS_IS_64_BIT} == 1    #NSIS 64bit
     ${IfNot} ${RunningX64}
         MessageBox MB_OK|MB_ICONSTOP "This version of Cockatrice requires a 64-bit Windows system."
@@ -158,6 +224,13 @@ ${EndIf}
 ; Now that $PortableMode reflects reality, commit InstDir into the correct slot
 Call SetModeDestinationFromInstdir
 
+!insertmacro CockLog "options: InstDir=$InstDir PortableMode=$PortableMode ReinstallMode=$ReinstallMode WaitPid=$WaitPid"
+${If} ${Silent}
+    !insertmacro CockLog "options: running silently"
+${Else}
+    !insertmacro CockLog "options: interactive"
+${EndIf}
+
 ; Wait for the process that launched this installer to exit before anything is
 ; replaced or deleted. It goes first because it is the one check here that needs
 ; no powershell.exe: on a machine where the process lookup cannot run at all, the
@@ -175,12 +248,14 @@ Call SetModeDestinationFromInstdir
 ; interactive installs the section re-checks after the directory page, when
 ; $INSTDIR is the directory the user actually picked.
 ${If} $ReinstallMode = 1
+    !insertmacro CockLog "reinstall: waiting for launcher and other instances"
     Call WaitForLauncherToExit
     Call EnsureAppsNotRunning
 ${EndIf}
 
 ${If} $ReinstallMode = 1
 ${AndIf} $PortableMode = 0
+    !insertmacro CockLog "reinstall: auto-uninstalling previous version"
     Call AutoUninstallIfNeeded
 ${EndIf}
 
@@ -226,13 +301,22 @@ Function AutoUninstallIfNeeded
 
 SetShellVarContext all
 
+; _?=$INSTDIR keeps the old uninstaller running in place. An NSIS uninstaller
+; normally copies itself to %TEMP% and runs from there so that it can delete its
+; own directory, and the process we started exits at once. ExecWait then returns
+; before the uninstall has run, and the temp copy's "RMDir /r $INSTDIR" executes
+; while the section below is still writing the new payload. _?= disables the copy
+; and makes ExecWait block until the uninstall finishes. It must be the last
+; argument and must not be quoted, even when the path contains spaces.
+;
 ; --- 32-bit uninstall ---
 SetRegView 32
 ReadRegStr $R0 HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "QuietUninstallString"
 
+!insertmacro CockLog "uninstall (32-bit): '$R0 _?=$INSTDIR'"
 StrCmp $R0 "" done32
 DetailPrint "Removing previous version (32-bit)..."
-ExecWait '$R0'
+ExecWait '$R0 _?=$INSTDIR'
 
 done32:
 
@@ -241,9 +325,10 @@ ${If} ${RunningX64}
     SetRegView 64
     ReadRegStr $R0 HKLM "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\Cockatrice" "QuietUninstallString"
 
+    !insertmacro CockLog "uninstall (64-bit): '$R0 _?=$INSTDIR'"
     StrCmp $R0 "" done64
     DetailPrint "Removing previous version (64-bit)..."
-    ExecWait '$R0'
+    ExecWait '$R0 _?=$INSTDIR'
 
     done64:
 ${EndIf}
@@ -360,6 +445,7 @@ Function WaitForAppToClose
     ; usage: set $R1 to the display name (e.g. "cockatrice.exe") and $R2 to the
     ; base image name (e.g. "cockatrice")
     Call IsAppRunning
+    !insertmacro CockLog "guard $R1: state=$R0 (dir=$GuardDir)"
     ; Only a definite "running" is worth waiting for. "Not running" is done, and
     ; "could not determine" must not keep the installer waiting on an answer it
     ; will never get - but it cannot simply be waved through either, see
@@ -384,6 +470,7 @@ Function WaitForAppToClose
         Sleep 500
         Call IsAppRunning
         ${If} $R0 = 0
+            !insertmacro CockLog "guard $R1: closed gracefully"
             DetailPrint "$R1 closed."
             Return
         ${EndIf}
@@ -402,6 +489,7 @@ Function WaitForAppToClose
         ; deck, and nothing has been replaced yet, so aborting just defers the
         ; update and preserves the user's data.
         ${If} $AllowForceClose = 0
+            !insertmacro CockLog "guard $R1: still running after 60s, aborting (no force close)"
             DetailPrint "$R1 is still running after 60 s; aborting the update so that no data is lost."
             StrCpy $AbortReason \
                 "$R1 did not close within 60 seconds, so the installation was aborted to avoid losing unsaved data.$\r$\n$\r$\nPlease close $R1 and run the installer again."
@@ -410,11 +498,13 @@ Function WaitForAppToClose
         ; Oracle and Servatrice are console applications: CloseMainWindow()
         ; returned false for them and waiting longer cannot help. They hold no
         ; unsaved user data, so stop them by PID.
+        !insertmacro CockLog "guard $R1: still running after 60s, force-stopping"
         DetailPrint "Force-stopping $R1 ..."
         Call ForceCloseApps
         Sleep 2000
         Call IsAppRunning
         ${If} $R0 = 0
+            !insertmacro CockLog "guard $R1: force-stopped"
             DetailPrint "$R1 closed."
             Return
         ${EndIf}
@@ -422,6 +512,7 @@ Function WaitForAppToClose
             Call HandleUnknownAppState
             Return
         ${EndIf}
+        !insertmacro CockLog "guard $R1: could not be stopped, aborting"
         DetailPrint "$R1 is still running after 60 s; aborting the update."
         StrCpy $AbortReason \
             "$R1 could not be stopped, so the installation was aborted.$\r$\n$\r$\nPlease close $R1 and run the installer again."
@@ -486,8 +577,16 @@ FunctionEnd
 ; Only the first of those two gets a dialog; the second reports through the log
 ; and the exit code.
 Function AbortInstall
-    ${IfNot} ${Silent}
-        MessageBox MB_OK|MB_ICONEXCLAMATION "$AbortReason"
+    !insertmacro CockLog "ABORT: $AbortReason"
+    !insertmacro CockLog "full installer log: $InstallLogPath"
+    ; Show a dialog whenever someone is at the keyboard to see it: an interactive
+    ; install, or the in-app updater's /R upgrade. The /R upgrade is silent, but
+    ; the client that launched the installer has already quit, so the user is
+    ; still there. Only a /S install without /R is attended by a package manager
+    ; and must go straight to the exit code.
+    ${If} $ReinstallMode = 1
+    ${OrIfNot} ${Silent}
+        MessageBox MB_OK|MB_ICONEXCLAMATION "$AbortReason$\r$\n$\r$\nA detailed installer log was written to:$\r$\n$InstallLogPath"
     ${EndIf}
     SetErrorLevel 1
     Abort
@@ -510,17 +609,33 @@ FunctionEnd
 ; falls back to the name based guard.
 Function WaitForLauncherToExit
     StrCpy $LauncherExited 0
+    !insertmacro CockLog "wait-launcher: pid='$WaitPid'"
     ${If} $WaitPid == ""
+        !insertmacro CockLog "wait-launcher: no pid given, falling back to name check"
         Return
     ${EndIf}
     ; PROCESS_SYNCHRONIZE is the only access this needs - enough to wait on the
     ; handle, and the least likely to be refused for a reason that has nothing to
     ; do with the upgrade.
+    ;
+    ; System.dll maps its output registers so that .rN writes $N and .RN writes
+    ; $RN. The caller reads $R0/$R1, so the uppercase .RN form is required; the
+    ; original .r0/.r1 wrote $0/$1 and left $R1 empty, which made the "did not
+    ; exit" test below compare "" against "0" and abort on every /PID= upgrade.
     ClearErrors
-    System::Call 'kernel32::OpenProcess(i 0x00100000, i 0, i $WaitPid) p.r0'
+    System::Call 'kernel32::OpenProcess(i 0x00100000, i 0, i $WaitPid) p.R0'
+    ; OpenProcess returns a null handle for a process that is already gone. The
+    ; System plugin does not raise the NSIS error flag from a called function's
+    ; return value, so ${Errors} alone never saw that case and the null handle was
+    ; passed straight to WaitForSingleObject. Fold a plugin failure into the same
+    ; null-handle case instead of reading a handle that was never written.
     ${If} ${Errors}
+        StrCpy $R0 0
+    ${EndIf}
+    ${If} $R0 == 0
     ; The launcher is gone - OpenProcess cannot open a dead process - which is
     ; exactly the state the wait was for.
+        !insertmacro CockLog "wait-launcher: already exited"
         DetailPrint "The application that started this installer has already exited."
         StrCpy $LauncherExited 1
         Return
@@ -529,14 +644,16 @@ Function WaitForLauncherToExit
     ; Same 60 s grace period the name based check uses, so a client that is slow
     ; to flush its card database does not get less room than it used to.
     ; WAIT_OBJECT_0 is 0; WAIT_TIMEOUT is 258 and a failed wait reports 0xFFFFFFFF.
-    System::Call 'kernel32::WaitForSingleObject(p r0, i 60000) i.r1'
-    System::Call 'kernel32::CloseHandle(p r0)'
+    System::Call 'kernel32::WaitForSingleObject(p R0, i 60000) i.R1'
+    System::Call 'kernel32::CloseHandle(p R0)'
     ${If} $R1 != 0
+        !insertmacro CockLog "wait-launcher: did not exit within 60s (wait=$R1)"
         DetailPrint "The application that started this installer did not exit within 60 s."
         StrCpy $AbortReason \
             "The application that started this update has not exited yet, so the installation was cancelled rather than replace files it is still using.$\r$\n$\r$\nPlease close Cockatrice and run the installer again."
         Call AbortInstall
     ${EndIf}
+    !insertmacro CockLog "wait-launcher: launcher exited"
     DetailPrint "The application that started this installer has exited."
     StrCpy $LauncherExited 1
 FunctionEnd
@@ -562,9 +679,11 @@ FunctionEnd
 Function GuardAppsNotRunning
     ; usage: set $GuardDir to the directory to guard, then call this
     Call IsProcessLookupAvailable
+    !insertmacro CockLog "guard: powershell available=$R0 dir=$GuardDir launcherExited=$LauncherExited"
     ${If} $R0 = 0
         ; Without PowerShell we cannot tell whether a running instance is
         ; holding locks on the files we are about to replace.
+        !insertmacro CockLog "guard: powershell unavailable, skipping instance check"
         DetailPrint "Could not start powershell.exe; skipping the running instance check. Close Cockatrice, Oracle and Servatrice manually if the installation reports a problem."
         ; On a silent upgrade that used to be a warning and nothing more, which
         ; left the very process that launched the installer free to go on
@@ -587,7 +706,8 @@ Function GuardAppsNotRunning
         ${EndIf}
         ; The user has to be told to close the applications by hand, but a /S
         ; install without /R is unattended and would block on this forever.
-        ${IfNot} ${Silent}
+        ${If} $ReinstallMode = 1
+        ${OrIfNot} ${Silent}
             MessageBox MB_OK|MB_ICONEXCLAMATION "This installer could not check whether Cockatrice is still running, because powershell.exe could not be started.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice manually before continuing."
         ${EndIf}
         Return
@@ -633,7 +753,11 @@ Function IsExistingCockatriceInstall
     ; there to prove that it is one. Every Qt module Cockatrice links is part of
     ; the payload, so match them all: probing only Qt6Core.dll would miss a
     ; directory where that one file happened to be deletable and the others
-    ; were not.
+    ; were not. Qt6*.dll rather than *.dll on purpose: this function decides
+    ; whether a directory is ours, and a folder that merely holds some DLLs of
+    ; its own must not be purged. Qt6*.dll is enough even when only the protobuf
+    ; runtime survived, because the whole Qt set is mapped by the same process
+    ; that maps abseil_dll.dll and so is locked or free with it.
     StrCpy $R3 "Qt6*.dll"
     Call CheckStaleRuntimeMask
     ${If} ${FileExists} "$INSTDIR\qt.conf"
@@ -645,36 +769,23 @@ FunctionEnd
 ; the purge. NSIS silently skips whatever it cannot delete, so one surviving file
 ; is enough to prove that something still holds a lock on the install directory.
 ;
-; The masks below have to cover the purge above one for one. A check that only
-; probed named files could not see a locked Qt6Multimedia.dll or a Plugins
-; directory the purge was unable to remove, so the exact files that broke the
-; install were also the ones this check was blind to.
+; Any DLL at all counts, not a list of expected ones. The purge removes every
+; top-level DLL, so after it has run the only DLL that can still be there is one
+; it failed to delete, whatever its name. A mask per library family (Qt, then
+; OpenSSL, then zlib) is what let abseil_dll.dll through: it is in neither the
+; purge nor this check, so it survived a 3.0.2 to 3.1 update untouched, the new
+; executables were written next to it, and the client then failed to start with
+; "abseil_dll.dll is either not designed to run on Windows or it contains an
+; error". Probing *.dll has no list to fall out of date when the next
+; transitive dependency appears.
 Function HasStaleRuntimeFiles
     StrCpy $R0 0
-    StrCpy $R3 "Qt*.dll"
+    StrCpy $R3 "*.dll"
     Call CheckStaleRuntimeMask
-    StrCpy $R3 "libcrypto*.dll"
-    Call CheckStaleRuntimeMask
-    StrCpy $R3 "libssl*.dll"
-    Call CheckStaleRuntimeMask
-    StrCpy $R3 "zlib*.dll"
-    Call CheckStaleRuntimeMask
-    StrCpy $R3 "icu*.dll"
-    Call CheckStaleRuntimeMask
-    StrCpy $R3 "libeay32.dll"
-    Call CheckStaleRuntimeMask
-    StrCpy $R3 "ssleay32.dll"
-    Call CheckStaleRuntimeMask
-    ${If} $R0 = 1
-        Return
-    ${EndIf}
-    ${If} ${FileExists} "$INSTDIR\libmysql.dll"
-        StrCpy $R0 1
-        Return
-    ${EndIf}
     ; IfFileExists takes wildcards, so a Plugins directory the purge could not
     ; remove counts as a leftover too.
     ${If} ${FileExists} "$INSTDIR\Plugins\*.*"
+        !insertmacro CockLog "stale-runtime: Plugins directory survived the purge"
         StrCpy $R0 1
     ${EndIf}
 FunctionEnd
@@ -687,6 +798,7 @@ Function CheckStaleRuntimeMask
     ${EndIf}
     FindFirst $StaleFindHandle $StaleFindName "$INSTDIR\$R3"
     ${If} $StaleFindHandle != ""
+        !insertmacro CockLog "stale-runtime: '$StaleFindName' matched $R3 in $INSTDIR"
         StrCpy $R0 1
         FindClose $StaleFindHandle
     ${EndIf}
@@ -925,9 +1037,10 @@ ${If} $PortableMode = 0
     ; directory before letting it delete anything. The uninstall string is
     ; stashed away first, the check works on $R0.
     StrCpy $OldUninstallString $R0
+    !insertmacro CockLog "componentsPagePre: old 32-bit uninstall '$OldUninstallString _?=$INSTDIR'"
     Call EnsureOldInstallNotRunning
     ClearErrors
-    ExecWait "$OldUninstallString"
+    ExecWait "$OldUninstallString _?=$INSTDIR"
 
 	done32:
 
@@ -947,9 +1060,10 @@ ${If} $PortableMode = 0
 
 	    uninst64:
 	    StrCpy $OldUninstallString $R0
+	    !insertmacro CockLog "componentsPagePre: old 64-bit uninstall '$OldUninstallString _?=$INSTDIR'"
 	    Call EnsureOldInstallNotRunning
 	    ClearErrors
-	    ExecWait "$OldUninstallString"
+	    ExecWait "$OldUninstallString _?=$INSTDIR"
 
 		done64:
 	${EndIf}
@@ -964,15 +1078,19 @@ Section "Application" SecApplication
 SetShellVarContext all
 SetOutPath "$INSTDIR"
 
+!insertmacro CockLog "section: InstDir=$INSTDIR PortableMode=$PortableMode ReinstallMode=$ReinstallMode"
+
 ; No application instance may be running while any file below is deleted or
 ; replaced: both upgrade blocks start by removing the old executables and
 ; runtime DLLs, and NSIS silently skips whatever is locked, which is how an
-; install ends up with a mix of old and new Qt DLLs next to the new executable
-; ("The procedure entry point X could not be located in the dynamic link
-; library ...Qt6Network.dll"). So the check has to come before the first
-; Delete/RMDir and not after it - an interactive install into an existing
-; portable directory with cockatrice.exe still running would otherwise delete
-; the old files under its feet.
+; install ends up with a mix of old and new runtimes next to the new executable.
+; The next start then fails with "The procedure entry point X could not be
+; located in the dynamic link library ...Qt6Network.dll" for the Qt ones, or
+; "abseil_dll.dll is either not designed to run on Windows or it contains an
+; error" for the protobuf ones. So the check has to come before the first
+; Delete/RMDir and not after it: an interactive install into an existing portable
+; directory with cockatrice.exe still running would otherwise delete the old
+; files under its feet.
 ;
 ; It is skipped on the /R path: .onInit already ran it before
 ; AutoUninstallIfNeeded removed the previous installation, so by the time this
@@ -987,6 +1105,8 @@ ${EndIf}
 ${If} $PortableMode = 1
 ${AndIf} ${FileExists} "$INSTDIR\portable.dat"
 	; upgrade portable mode
+	!insertmacro CockLog "section: portable upgrade purge"
+	!insertmacro CockLogListing "before-portable-purge" "*.dll"
 	RMDir /r "$INSTDIR\plugins"
 	RMDir /r "$INSTDIR\sounds"
 	RMDir /r "$INSTDIR\themes"
@@ -995,24 +1115,24 @@ ${AndIf} ${FileExists} "$INSTDIR\portable.dat"
 	Delete "$INSTDIR\cockatrice.exe"
 	Delete "$INSTDIR\oracle.exe"
 	Delete "$INSTDIR\servatrice.exe"
-	Delete "$INSTDIR\Qt*.dll"
-	Delete "$INSTDIR\libmysql.dll"
-	Delete "$INSTDIR\icu*.dll"
-	Delete "$INSTDIR\libcrypto*.dll"
-	Delete "$INSTDIR\libssl*.dll"
-	Delete "$INSTDIR\libeay32.dll"
-	Delete "$INSTDIR\ssleay32.dll"
+	; Every top-level DLL, not a list of expected ones. The payload is written
+	; after this block, so a DLL the new build still needs is put straight back;
+	; what this has to guarantee is that nothing from the previous build is left
+	; to be loaded next to the new executables. Naming the families by hand is
+	; what let abseil_dll.dll through; see HasStaleRuntimeFiles.
+	Delete "$INSTDIR\*.dll"
 	Delete "$INSTDIR\qt.conf"
 	Delete "$INSTDIR\qdebug.txt"
 	Delete "$INSTDIR\servatrice.sql"
 	Delete "$INSTDIR\servatrice.ini.example"
-	Delete "$INSTDIR\zlib*.dll"
 	RMDir "$INSTDIR"
+	!insertmacro CockLogListing "after-portable-purge" "*.dll"
 ${EndIf}
 
 ; Remove any runtime DLLs left over from older versions, so a mismatched
-; Qt/OpenSSL set cannot survive next to the new executables.
+; Qt/OpenSSL/protobuf set cannot survive next to the new executables.
 Call IsExistingCockatriceInstall
+!insertmacro CockLog "section: IsExistingCockatriceInstall=$R0"
 ; Remembered because the stale-runtime check below must only ever fire for a
 ; directory this installer recognised as a Cockatrice install. Scanning for
 ; leftovers in an unrelated directory (a first-time install into a folder that
@@ -1021,48 +1141,48 @@ Call IsExistingCockatriceInstall
 StrCpy $IsUpgrade $R0
 ${If} $PortableMode = 0
 ${AndIf} $R0 = 1
+	!insertmacro CockLog "section: non-portable upgrade purge"
+	!insertmacro CockLogListing "before-purge" "*.dll"
 	RMDir /r "$INSTDIR\Plugins"
-	Delete "$INSTDIR\Qt*.dll"
-	Delete "$INSTDIR\libcrypto*.dll"
-	Delete "$INSTDIR\libssl*.dll"
-	Delete "$INSTDIR\zlib*.dll"
-	Delete "$INSTDIR\libmysql.dll"
-	Delete "$INSTDIR\icu*.dll"
-	Delete "$INSTDIR\libeay32.dll"
-	Delete "$INSTDIR\ssleay32.dll"
+	Delete "$INSTDIR\*.dll"
+	!insertmacro CockLogListing "after-purge" "*.dll"
+	!insertmacro CockLogListing "after-plugins" "Plugins\*.*"
 ${EndIf}
 
-; Whatever was locked during the purge - by an instance that started while the
+; Whatever was locked during the purge (by an instance that started while the
 ; installer was running, or because the running instance check could not be
-; performed at all - is still on disk, and the new executables are about to be
+; performed at all) is still on disk, and the new executables are about to be
 ; written next to it. That mix is exactly what makes the next start fail with
 ; "The procedure entry point X could not be located in the dynamic link library
-; ...Qt6QuickWidgets.dll" or "The code execution cannot proceed because
-; Qt6Multimedia.dll was not found", so stop here instead of compounding it.
-; Nothing of the new payload has been written at this point, so the next run -
-; with the applications closed - gets a clean install.
+; ...Qt6QuickWidgets.dll", "The code execution cannot proceed because
+; Qt6Multimedia.dll was not found", or "abseil_dll.dll is either not designed to
+; run on Windows or it contains an error", so stop here instead of compounding
+; it. Nothing of the new payload has been written at this point, so the next run,
+; with the applications closed, gets a clean install.
 ${If} $IsUpgrade = 1
     Call HasStaleRuntimeFiles
+    !insertmacro CockLog "section: HasStaleRuntimeFiles=$R0"
 ${Else}
     StrCpy $R0 0
 ${EndIf}
 ${If} $R0 = 1
+    !insertmacro CockLog "section: aborting, files of a previous version survived the purge"
     DetailPrint "Files of a previous version could not be removed; aborting the installation."
     ; The in-app updater runs this installer silent and detached and never looks
     ; at its exit code, so the user has to be told what happened. A /S install
-    ; without /R is unattended - a package manager, for instance - and a message
+    ; without /R is unattended (a package manager, for instance) and a message
     ; box there would block forever, so that case only reports through the log
-    ; and the exit code.
-    ${IfNot} ${Silent}
-        MessageBox MB_OK|MB_ICONEXCLAMATION \
+    ; and the exit code. AbortInstall owns that decision.
+    StrCpy $AbortReason \
         "The installation could not remove all files of a previous version of Cockatrice, most likely because it is still running or was started as administrator, and was cancelled rather than leave a mismatched install behind.$\r$\n$\r$\nPlease close Cockatrice, Oracle and Servatrice and run the installer again."
-    ${EndIf}
-    SetErrorLevel 1
-    Abort
+    Call AbortInstall
 ${EndIf}
 
 @CPACK_NSIS_EXTRA_PREINSTALL_COMMANDS@
+!insertmacro CockLog "section: writing payload to $INSTDIR"
 @CPACK_NSIS_FULL_INSTALL@
+!insertmacro CockLog "section: payload written"
+!insertmacro CockLogListing "final" "Qt6*.dll"
 
 ${If} $PortableMode = 0
 	WriteUninstaller "$INSTDIR\uninstall.exe"
@@ -1126,6 +1246,7 @@ ${If} $ReinstallMode = 1
         Exec '"$INSTDIR\cockatrice.exe"'
 ${EndIf}
 
+!insertmacro CockLog "section: application install complete"
 SectionEnd
 
 Section "Start menu item" SecStartMenu

cockatrice/CMakeLists.txt

@@ -691,7 +691,7 @@ if(WIN32)
   # import table, and the glob above only sees DLLs that were already copied next
   # to the executable. Qt6::Concurrent is linked but never imported - QtConcurrent::run
   # and QFutureWatcher are header-only in Qt 6 - so neither step ships
-  # Qt6Concurrent.dll, even though required-qt-runtime.txt lists it and the
+  # Qt6Concurrent.dll, even though required-runtime.txt lists it and the
   # Windows package check then fails on a package that is fine to run.
   # Qt6::ShaderTools is skipped for the same reason.
   #