wasm_js rustlfags on top workspace - #6116
Conversation
Sorry, something went wrong.
WalkthroughAdds a top-level wasm32 target rustflag, introduces a new wasm_js feature in common/Cargo.toml, removes the duplicate wasm target rustflags from wasm/lib/.cargo/config.toml, and updates the vm crate's wasmbind feature to depend on rustpython-common/wasm_js. Changes
Sequence Diagram(s)(omitted — changes are configuration-only and do not alter runtime control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 51acf58 and 2e36ff8. 📒 Files selected for processing (2)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2).cargo/config.toml (1)📜 Review detailscommon/Cargo.toml (1)7-8: Workspace-level wasm_js cfg looks good; minor quoting/style nit and scope check
Moving the cfg to the top-level target is the right direction. Two nits:
- Use single-quoted TOML to avoid inner escapes.
- Confirm this broad workspace-level cfg is intended for all crates when building for wasm32-unknown-unknown (it will apply to all members and their deps for that target).
Apply within this hunk:
-[target.wasm32-unknown-unknown] -rustflags = ["--cfg=getrandom_backend=\"wasm_js\""] +[target.wasm32-unknown-unknown] +rustflags = ['--cfg=getrandom_backend="wasm_js"']39-41: Double-check getrandom feature name; likely unnecessary/incorrect here
Please verify that getrandom provides a feature literally named "wasm_js". If not, this will fail or be a no-op; the backend selection is already driven by the cfg you added via rustflags. If the cfg is sufficient (it usually is), drop the feature and possibly the whole target-specific dep block to avoid confusion/duplication with the global getrandom dep on Line 22.
Option A — keep the block but remove the feature:
[target.'cfg(all(target_arch = "wasm32", not(target_os = "wasi")))'.dependencies] -getrandom = { workspace = true, features = ["wasm_js"] } +getrandom = { workspace = true }Option B — remove the redundant block entirely (global dep already exists):
-[target.'cfg(all(target_arch = "wasm32", not(target_os = "wasi")))'.dependencies] -getrandom = { workspace = true, features = ["wasm_js"] }Also, if the workspace isn’t on Cargo feature resolver = "2", consider enabling it at the workspace root to prevent unintended feature unification across targets.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between 7509387 and fdf5f3e.
📒 Files selected for processing (3)- .cargo/config.toml (1 hunks)
- common/Cargo.toml (1 hunks)
- wasm/lib/.cargo/config.toml (0 hunks)
- wasm/lib/.cargo/config.toml
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
Sorry, something went wrong.
|
Ah, actually - that test failure is why the config is the way it is. Putting it in the top level means js gets selected even when we're not compiling wasm for browser. |
Sorry, something went wrong.
| [target.'cfg(all(target_arch = "wasm32", not(target_os = "wasi")))'.dependencies] | ||
| getrandom = { workspace = true, features = ["wasm_js"] } | ||
|
|
There was a problem hiding this comment.
| [target.'cfg(all(target_arch = "wasm32", not(target_os = "wasi")))'.dependencies] | |
| getrandom = { workspace = true, features = ["wasm_js"] } |
This is wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, true. I guess that should be enabled by a js feature flag that common exposes.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review detailsConfiguration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between fdf5f3e and 51acf58.
📒 Files selected for processing (4)- .cargo/config.toml (1 hunks)
- common/Cargo.toml (1 hunks)
- vm/Cargo.toml (1 hunks)
- wasm/lib/.cargo/config.toml (0 hunks)
- wasm/lib/.cargo/config.toml
- .cargo/config.toml
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to {vm,stdlib}/**/*.rs : Use RustPython macros (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
- vm/Cargo.toml
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
Sorry, something went wrong.
| parser = ["ast"] | ||
| serde = ["dep:serde"] | ||
| wasmbind = ["chrono/wasmbind", "getrandom/wasm_js", "wasm-bindgen"] | ||
| wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "getrandom/wasm_js", "wasm-bindgen"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-enabling getrandom’s JS backend; rely on common’s feature only.
wasmbind currently enables both rustpython-common/wasm_js and getrandom/wasm_js. Due to feature unification this is redundant and risks confusion about the single source of truth. Drop the direct getrandom/wasm_js from vm and let common own it.
Apply:
-wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "getrandom/wasm_js", "wasm-bindgen"]
+wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "wasm-bindgen"]‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "getrandom/wasm_js", "wasm-bindgen"] | |
| wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "wasm-bindgen"] |
In vm/Cargo.toml around line 27, the wasmbind feature list currently includes both "rustpython-common/wasm_js" and "getrandom/wasm_js"; remove "getrandom/wasm_js" from the vm crate's wasmbind feature so the getrandom JS backend is provided only via rustpython-common's wasm_js feature, leaving wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "wasm-bindgen"] (ensure no other references to getrandom/wasm_js remain in this feature).
💡 Verification agent
🧩 Analysis chainDo not enable browser/wasm bindings by default; this likely caused the test failures.
default = ["compiler", "wasmbind", "stdio"] means native builds also activate wasmbind, which (transitively) opts into the JS randomness backend. Make wasmbind opt‑in only.
Apply:
-default = ["compiler", "wasmbind", "stdio"]
+default = ["compiler", "stdio"]Quick checks:
🏁 Script executed:
#!/bin/bash
# 1) Confirm default features no longer include wasmbind
rg -nP '^\s*default\s*=\s*\[.*wasmbind' -g 'vm/Cargo.toml'
# 2) Ensure no other crate pulls wasmbind by default
rg -nP '\bfeatures\b.*wasmbind' -C2 -g '!**/target/**'
# 3) Inspect .cargo/config.* to ensure any `--cfg getrandom_backend="wasm_js"` is target-scoped to wasm32 only
rg -nP --hidden -C2 'getrandom_backend\s*=\s*"wasm_js"|--cfg\s+getrandom_backend' -g '.cargo/**'Length of output: 209
Remove wasmbind from default features in vm/Cargo.toml
vm/Cargo.toml:13 currently enables wasmbind in default features, which pulls in the JS getrandom backend for native builds — make wasmbind opt‑in.
Apply:
-default = ["compiler", "wasmbind", "stdio"]
+default = ["compiler", "stdio"]Also ensure any .cargo/config.* or --cfg getrandom_backend="wasm_js" entries are target‑scoped to wasm32 and that no other crate exposes wasmbind in its default features.
🤖 Prompt for AI AgentsCommittable suggestion skipped: line range outside the PR's diff.
In vm/Cargo.toml around line 27, remove "wasmbind" from the default features list so the getrandom wasm_js backend is not pulled into native builds; instead add a separate optional feature (e.g., wasmbind) that includes the wasmbind deps and mark those dependencies as optional under that feature so wasm support is opt‑in. Also audit any .cargo/config.* files and Cargo command args and make sure any --cfg getrandom_backend="wasm_js" or equivalent settings are restricted to the wasm32 target (or moved into target."cfg(target_arch = \"wasm32\")" sections), and verify no other crate in the workspace exposes wasmbind in its default-features so the change actually prevents native pulls.
Sorry, something went wrong.
Summary by CodeRabbit
Bug Fixes
Chores