-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix: improve windows installer and relocate backrest on Windows to %localappdata%\programs #568
Conversation
Various fixes before decisions are made regarding multi-purpose installer (garethgeorge#549) 1. Changed installation location to user %localappdata%\programs since the installer already installed all icons and made registry entry only for the current user. 2. Fixed blurry installer text on high DPI screens. 3. Fixed icon for Start Menu, Desktop shortcuts, and Add/Remove programs entry. 4. Fixed uninstaller not terminating running application and not fully cleaning up installed files. Users upgrading from the previous installer should uninstall it first to avoid conflicts. User configuration will stay intact.
CreateShortCut "$SMPROGRAMS\$SM_Folder\Uninstall ${APP_NAME}.lnk" "$INSTDIR\uninstall.exe" "" "${BUILD_DIR}\icon.ico" 0 | ||
CreateShortCut "$SMPROGRAMS\$SM_Folder\${APP_NAME}.lnk" "$INSTDIR\${MAIN_APP_EXE}" "" "$INSTDIR\icon.ico" 0 | ||
CreateShortCut "$DESKTOP\${APP_NAME}.lnk" "$INSTDIR\${MAIN_APP_EXE}" "" "$INSTDIR\icon.ico" 0 | ||
CreateShortCut "$DESKTOP\${APP_NAME} UI.lnk" "http://localhost:9898/" "" "$INSTDIR\icon.ico" 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat!
@@ -3,7 +3,7 @@ | |||
!define APP_NAME "Backrest" | |||
!define COMP_NAME "garethgeorge" | |||
!define WEB_SITE "https://github.com/garethgeorge/backrest" | |||
!define VERSION "00.00.00.00" | |||
!define VERSION "1.6.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this will actually be out of date immediately with the next version, the first version of this installer will ship as 1.6.2.
I'll merge as is since it's not really a blocker, but I think I need to followup up re: automatically passing in the version from the release workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out NSIS is quite flexible and already had such a scenario covered. I will submit a new PR to have the installer grab the version from the changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat! Is that a built in capability of NSIS? My original thinking had been to stamp the version based on the git tag’s on the current commit — the release workflow tags a release commit with the current version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, built-in functionality. Given the format and structure of changelog.md, this should work well.
Test failure is a flake -- broadly LGTM. Thanks for contributing this! Per the discussion on the bug I think your recommendation of going with a single user install makes a lot of sense. It probably also has fewest footguns. Overall quite excited to see backrest's packaging on windows improved, and you're motivating me to look a bit more at a few related windows issues :). |
Just a few additional comments on the installer. When releasing 1.6.2, you might want to add a note for the users regarding the correct upgrade process. Because the current installer doesn't stop Backrest, it doesn't fully remove it, and also leaves behind Program Files\restic, install.lock, and desktop shortcuts. The cleanest way to upgrade would be:
It still won't clean up the above items, but at least there won't be a situation with a second Backrest instance launching upon new version installation. Even if a user doesn't follow the process above, the new installer will update the startup shortcut, so after a reboot it will be good. Except for the remnants from the old installer, which won't interfere at that point. I do need to add process "check and stop" function to avoid multiple processes issue with future upgrades. I was going to look deeper into the upgrade process going forward. It will probably be a week before I have time to work on it. |
Various fixes before decisions are made regarding multi-purpose installer (#549)
Users upgrading from the previous installer should uninstall it first to avoid conflicts. User configuration will stay intact.