[codex] default Windows ARM backend runtime to x64#90
[codex] default Windows ARM backend runtime to x64#90zouyonghe merged 2 commits intoAstrBotDevs:mainfrom
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical compatibility issue on Windows on ARM where the default ARM64 CPython backend often failed to install native packages due to a lack of Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully defaults the Windows on ARM backend runtime to x64, which should improve compatibility with Python's native package ecosystem. The introduction of new environment variables for architecture control, the centralization of resolution logic into a new utility module, and the addition of comprehensive unit tests are all excellent improvements. I've identified a minor robustness issue in the new utility module concerning the handling of null environment variable values, which could lead to unexpected errors. My review includes specific suggestions to address this.
|
|
||
| export const resolveDesktopTargetArch = ({ arch = process.arch, env = process.env } = {}) => { | ||
| const overrideRaw = env[DESKTOP_TARGET_ARCH_ENV]; | ||
| if (overrideRaw !== undefined && String(overrideRaw).trim()) { |
There was a problem hiding this comment.
The current check for overrideRaw is not robust against null values. If env[DESKTOP_TARGET_ARCH_ENV] is null, overrideRaw !== undefined evaluates to true, and String(overrideRaw).trim() becomes 'null', which is a truthy string. This would cause normalizeDesktopArch to be called with null, leading to an error. Using the nullish coalescing operator (??) provides a more robust way to handle potentially null or undefined values, ensuring they are treated as empty strings and correctly ignored.
| if (overrideRaw !== undefined && String(overrideRaw).trim()) { | |
| if (String(overrideRaw ?? '').trim()) { |
| env = process.env, | ||
| } = {}) => { | ||
| const explicitBundledRuntimeArch = env[BUNDLED_RUNTIME_ARCH_ENV]; | ||
| if (explicitBundledRuntimeArch !== undefined && String(explicitBundledRuntimeArch).trim()) { |
There was a problem hiding this comment.
Similar to the previous comment, this check is not robust against null values. If explicitBundledRuntimeArch is null, it will be incorrectly processed as the string 'null'. Please use the nullish coalescing operator (??) for a more robust check that correctly handles null and undefined values.
| if (explicitBundledRuntimeArch !== undefined && String(explicitBundledRuntimeArch).trim()) { | |
| if (String(explicitBundledRuntimeArch ?? '').trim()) { |
| } | ||
|
|
||
| const windowsArmBackendArch = env[WINDOWS_ARM_BACKEND_ARCH_ENV]; | ||
| if (windowsArmBackendArch === undefined || !String(windowsArmBackendArch).trim()) { |
There was a problem hiding this comment.
This check for windowsArmBackendArch is also vulnerable to null values. If windowsArmBackendArch is null, the condition windowsArmBackendArch === undefined is false, and !String(windowsArmBackendArch).trim() is also false. This causes the if block to be skipped and normalizeDesktopArch to be called with null, which will fail. A more robust check is needed here as well.
| if (windowsArmBackendArch === undefined || !String(windowsArmBackendArch).trim()) { | |
| if (!String(windowsArmBackendArch ?? '').trim()) { |
1 participant
Summary
On Windows on ARM, the desktop shell currently bundles an ARM64 CPython backend by default. That works in CI when dependencies are preinstalled on the runner, but it breaks down for runtime-installed native packages on user machines because many upstream packages ship
win_amd64wheels but notwin_arm64wheels.This change keeps the Windows ARM desktop shell native while defaulting the bundled backend runtime to x64. It also makes the runtime-selection logic depend on the intended desktop target architecture instead of the current Node process architecture, which matters when a WOA build runs under an emulated x64 Node process.
Root cause
The existing runtime mapping in
scripts/prepare-resources/backend-runtime.mjsusedprocess.archdirectly, so Windows ARM builds always selectedaarch64-pc-windows-msvcwhen the build host reported ARM64. That meant the packaged backend inherited the smallerwin_arm64Python wheel ecosystem. Optional native packages likepilkthen fell back to source builds on end-user machines and failed without a matching local toolchain.What changed
x86_64-pc-windows-msvcASTRBOT_DESKTOP_WINDOWS_ARM_BACKEND_ARCHto explicitly forceamd64/x64orarm64/aarch64ASTRBOT_DESKTOP_TARGET_ARCHso CI can pass the intended desktop target arch and avoid relying on the active Node binary archUser impact
Windows ARM users still get a native desktop shell, but the packaged Python backend now prefers the broader x64 wheel ecosystem. That reduces the chance of runtime package installation failures for native-extension dependencies while preserving an escape hatch to force the legacy ARM64 backend when needed.
Validation
node --test scripts/backend/runtime-arch-utils.test.mjs scripts/prepare-resources/backend-runtime.test.mjspnpm test:prepare-resourcesBoth checks passed locally after the final commit.
Summary by Sourcery
Default the bundled backend runtime for Windows-on-ARM desktop builds to x64 while centralizing runtime-architecture resolution and wiring it through build and packaging.
New Features:
Enhancements:
Tests: