fix(runtime): isolate packaged Python from user site-packages#92
fix(runtime): isolate packaged Python from user site-packages#92zouyonghe wants to merge 3 commits intoAstrBotDevs:mainfrom
Conversation
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 enhances the robustness of the application's Python backend by ensuring a clean and isolated runtime environment. It addresses potential conflicts arising from user-installed Python packages by explicitly disabling them, thereby guaranteeing that only bundled dependencies are used. Furthermore, it improves the build process's flexibility by allowing explicit specification of target architectures, which is crucial for cross-compilation and supporting diverse environments like Windows ARM64. 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/backend/runtime-arch-utils.mjs" line_range="60-64" />
<code_context>
+ return normalizeDesktopArch(windowsArmBackendArch, WINDOWS_ARM_BACKEND_ARCH_ENV);
+};
+
+export const isWindowsArm64BundledRuntime = ({
+ platform = process.platform,
+ arch = process.arch,
+ env = process.env,
+} = {}) => platform === 'win32' && resolveBundledRuntimeArch({ platform, arch, env }) === 'arm64';
</code_context>
<issue_to_address>
**issue:** Calling `resolveBundledRuntimeArch` unconditionally here can now throw on unsupported `process.arch`, where the previous logic would just return `false`.
The previous inlined check `process.platform === 'win32' && process.arch === 'arm64'` could not throw. Now `isWindowsArm64BundledRuntime()` always calls `resolveBundledRuntimeArch()`, which can throw for unknown/unsupported arches (e.g. `ia32`) because `resolveDesktopTargetArch` only handles `x64` and `arm64`. To keep the old non‑throwing behavior, short‑circuit on `platform !== 'win32'` (and possibly on unsupported `arch`) before calling `resolveBundledRuntimeArch()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| export const isWindowsArm64BundledRuntime = ({ | ||
| platform = process.platform, | ||
| arch = process.arch, | ||
| env = process.env, | ||
| } = {}) => platform === 'win32' && resolveBundledRuntimeArch({ platform, arch, env }) === 'arm64'; |
There was a problem hiding this comment.
issue: Calling resolveBundledRuntimeArch unconditionally here can now throw on unsupported process.arch, where the previous logic would just return false.
The previous inlined check process.platform === 'win32' && process.arch === 'arm64' could not throw. Now isWindowsArm64BundledRuntime() always calls resolveBundledRuntimeArch(), which can throw for unknown/unsupported arches (e.g. ia32) because resolveDesktopTargetArch only handles x64 and arm64. To keep the old non‑throwing behavior, short‑circuit on platform !== 'win32' (and possibly on unsupported arch) before calling resolveBundledRuntimeArch().
![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 effectively isolates the packaged Python runtime from user site-packages by setting the PYTHONNOUSERSITE environment variable, which is a solid approach to prevent dependency conflicts. The addition of a regression test for this change is commendable. Furthermore, the extensive refactoring to centralize and improve architecture resolution logic, especially for Windows on ARM, significantly enhances the build process's robustness and configurability. The new utility scripts are well-structured and thoroughly tested. I've identified a few minor robustness issues in the new runtime-arch-utils.mjs script related to handling null values from environment variables and have suggested improvements.
| if (overrideRaw !== undefined && String(overrideRaw).trim()) { | ||
| return normalizeDesktopArch(overrideRaw, DESKTOP_TARGET_ARCH_ENV); | ||
| } |
There was a problem hiding this comment.
The current check for a non-empty environment variable is not robust against null values. If overrideRaw were null, String(overrideRaw) would evaluate to the string "null", which is truthy, leading to an error in normalizeDesktopArch. Using optional chaining (?.) with trim() provides a more concise and robust way to check for a non-empty string.
| if (overrideRaw !== undefined && String(overrideRaw).trim()) { | |
| return normalizeDesktopArch(overrideRaw, DESKTOP_TARGET_ARCH_ENV); | |
| } | |
| if (overrideRaw?.trim()) { | |
| return normalizeDesktopArch(overrideRaw, DESKTOP_TARGET_ARCH_ENV); | |
| } |
| if (explicitBundledRuntimeArch !== undefined && String(explicitBundledRuntimeArch).trim()) { | ||
| return normalizeDesktopArch(explicitBundledRuntimeArch, BUNDLED_RUNTIME_ARCH_ENV); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, this check is not robust against null values. Using optional chaining (?.) with trim() is a safer and more concise approach.
| if (explicitBundledRuntimeArch !== undefined && String(explicitBundledRuntimeArch).trim()) { | |
| return normalizeDesktopArch(explicitBundledRuntimeArch, BUNDLED_RUNTIME_ARCH_ENV); | |
| } | |
| if (explicitBundledRuntimeArch?.trim()) { | |
| return normalizeDesktopArch(explicitBundledRuntimeArch, BUNDLED_RUNTIME_ARCH_ENV); | |
| } |
| if (windowsArmBackendArch === undefined || !String(windowsArmBackendArch).trim()) { | ||
| return 'amd64'; | ||
| } |
There was a problem hiding this comment.
This check for a missing or empty environment variable can also be simplified and made more robust against null values by using optional chaining (?.). The expression !windowsArmBackendArch?.trim() correctly handles undefined, null, and empty/whitespace strings.
| if (windowsArmBackendArch === undefined || !String(windowsArmBackendArch).trim()) { | |
| return 'amd64'; | |
| } | |
| if (!windowsArmBackendArch?.trim()) { | |
| return 'amd64'; | |
| } |
1 participant
Summary
pydantic-settingsfrom importing an incompatible user-installedpydanticTest Plan
cargo test sanitize_packaged_python_environmentcargo testcargo fmt --checkSummary by Sourcery
Ensure the packaged backend Python runtime is isolated from user-installed packages and make Windows ARM64 runtime architecture selection explicit and configurable.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: