build: Improve Windows uninstaller #16769
Conversation
|
We talked about the uninstall.exe icon here: #16760 The uninstall.exe icon looks like it's from 2010 (horrible) and the previous PR didn't really change that - it was more of a quick fix, but only for newer versions of Windows (maybe only Windows 10 and 8). Current uninstall.exe icon / New uninstall icon:
|
|
You can split this into two commits, each with a more descriptive commit message. i.e One for the registry modifications, with an explanation of the values being removed, and one for updating the uninstaller icon. |
|
@fanquake got it, done. |
|
@GChuf you are missing a double quote:
|
|
@GChuf the registry keys on my machine under HKCU and HKLM are both |
|
@sipsorcery nice catch on double quotes, thank you. Which windows version are you using? Keys on my Win10 (1709) and my Windows 7 (I just tested) appear just how I've set to delete them. This is from my Win7 machine after uninstall: In any case, we can simply add a line to delete |
|
@GChuf I think we should only delete registry keys in the uninstall script that are explicitly set in the install script. If there is somewhere in the Bitcoin app that is setting an additional registry key then IMHO we should add it to the install script. It seems a bit dangerous to simply delete keys that use the word Bitcoin. There are some other notable products that may have very similar names. |
|
Well the registry keys deleted don't just contain the word "bitcoin", they're exactly specified. If there is no key named Other products would be wise to avoid naming registry keys (or anything else) exactly The fact is that the installer generates these keys and does not delete them, unlike other keys. Also, you haven't answered my question earlier - what windows version are you using, and what bitcoin version? |
|
Windows 10 Pro Insider Preview Build 18956.rs_prerelease.190803-1414.
This delete command from your PR is just "Bitcoin".
With some more testing I've found that the The other delete key in the PR wasn't needed in my testing:
The existing installer (bitcoin-0.18.1-win64-setup.exe from https://bitcoin.org/en/download) already deletes that key as a result of:
|
|
Whatever is specified, that exact thing will be deleted. Be it "Bitcoin" or "Bitcoin Core (64-bit)". In the case of I have to agree on leaving As for I also tested it manually with deleting registry keys in HKCU - HKLM was left untouched. |
|
Updated |
|
Next suggestion now is to follow the existing convention in the file for specifying the keys to delete. Instead of:
it should be:
That way if the name of the installer package ever changes it will automatically get applied without needing the additional step of also changing the on this step. I think it's also worth adding /IfEmpty as a precaution. On my machine there are no subkeys added under |
|
Agreed. No subkeys on my machine as well. Do you think we could also remove these entries which delete values? These values should automatically be deleted when deleting keys anyway: |
No they should be left as is. The original author has decided that it's safe to explicitly delete those sub keys no matter what they contain. If the commands are removed and the keys do exist then the command below will fail due to the
|
|
It is true that the command will fail if there are subkeys. However these are values (!) being deleted, not subkeys. Therefore, deleting values and keys afterwards is doing the same thing twice. |
|
@sipsorcery @fanquake |
|
@GChuf I can confirm the Windows installer builds correctly on an Ubuntu 18.04 VM using those instructions. What OS were you using? |
|
@sipsorcery using Ubuntu 18.04 VM as well. I get some .exe files when the process is finished, but bitcoin-qt.exe appears to be portable install.
|
|
The bitcoin-qt.exe isn't an installer. That's solely the GUI application. It's big because it's a static build and pulls in the all Qt dependencies. Did you definitely do the |
|
@sipsorcery thank you. I was missing I've made sure that deleting registry values is useless since registry keys are deleted anyway. And this is how the uninstaller looks with the new icon: |
Removes 3 lines where deleting registry values is not needed (they are deleted when the corresponding registry keys are deleted) Note: /IfEmpty is looking for subkeys, not values.
|
The icon change looks ok to me but removing the delete of the registry keys isn't justified. Those instructions may not have an effect on yours or my Win10 systems but maybe they do on Win7 or Vista, XP etc. General rule of thumb with Bitcoin Core changes is that the impacts need to be very well understood or they shouldn't be applied. Not saying it would have to go as far back as Windows 3.1 but at least the last 3 or 4 versions. IMHO the main benefit of this PR is the better looking icon. Why not leave the change at that? |
|
Let's clear some things out.
|
I agree. This PR could be just that change. I don't have experience working with the Windows registry or unisntall process, however I think we're getting bogged down into a change where, if we leave everything as is, in the worse case a registry key that doesn't exist doesn't get deleted? Will defer to others that have an opinion. |
| @@ -21,7 +21,7 @@ SetCompressor /SOLID lzma | |||
| !define MUI_STARTMENUPAGE_DEFAULTFOLDER "@PACKAGE_NAME@" | |||
| !define MUI_FINISHPAGE_RUN "$WINDIR\explorer.exe" | |||
| !define MUI_FINISHPAGE_RUN_PARAMETERS $INSTDIR\@BITCOIN_GUI_NAME@@EXEEXT@ | |||
| !define MUI_UNICON "${NSISDIR}\Contrib\Graphics\Icons\modern-uninstall.ico" | |||
| !define MUI_UNICON "@abs_top_srcdir@/share/pixmaps/bitcoin-black.ico" | |||
There was a problem hiding this comment.
Where is this file from and how was it created?
There was a problem hiding this comment.
This icon file was created by me with the help of Adobe Photoshop and another program for making .ico files on Windows (Axialis IconWorkshop).
The original file I used was this: https://raw.githubusercontent.com/bitcoin/bitcoin/0.18/src/qt/res/icons/about.png
I'm not sure how you got here. The result will be the same if we make the change or not. I simply deleted unnecessary lines and made the uninstall process cleaner. So, this is an optimization. I believe I explained everything in my previous comment (and commit message), but to hopefully make it clearer: If registry keys (which contain values) are deleted, those values are automatically deleted as well. I've worked with windows registry on various occasions and I've tested the uninstaller after these changes. If there are any other people you know about who worked with windows and the registry extensively, you can mention them here. |
Except if the The existing logic that's relevant here seems pretty clear to me: Where the install is on Windows 64bit:
This PR makes changes to that logic and even though the changes are very trivial I can't see any justification. NACK from me. |
|
@sipsorcery I've explained 2 times already that /IfEmpty flag looks for subkeys not values. I don't know what else to say. Read the NSIS link again. |
Yes but what about the case where there are subkeys that exist under |
I don't understand where you're getting at. First, do we agree that deleting registry values specifically is not needed in the script? Have we established that Second, what do you mean by desirable? The |
|
@NicolasDorier would you be willing to take another look? In particular, the registry values? We've already talked about the icon in the previous PR if you remember. |
|
Uh, won't this change the Start Menu icon for the uninstaller? People looking through their Start Menu to run Bitcoin don't want to get the icons confused IMO. Having the icon itself reflect uninstallation is therefore important. |
|
@luke-jr yes, it will change the start menu icon, but since for most users the first icon will be |
This is a very good point why the icon for uninstallation needs to be qualitatively different and not just, say, the bitcoin icon in a different color. |
laanwj
changed the title
|
Closing this as it seems controversial. |
Adds an icon for uninstall.exe
Deletes some registry values that were otherwise left in the windows registry after uninstallDeletes some code (deleting registry values) which was unnecessary