fix(install): resolve Windows arch from env, not OSArchitecture#276
Conversation
The previous code read [RuntimeInformation]::OSArchitecture, which can be absent on the type PowerShell resolves in Windows PowerShell 5.1: a shadowing assembly such as PSReadLine may define its own RuntimeInformation without that property. Under Set-StrictMode the missing-property read becomes a terminating PropertyNotFoundStrict error, so `irm https://cli.oomol.com/install.ps1 | iex` fails. Detect the architecture from PROCESSOR_ARCHITEW6432 (falling back to PROCESSOR_ARCHITECTURE) instead. These work on every PowerShell version and report the true host architecture, including the WOW64 case where the inbox 32-bit PowerShell 5.1 on ARM64 resolves to arm64. The switch gains an amd64 case because that is the token PROCESSOR_ARCHITECTURE reports on x64 Windows. Verified on real Windows PowerShell 5.1 and PowerShell 7.6. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThis PR refactors Windows architecture detection in the installation script by replacing .NET runtime APIs with direct environment variable reads. The 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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: 1
🤖 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 `@contrib/install/install-ps1.test.ts`:
- Around line 287-303: The test is nondeterministic because runPowerShellCommand
forwards process.env including override vars like OO_INSTALL_PLATFORM that can
short-circuit Resolve-Platform; fix runPowerShellCommand by creating a shallow
copy of process.env, delete/clear OO_INSTALL_PLATFORM (and any other install
override variables you want neutralized), and pass that sanitized env object to
Bun.spawnSync instead of process.env so Resolve-Platform runs with a clean
environment.
🪄 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
Run ID: 53dff96f-1cda-4f11-9902-551748224ab1
📒 Files selected for processing (2)
contrib/install/install-ps1.test.tscontrib/install/install.ps1
runPowerShellCommand forwarded the full process environment, so an externally set OO_INSTALL_PLATFORM would short-circuit Resolve-Platform and make the architecture-detection tests nondeterministic. Clear that variable in the spawned environment so the tests always exercise the real detection path. Signed-off-by: Kevin Cui <bh@bugs.cc>
1 participant
The previous code read [RuntimeInformation]::OSArchitecture, which can be absent on the type PowerShell resolves in Windows PowerShell 5.1: a shadowing assembly such as PSReadLine may define its own RuntimeInformation without that property. Under Set-StrictMode the missing-property read becomes a terminating PropertyNotFoundStrict error, so
irm https://cli.oomol.com/install.ps1 | iexfails.Detect the architecture from PROCESSOR_ARCHITEW6432 (falling back to PROCESSOR_ARCHITECTURE) instead. These work on every PowerShell version and report the true host architecture, including the WOW64 case where the inbox 32-bit PowerShell 5.1 on ARM64 resolves to arm64. The switch gains an amd64 case because that is the token PROCESSOR_ARCHITECTURE reports on x64 Windows.
Verified on real Windows PowerShell 5.1 and PowerShell 7.6.