Repository navigation
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The native macOS permission dialog still requires verification on Apple Silicon without Rosetta.
0 open findings
What changed in this PR
Updates desktop CLI installation for Apple Silicon compatibility.
Changes:
- Replaces
sudo-promptwith the universal@vscode/sudo-prompt. - Detects Rosetta translation to download the ARM64 CLI.
- Adds cross-platform architecture regression tests.
| File | Description |
|---|---|
src/main/installCLI.ts |
Selects ARM64 under Rosetta and uses the new elevation package. |
tests/unit/main/installCLI.spec.js |
Covers macOS, Windows, and Linux architecture selection. |
package.json |
Replaces the elevation dependency. |
yarn.lock |
Locks the new dependency version. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ysfscream
left a comment
ysfscream
left a comment
There was a problem hiding this comment.
The fix looks right to me. I checked both packages:
- The helper app in
sudo-prompt@9.2.1only contains i386 and x86_64 code, so it can't run on an Apple Silicon Mac without Rosetta. - The helper app in
@vscode/sudo-prompt@9.3.2contains both x86_64 and arm64 code, and its API is the same. app.runningUnderARM64Translationis a documented Electron API. The Electron docs suggest using it for exactly this case.
I left a few inline comments about simplifying the change. The main one is the test file: it is much more complex than the one line it tests.
One note on the PR description: the Rosetta case is not a crash. An Intel build can only run if Rosetta is installed, and then the x64 CLI also runs. That part only gets users a native binary. The crash only happens with the native arm64 desktop app on a Mac without Rosetta. Could you split these two cases in the description, so readers know which change fixes the reported error?
| const vm = require('vm') | ||
| const ts = require('typescript') | ||
|
|
||
| async function requestedBinary(platform, arch, translated = false) { |
There was a problem hiding this comment.
This test compiles the TypeScript file by hand, runs it in a vm sandbox, and fakes every import. That is a lot of setup for one line of logic. It will also break when someone adds any new import to installCLI.ts, because the fake require throws Unexpected import.
A simpler way: move the decision into a small exported function and test that function directly. The test config already replaces electron and electron-store with mocks (see vue.config.js), so a normal import should work:
import { expect } from 'chai'
import { getCLIArch } from '@/main/installCLI'
describe('getCLIArch', () => {
it('uses arm64 when an x64 build runs under Rosetta', () => {
expect(getCLIArch('x64', true)).to.equal('arm64')
})
it('keeps the process architecture otherwise', () => {
expect(getCLIArch('x64', false)).to.equal('x64')
expect(getCLIArch('arm64', false)).to.equal('arm64')
})
})That cuts about 78 lines down to about 12. The Windows and Linux URL checks can go too: this PR doesn't change how those URLs are built.
The line is simple enough that dropping the test file would also be fine. The real risk is the password dialog on actual hardware, and a unit test can't check that anyway.
| const platform = os.platform() | ||
| const isWindows = platform === 'win32' | ||
| const isMacOS = platform === 'darwin' | ||
| const arch = isMacOS && app.runningUnderARM64Translation ? 'arm64' : os.arch() |
There was a problem hiding this comment.
If you take the test suggestion, this becomes:
/**
* An x64 build running under Rosetta is still on an ARM64 Mac, so it should get the native ARM64 CLI.
*/
export function getCLIArch(arch: string, isTranslated: boolean): string {
return isTranslated ? 'arm64' : arch
}const arch = getCLIArch(os.arch(), isMacOS && app.runningUnderARM64Translation)The isMacOS && check stays here, so Windows and Linux never read the property. Otherwise, the current one-line version is fine as it is.
| platform: os.platform(), | ||
| arch: os.arch(), | ||
| } | ||
| export default async function installCLI(win: BrowserWindow): Promise<void> { |
There was a problem hiding this comment.
Nit: the : Promise<void> return type and splitting up the old { platform, arch } object are unrelated to this fix. Both are harmless, but keeping the diff to the actual fix makes it easier to review and revert.
What is the current behavior?
On an Apple Silicon Mac running the native ARM64 Desktop app without Rosetta, Desktop's Install CLI action fails with
./applet: Bad CPU type in executable. The macOS permission helper bundled withsudo-prompt@9.2.1contains i386 and x86_64 code, but no ARM64 code. In this case, Desktop already selects the ARM64 CLI; the failure occurs when starting the permission helper.Separately, an Intel Desktop app running under Rosetta selects the x64 CLI because
os.arch()reports the process architecture. The x64 CLI can run in this environment because Rosetta is present, but users receive a translated binary.Issue Number
Reported by Ivan in Slack. Companion Homebrew fix: emqx/homebrew-mqttx#3 (Refs emqx/homebrew-mqttx#1).
What is the new behavior?
sudo-promptwith@vscode/sudo-prompt@9.3.2, whose permission helper contains both ARM64 and x86_64 code.Native ARM64 Desktop apps and Intel Macs retain their existing CLI architecture selection. Windows and Linux behavior is unchanged.
Does this PR introduce a breaking change?
Specific Instructions
Verify the permission dialog and completed CLI installation using the native ARM64 Desktop app on an Apple Silicon Mac without Rosetta.
Other information
Validation:
getCLIArchunit tests passed through the normalyarn test:unitrunner, covering Rosetta and native architectures.--max-warnings 0.app.asar.