Installer Improvements - #678
Open
dpaulat wants to merge 7 commits into
Open
Conversation
Removing the shortcut causes pins based on the shortcut to break
The bootstrapper will combine the VC redist and .msi installation packages.
Closed
📝 WalkthroughWalkthroughThe Windows packaging flow now builds an NSIS bootstrapper around the MSI, installs the VC++ redistributable separately, adjusts MSI upgrade behavior, and uploads both unsigned and signed bootstrap executables through CI signing workflows. ChangesWindows packaging and installer delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant CPack
participant BootstrapBuild as Bootstrapper build script
participant SignPath
participant ArtifactStore as Artifact storage
CI->>CPack: Build Windows MSI
CI->>BootstrapBuild: Build NSIS bootstrapper
BootstrapBuild-->>CI: Produce unsigned EXE
CI->>ArtifactStore: Upload unsigned EXE
CI->>SignPath: Submit EXE for signing
SignPath-->>CI: Return signed EXE
CI->>ArtifactStore: Upload signed EXE
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scwx-qt/nsis/supercell-wx.nsi`:
- Around line 114-120: Update Bootstrapper_StripTrailingSlash to preserve
drive-root paths: when $INSTDIR matches a drive root such as C:\, normalize it
to an equivalent absolute form like C:\. instead of removing the trailing
backslash. Continue stripping trailing backslashes for non-root install paths
before constructing the msiexec command.
In `@scwx-qt/wix.template.in`:
- Around line 49-53: Update the installer upgrade handling around
RemoveShortcuts and RemoveExistingProducts so upgrades from existing releases do
not remove the prior shortcut component; use a migration strategy that preserves
the old shortcuts, or explicitly document and mitigate the unavoidable one-time
pin break during the first bootstrapper migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5927b1a9-857a-42be-976f-93b5badec89d
⛔ Files ignored due to path filters (2)
scwx-qt/nsis/scwx-header.bmpis excluded by!**/*.bmpscwx-qt/nsis/scwx-welcome.bmpis excluded by!**/*.bmp
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/sign-windows-packages.ymldatascwx-qt/nsis/supercell-wx.nsiscwx-qt/scwx-qt.cmakescwx-qt/wix.template.intools/build-windows-nsis-bootstrapper.ps1
Comment on lines
+114
to
+120
| Function Bootstrapper_StripTrailingSlash | ||
| ; Ensures INSTALL_ROOT="..." is not broken by a trailing backslash escape. | ||
| StrCpy $R8 $INSTDIR 1 -1 | ||
| ${If} $R8 == "\" | ||
| StrCpy $INSTDIR $INSTDIR -1 | ||
| ${EndIf} | ||
| FunctionEnd |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve drive-root install paths.
Selecting C:\ changes $INSTDIR to C:, so INSTALL_ROOT becomes drive-relative and can install into an unintended directory. Handle drive roots separately—for example, normalize C:\ to C:\. before constructing the msiexec command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scwx-qt/nsis/supercell-wx.nsi` around lines 114 - 120, Update
Bootstrapper_StripTrailingSlash to preserve drive-root paths: when $INSTDIR
matches a drive root such as C:\, normalize it to an equivalent absolute form
like C:\. instead of removing the trailing backslash. Continue stripping
trailing backslashes for non-root install paths before constructing the msiexec
command.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add NSIS bootstrapper that installs both the Visual C++ Redistributable and .msi.
Additional improvements:
Subsequent updates will migrate the auto-updater to use the NSIS bootstrapper instead of the .msi. The .msi will be retained for compatibility for the foreseeable future.