Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compare registry values to $INSTDIR before removing keys #684

Merged
merged 6 commits into from
Jun 27, 2023
Merged

Compare registry values to $INSTDIR before removing keys #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2171748
Find registry keys for uninstaller based on INSTDIR
marcoesters Jun 15, 2023
a80b364
Add news
marcoesters Jun 15, 2023
fa2040b
Remove stray EndIf
marcoesters Jun 15, 2023
e6b0ce9
Update news/684-uninstaller-check-registry
marcoesters Jun 16, 2023
58a7b89
Merge branch 'main' into uninstaller-registry
jaimergp Jun 21, 2023
009f843
Merge branch 'main' into uninstaller-registry
marcoesters Jun 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions constructor/nsis/main.nsi.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ Unicode "true"
\Uninstall\${UNINSTALL_NAME}"

var /global INSTDIR_JUSTME
var /global INSTALLER_VERSION
var /global INSTALLER_NAME_FULL

# UAC shield overlay
!ifndef BCM_SETSHIELD
Expand Down Expand Up @@ -1201,10 +1203,36 @@ Section "Uninstall"
# carefully. More info at https://docs.conda.io/projects/conda/en/latest/user-guide/troubleshooting.html#solution
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("CONDA_DLL_SEARCH_MODIFICATION_ENABLE", "1").r0'

# Read variables the uninstaller needs from the registry
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, this block will search the registry for two variables:

  • INSTALLER_NAME_FULL
  • INSTALLER_VERSION

These were previously assumed to be constant and would have used the hardcoded values at install time (${NAME} [used in line 65] and ${VERSION}, respectively).

However we are now saying that this might not be always the case. Could you elaborate under what circumstances this is true? e.g. updating Anaconda in place but not updating the registry keys (or not deleting the old ones before adding the new ones)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this logic allows the uninstaller to run for other conda installations if needed, I don't know if that's a bug or a feature 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this logic allows the uninstaller to run for other conda installations if needed, I don't know if that's a bug or a feature 😬

That is already possible. The changes in this PR will, however, conduct checks to make sure the deletion is performed consistently on the file system and the registry.

There is another, admittedly nice, application: if you install the same Miniconda/Miniforge version twice, installation 2 will overwrite the registry keys of number 1. If you then run the uninstaller of number 1, it will delete the keys of number 2.

StrCpy $R0 "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall"
StrLen $R1 "Uninstall-${NAME}.exe"
IntOp $R1 $R1 + 3
StrCpy $0 0
loop_path:
EnumRegKey $1 SHCTX $R0 $0
StrCmp $1 "" endloop_path
StrCpy $2 "$R0\$1"
ReadRegStr $4 SHCTX $2 "UninstallString"
StrLen $5 $4
IntOp $5 $5 - $R1
StrCpy $4 $4 $5 1
${If} $4 == $INSTDIR
StrCpy $INSTALLER_NAME_FULL $1
ReadRegStr $INSTALLER_VERSION SHCTX $2 "DisplayVersion"
goto endloop_path
${EndIf}
IntOp $0 $0 + 1
goto loop_path
endloop_path:

# Extra info for pre_uninstall scripts
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("PREFIX", "$INSTDIR").r0'
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("INSTALLER_NAME", "${NAME}").r0'
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("INSTALLER_VER", "${VERSION}").r0'
StrCpy $0 ${VERSION}
${If} $INSTALLER_VERSION != ""
StrCpy $0 $INSTALLER_VERSION
${EndIf}
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("INSTALLER_VER", "$0").r0'
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("INSTALLER_PLAT", "${PLATFORM}").r0'
System::Call 'kernel32::SetEnvironmentVariable(t,t)i("INSTALLER_TYPE", "EXE").r0'

Expand All @@ -1218,13 +1246,26 @@ Section "Uninstall"
# In case the last command fails, run the slow method to remove leftover
RMDir /r /REBOOTOK "$INSTDIR"

DeleteRegKey SHCTX "${UNINSTREG}"
${If} $INSTALLER_NAME_FULL != ""
DeleteRegKey SHCTX "SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\$INSTALLER_NAME_FULL"
${EndIf}

# If Anaconda was registered as the official Python for this version,
# remove it from the registry
ReadRegStr $0 SHCTX "Software\Python\PythonCore\${PY_VER}\InstallPath" ""
${If} $0 == "$INSTDIR"
DeleteRegKey SHCTX "Software\Python\PythonCore\${PY_VER}"
${EndIf}
StrCpy $R0 "SOFTWARE\Python\PythonCore"
Copy link
Contributor

Choose a reason for hiding this comment

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

This block is easier to understand, I think. Basically we don't want to assume that the base Python is still the same version. However, is this updated when conda updates the python package in base? I don't remember seeing code for this 🤔 It would still be the same path, so it would work, but maybe the metadata is outdated when it comes to the reported version in the registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also prevents issues in cross-installer usage, as described in my other comment.

I do not think that conda updates the registry when python is updated, which it should probably do.

StrCpy $0 0
loop_py:
EnumRegKey $1 SHCTX $R0 $0
StrCmp $1 "" endloop_py
ReadRegStr $2 SHCTX "$R0\$1\InstallPath" ""
${If} $2 == $INSTDIR
StrCpy $R1 $1
DeleteRegKey SHCTX "$R0\$1"
goto endloop_py
${EndIf}
IntOp $0 $0 + 1
goto loop_py
endloop_py:
SectionEnd

!if '@SIGNTOOL_COMMAND@' != ''
Expand Down
19 changes: 19 additions & 0 deletions news/684-uninstaller-check-registry
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
### Enhancements

* The Windows uninstaller will check the registry for `$INSTDIR` before deleting hardcoded registry keys (#684)

### Bug fixes

* <news item>

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>