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.

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

build(nsis): Write registry keys to HKLM instead of HKCU, Install shortcuts for all users, Fix INSTALLDIR removal bug #2643

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

sitiom
Copy link
Contributor

@sitiom sitiom commented Feb 27, 2023

This is the proper root key to write registries for machine-wide installation. Winget was having problems due to this inconsistency: microsoft/winget-pkgs#47655

@jamescowens
Copy link
Member

jamescowens commented Feb 27, 2023

Thanks! :)

UT. Concept ACK. If you are going to change the key root, you will need to handle the removal of stale keys in the old location upon install. Your current PR will not do that.

This will also produce a requirement for user elevation for the install I think. Gridcoin itself is not to be run under elevated privileges.

@sitiom
Copy link
Contributor Author

sitiom commented Feb 27, 2023

This will also produce a requirement for user elevation for the install I think. Gridcoin itself is not to be run under elevated privileges.

The current installer does require user elevation (it installs to Program Files). See the UAC icon:
image

@jamescowens
Copy link
Member

Agreed. Still need to deal with removing the old stale entries leftover from the old install on an overlay install using the new installer. You also should think about the downgrade case in case someone temporarily downgrades to make sure the extra keys in HKLM (from the perspective of the old install) don't cause a problem. There are user specific settings written to the registry regarding Window Size, etc that should be maintained in HKCU.

@sitiom
Copy link
Contributor Author

sitiom commented Feb 27, 2023

I think the easiest solution would be to run the uninstaller first before installing the new version to remove old user entries and shortcuts.

@jamescowens
Copy link
Member

You can't depend on people doing that. You would have to put a check to not allow the installer to run if the uninstaller had not run before. You would also have to ensure people that the data directory would not be affected. Many people can get confused between the data directory and the program executable directory.

@jamescowens
Copy link
Member

BTW, Gridcoin has its own internal update check. Not sure why we need to get an external one to work.

@sitiom
Copy link
Contributor Author

sitiom commented Feb 27, 2023

You can't depend on people doing that. You would have to put a check to not allow the installer to run if the uninstaller had not run before.

This installer can also prompt to uninstall first if it detects the old HKCU entry; that would be more convenient.

BTW, Gridcoin has its own internal update check. Not sure why we need to get an external one to work.

Because of the installation inconsistency, since the current installer writes to HKCU, other users will not be able to see the start menu shortcuts and installation entry in the Control Panel despite it being installed machine-wide. That's another problem that needs to be solved.

@sitiom
Copy link
Contributor Author

sitiom commented Feb 27, 2023

And, oh yeah. I found a bug in the installer. I was confused why my installation directory disappeared when I hadn't done anything yet, so I found out that just running the installer from the start will delete the installation directory due to these lines of code:

rununin:
Exec $INSTDIR\uninst.exe
Delete $INSTDIR\*"

Also, the uninst.exe line doesn't do anything, as the uninstaller file is uninstaller.exe. I'll also attempt to fix this in this PR.

@sitiom sitiom marked this pull request as draft February 27, 2023 04:14
@sitiom sitiom changed the title build(nsis): Write registry keys to HKLM instead of HKCU build(nsis): Write registry keys to HKLM instead of HKCU, install shortcut for all users Feb 27, 2023
@sitiom sitiom marked this pull request as ready for review February 27, 2023 10:21
@sitiom
Copy link
Contributor Author

sitiom commented Feb 27, 2023

This installer can also prompt to uninstall first if it detects the old HKCU entry; that would be more convenient.

Done. Fixed the bug mentioned in #2643 (comment) as well. If the HKCU keys are found, it will prompt the user to uninstall first:
image

Installation will immediately proceed afterwards after a successful uninstallation. Otherwise, it will abort. Silent installation using the /S switch also works seamlessly.

@sitiom sitiom changed the title build(nsis): Write registry keys to HKLM instead of HKCU, install shortcut for all users build(nsis): Write registry keys to HKLM instead of HKCU, install shortcuts for all users Feb 27, 2023
@sitiom sitiom changed the title build(nsis): Write registry keys to HKLM instead of HKCU, install shortcuts for all users build(nsis): Write registry keys to HKLM instead of HKCU, Install shortcuts for all users, Fix INSTALLDIR removal bug Feb 27, 2023
@jamescowens
Copy link
Member

Very good. I will look at this closely today! :)

@jamescowens jamescowens self-requested a review February 28, 2023 20:54
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

tACK except for the changes I asked for in the dialog box. Good work.

share/setup.nsi.in Outdated Show resolved Hide resolved
@jamescowens jamescowens added this to the LaVerne milestone Feb 28, 2023
Copy link
Member

@jamescowens jamescowens left a comment

Choose a reason for hiding this comment

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

tACK.

@jamescowens jamescowens merged commit 3afee72 into gridcoin-community:development Mar 2, 2023
@sitiom sitiom deleted the patch-1 branch March 2, 2023 03:20
jamescowens added a commit to jamescowens/Gridcoin-Research that referenced this pull request Mar 26, 2023
Added
 - key, wallet: HD wallets gridcoin-community#2540 (@div72)
 - ARMv8 SHA2 Intrinsics gridcoin-community#2612 (@barton2526)
 - build: vendor bdb 5.3 gridcoin-community#2620 (@div72)
 - scraper, gui: Add external adapter projects indication gridcoin-community#2625 (@jamescowens)
 - gui: implement INSUFFICIENT_MATURE_FUNDS status for the mrcmodel gridcoin-community#2628 (@jamescowens)
 - gui, accrual: Implement accrual limit warning gridcoin-community#2636 (@jamescowens)
 - rpc: add `getnodeaddresses` gridcoin-community#2646 (@Pythonix)
 - consensus: Add new checkpoints gridcoin-community#2651 (@barton2526)

Changed
 - voting: Optimize poll locks gridcoin-community#2619 (@jamescowens)
 - util: move threadinterrupt.{cpp,h} to util gridcoin-community#2613 (@Pythonix)
 - gui, voting: Update pool cpids and avw rules gridcoin-community#2624 (@jamescowens)
 - ci: bump python and setup-python action version gridcoin-community#2626 (@div72)
 - gui: Change text from username to name (real name or nickname) gridcoin-community#2633 (@jamescowens)
 - locale: Translation update, phase 1 gridcoin-community#2637 (@jamescowens)
 - gui: Change MRC too soon to submit error to be less confusing gridcoin-community#2645 (@jamescowens)
 - locale: Update translations prior to release (phase 2/2) gridcoin-community#2658 (@jamescowens)
 - gui: Enhance MRC request form to avoid fee boost field confusion gridcoin-community#2659 (@jamescowens)

Removed
none

Fixed
 - net: Turn net structures into dumb storage classes (backport) gridcoin-community#2561 (@Pythonix)
 - build: Include native_X.mk before X.mk gridcoin-community#2609 (@barton2526)
 - depends: fix OpenSSL for Darwin builds gridcoin-community#2610 (@div72)
 - build: Change actions runner image to Focal, Force Lint to use 22.04, Change cd runner version gridcoin-community#2611 (@barton2526)
 - gui: don't show datadir error msgbox if arg isn't specified gridcoin-community#2617 (@div72)
 - rpc: Repair auditsnapshotaccrual rpc function gridcoin-community#2621 (@jamescowens)
 - gui: Correct updateBeaconIcon() function in bitcoingui.cpp gridcoin-community#2622 (@jamescowens)
 - wallet: Strengthen CWalletTx::RevalidateTransactions gridcoin-community#2627 (@jamescowens)
 - test: Fix Wambiguous-reversed-operator compiler warning, drop boost::assign gridcoin-community#2632 (@barton2526)
 - gui: Fix wallet overview displaying lower-case poll name gridcoin-community#2640 (@delta1513)
 - Fix and optimize ResendWalletTransactions gridcoin-community#2642 (@jamescowens)
 - build(nsis): Write registry keys to HKLM instead of HKCU, Install shortcuts for all users, Fix INSTALLDIR removal bug gridcoin-community#2643 (@sitiom)
 - gui: Fix TransactionRecord::decomposeTransaction to properly display self-sidestake gridcoin-community#2647 (@jamescowens)
 - rpc: Fixed the RPC error when running `help voting` while syncing gridcoin-community#2649 (@delta1513)
 - build: Fix compilation with GCC 13 gridcoin-community#2653 (@theMarix)
 - rpc: Formatting - typo correction rpc help for listresearcheraccounts gridcoin-community#2654 (@PrestackI)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants