Skip to content

Fix: Windows installer now deletes files and cleans up PATH environment var#3066

Open
mwinkel-dev wants to merge 5 commits into
MDSplus:alphafrom
mwinkel-dev:mw-3063-win-nsis
Open

Fix: Windows installer now deletes files and cleans up PATH environment var#3066
mwinkel-dev wants to merge 5 commits into
MDSplus:alphafrom
mwinkel-dev:mw-3063-win-nsis

Conversation

@mwinkel-dev

@mwinkel-dev mwinkel-dev commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

This fixes Issue #3063.

  • On uninstall, $MDSPLUS_DIR/bin is now removed from PATH regardless of whether installed for AllUsers or the CurrentUser.
  • MDSplus files were not being deleted because recycle.nsh was broken, so removed it.
  • Now deletes the MDSplus files by using the usual Nullsoft statements.
  • Revised logic to guard against deletion of system directories. (If user intended to install to C:\Program Files\MDSplus but made a typo and instead installed to C:\Program Files don't want to do deletion because it would clobber all programs installed on the computer. Now, deletion only occurs if the directory's path ends in MDSplus.)
  • The CMake PR 2527 changed the Windows installer so 32-bit and 64-bit are installed separately. That change rendered some installation and uninstallation code obsolete, thus it was removed.
  • Fixed various typos (mostly in comments).
  • Confirmed that the installer has a silent install mode for both AllUsers and CurrentUser.
  • Added some header comments to help the next maintenance developer.

@mwinkel-dev

Copy link
Copy Markdown
Contributor Author

The installation instructions in ReadTheDocs should mention the silent installation commands. These commands are useful when sysadmins are writing scripts to install MDSplus.

  • <installer_exe> /S /AllUsers
  • <installer_exe> /S /CurrentUser

@mwinkel-dev mwinkel-dev self-assigned this Jul 8, 2026
@mwinkel-dev mwinkel-dev added packaging Relates to packaging and distribution os/windows This is present on or relates to Windows labels Jul 8, 2026
@mwinkel-dev

Copy link
Copy Markdown
Contributor Author

Retest this please

@WhoBrokeTheBuild

Copy link
Copy Markdown
Member

What if a user installs it to C:\Program Files\MDSplus-1-2-3 ?

@mwinkel-dev

mwinkel-dev commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

In that ;case, the uninstaller does unregister MDSplus. But the user will have to manually delete the MDSplus-1-2-3 directory. Nonetheless, the uninstaller does delete the standard directories, C:\Program Files\MDSplus and C:\Program Files (x86)\MDSplus.

The above policy is of course a compromise.

  • It is better than the status quo which never deletes "C:\Program Files\MDSplus". That is because recycle.nsh is broken (and probably has been broken for years). So customers are already manually deleting MDSplus after an uninstall.
  • It ensures that the uninstaller will never delete a system directory.
  • On a Windows system, I think it unlikely that users will install to non-standard directories (e.g., they won't have multiple versions of MDSplus installed).
  • Usually, the uninstaller runs in "silent" mode, however there are some scenarios that cause a GUI to be displayed. The uninstaller GUI allows the user to change the path. Although it is unlikely that a user would do so, if they did and made a typo, they could end up deleting a directory completely unrelated to MDSplus.
  • From a support standpoint, it is easier for us to 1) help a customer delete MDSplus from a non-standard directory than it is to 2) help them restore system software that got nuked because the uninstaller was too trusting.

The only robust solution is to generate a log file during installation of all files that were installed. Then the uninstaller only deletes files that are listed in that log. There are Nullsoft users that have posted macros that do that, but it would add a lot of code to our installer. (Note that prior to the CMake PR 2527, that MDSplus was being installed in system directories and the installer did have code to create a log file. But that code was rendered obsolete by PR 2527, which is one of the reasons the uninstaller stopped working. For the details, refer to the commented code in the first commit of this PR.)

@mwinkel-dev

Copy link
Copy Markdown
Contributor Author

There is an alternate approach for dealing with non-standard installation directories (such as MDSplus-1-2-3).

  1. If we assume that the DirectoryLeave function (around line 120) never fails then MDSplus will always be installed in a new directory. That directory is of course recorded in the Windows registry.
  2. The uninstaller GUI could be changed so it does not allow the user to change the path, but instead forces the uninstaller to only use the MDSplus registry value.
  3. If the previous two conditions are met, then it would be safe for the uninstaller to delete non-standard directories.

The above approach isn't as robust as using a log of installed files to drive the uninstaller. However, the above approach is more flexible than the compromise taken with this PR.

@mwinkel-dev

Copy link
Copy Markdown
Contributor Author

VSCode has an NSIS extension that is nice. The extension allows collapsing code blocks (macros, functions, if clauses, etcetera), thus makes it much easier to understand the overall architecture of the installer. Plus, it has all the other usual features: keywords in color, keyword completion and so forth.

Perhaps the Developer Tips header comment of this PR should mention the NSIS extension.

@mwinkel-dev

Copy link
Copy Markdown
Contributor Author

Here are some macros that manage NSIS uninstall lists.

Another possibility is to write a custom solution (i.e., something similar to what the MDSplus installer had in the past).

If we decide to use a log file to drive the uninstaller, my preference is to use a third-party solution if any are suitable. To be suitable, it needs to be widely used, maintained, full-featured, robust and not be quarantined by Windows Defender. If none of the available third-party solutions meet those criteria, then as a contingency plan we should write a custom solution tailored to our needs.

@WhoBrokeTheBuild WhoBrokeTheBuild left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, however I do think we should use an explicit list of files during uninstall if possible. If not, then I'll approve this PR. Great work!

Comment thread deploy/packaging/windows/mdsplus.nsi Outdated
Var /GLOBAL InstBaseName
${GetBaseName} $INSTDIR $InstBaseName
${IfNot} "$InstBaseName" == ${MULTIUSER_INSTALLMODE_INSTDIR}
${ToLog} "Non-standard dir, so MDPlus was unregistered but not deleted."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MDPlus spelling

Comment thread deploy/packaging/windows/mdsplus.nsi Outdated
; MessageBox MB_OKCANCEL "Non-standard dir, so MDPlus was unregistered but not deleted." /SD IDOK IDOK next
IfSilent next gui ; Note that ${IfSilent} is not available in NSIS v3.09
gui:
MessageBox MB_OK "Non-standard dir, so MDPlus was unregistered but not deleted." IDOK next

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MDPlus spelling

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

os/windows This is present on or relates to Windows packaging Relates to packaging and distribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants