Fix Windows installer to use 64-bit Program Files#1299
Fix Windows installer to use 64-bit Program Files#1299alltheseas wants to merge 1 commit intodamus-io:masterfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughInstaller and CI changes: the Inno Setup script ( Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions Runner
participant ISCC as Inno Setup Compiler (ISCC)
participant Artifacts as Build Artifacts Store
participant Installer as Generated Installer
participant User as End User Installer
CI->>Artifacts: build binaries (per-arch)
CI->>ISCC: run ISCC /DBuildArch=matrix.arch with scripts/windows-installer.iss
ISCC->>Artifacts: read `ExeSource` (arch-specific) and `{`#PkgOutputDir`}`
ISCC-->>Installer: produce installer exe
User->>Installer: run installer
Installer->>User: install to `{autopf}\Notedeck` and set 64-bit mode
Installer->>User: optionally run `notedeck.exe` (Run entry)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
alltheseas
force-pushed
the
fix/windows-installer-64bit
branch
from
9a2f9d6 to
9c89bb7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/windows-installer.iss (2)
1-11: Add a stableAppIdto ensure correct upgrade/uninstall detection.Without an explicit
AppId, Inno Setup derives the install registry key fromAppName. A future rename of the app (e.g., branding change) would break upgrade detection and leave orphaned entries in Add/Remove Programs.♻️ Suggested addition
[Setup] AppName=Damus Notedeck +AppId={{XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX} AppVersion=0.1Generate a fresh GUID once (e.g., via PowerShell
[Guid]::NewGuid()) and keep it constant across all future releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/windows-installer.iss` around lines 1 - 11, Add an explicit stable AppId to the Inno Setup [Setup] section so upgrades/uninstalls are reliably detected: generate a single GUID (e.g., via PowerShell [Guid]::NewGuid()), add AppId={YOUR-GUID-HERE} to the setup entries in scripts/windows-installer.iss (alongside AppName, AppVersion, etc.), and commit that GUID permanently so future releases use the same AppId for correct upgrade/uninstall behavior.
3-3:AppVersionis hardcoded to0.1— consider driving it from the actual build version.Every installer produced by this script will report version
0.1in Add/Remove Programs, regardless of the actual notedeck release. Consider passing the version via the ISCC/Dflag from the CI/CD step that invokes the compiler (e.g.,iscc /DMyAppVersion=0.2.0 windows-installer.iss) and then referencing it here:♻️ Suggested change
-AppVersion=0.1 +AppVersion={`#MyAppVersion`}Then pass
/DMyAppVersion=<version>from the build script.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/windows-installer.iss` at line 3, Replace the hardcoded AppVersion value with a preprocessor symbol and use the ISCC /D flag to supply the real build version: change the literal AppVersion=0.1 to reference the preprocessor variable (AppVersion should use the {`#MyAppVersion`#} preprocessor symbol) and update your CI build step to call iscc with /DMyAppVersion=<version> so the installer picks up the actual release version at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/windows-installer.iss`:
- Around line 1-11: Add an explicit stable AppId to the Inno Setup [Setup]
section so upgrades/uninstalls are reliably detected: generate a single GUID
(e.g., via PowerShell [Guid]::NewGuid()), add AppId={YOUR-GUID-HERE} to the
setup entries in scripts/windows-installer.iss (alongside AppName, AppVersion,
etc.), and commit that GUID permanently so future releases use the same AppId
for correct upgrade/uninstall behavior.
- Line 3: Replace the hardcoded AppVersion value with a preprocessor symbol and
use the ISCC /D flag to supply the real build version: change the literal
AppVersion=0.1 to reference the preprocessor variable (AppVersion should use the
{`#MyAppVersion`#} preprocessor symbol) and update your CI build step to call iscc
with /DMyAppVersion=<version> so the installer picks up the actual release
version at compile time.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/rust.yml (1)
240-243:⚠️ Potential issue | 🟠 Major
Move-Itemwith identical source and destination is dead code—remove it.The ISS script is invoked with
/DBuildArch=${{ matrix.arch }}, which causes the preprocessor to setPkgOutputDirto..\packages\<arch>(seescripts/windows-installer.isslines 7, 18). The installer is therefore already written topackages\<arch>\DamusNotedeckInstaller.exe. The step at lines 240-243 attempts to move the file from that path to itself, which silently succeeds on Windows and masks any actual errors (e.g., if the ISS failed to create the output). TheNew-Itemis also unnecessary since ISS output creation to that directory is already part of the build.♻️ Proposed fix: remove the dead step
- # Move output - - name: Move Inno Script outputs to architecture-specific folder - run: | - New-Item -ItemType Directory -Force -Path packages\${{ matrix.arch }} - Move-Item -Path packages\${{ matrix.arch }}\DamusNotedeckInstaller.exe -Destination packages\${{ matrix.arch }}\DamusNotedeckInstaller.exe🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 240 - 243, Remove the entire workflow step named "Move Inno Script outputs to architecture-specific folder" (the block that runs New-Item and Move-Item) because Move-Item is a no-op here and New-Item is unnecessary; delete that step so the workflow relies on the ISS output already writing to packages\${{ matrix.arch }} (as configured via /DBuildArch and PkgOutputDir) and avoid masking failures when the installer isn't produced.
🧹 Nitpick comments (1)
.github/workflows/rust.yml (1)
211-212: Redundant "Build (Native Only)" step adds unnecessary CI time.The
cargo build --releasehere (no--target) outputs totarget/release/but the subsequent "Build" step at line 232 already builds with--target=${{ matrix.arch }}-pc-windows-msvc, which is the binary the ISS script consumes. For thex86_64matrix this duplicates effort; for theaarch64matrix it produces an x86_64 artifact that is never used.♻️ Proposed fix: remove the redundant step
- # Build - - name: Build (Native Only) - run: cargo build --release - # Create packages directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/rust.yml around lines 211 - 212, Remove the redundant CI step named "Build (Native Only)" that runs the command `cargo build --release`; this duplicates work and produces unused artifacts for non-x86_64 matrix entries—delete the step block so only the later "Build" step that uses `--target=${{ matrix.arch }}-pc-windows-msvc` remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/rust.yml:
- Line 237: The ISS currently hardcodes ArchitecturesAllowed=x64compatible and
ArchitecturesInstallIn64BitMode=x64compatible; modify
scripts\windows-installer.iss to conditionally set those two directives to arm64
when the ISPP BuildArch symbol equals "aarch64" (otherwise keep x64compatible).
Use ISPP conditional logic around the ArchitecturesAllowed and
ArchitecturesInstallIn64BitMode lines, checking BuildArch and emitting "arm64"
for aarch64 builds and "x64compatible" for others so native ARM64 installers run
in native 64-bit mode.
---
Outside diff comments:
In @.github/workflows/rust.yml:
- Around line 240-243: Remove the entire workflow step named "Move Inno Script
outputs to architecture-specific folder" (the block that runs New-Item and
Move-Item) because Move-Item is a no-op here and New-Item is unnecessary; delete
that step so the workflow relies on the ISS output already writing to
packages\${{ matrix.arch }} (as configured via /DBuildArch and PkgOutputDir) and
avoid masking failures when the installer isn't produced.
---
Nitpick comments:
In @.github/workflows/rust.yml:
- Around line 211-212: Remove the redundant CI step named "Build (Native Only)"
that runs the command `cargo build --release`; this duplicates work and produces
unused artifacts for non-x86_64 matrix entries—delete the step block so only the
later "Build" step that uses `--target=${{ matrix.arch }}-pc-windows-msvc`
remains.
The Inno Setup installer used {pf} which resolves to Program Files
(x86), causing CreateProcess error 216 (architecture mismatch) when
launching the 64-bit notedeck.exe.
- Switch to {autopf} and add ArchitecturesAllowed/InstallIn64BitMode
- Conditionally emit arm64 directives for aarch64 builds
- Remove duplicated inline .iss generation from CI workflow; use the
shared scripts/windows-installer.iss via /DBuildArch instead
- Remove redundant untargeted cargo build step
- Remove no-op Move-Item step (OutputDir already writes to packages\arch)
Closes damus-io#1298
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alltheseas
force-pushed
the
fix/windows-installer-64bit
branch
from
9c89bb7 to
cac9b75
Compare
1 participant
Summary
{pf}(32-bit Program Files) instead of{autopf}(auto-detecting 64-bit)ArchitecturesAllowed=x64compatibleandArchitecturesInstallIn64BitMode=x64compatibleto ensure proper 64-bit installationCloses #1298
Test plan
C:\Program Files\Notedeckon 64-bit Windows🤖 Generated with Claude Code
Summary by CodeRabbit