wasm_js rustlfags on top workspace#6116
Conversation
WalkthroughAdds a top-level wasm32 target rustflag, introduces a new 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)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (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. Comment |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.cargo/config.toml (1)
7-8: Workspace-level wasm_js cfg looks good; minor quoting/style nit and scope checkMoving 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"']common/Cargo.toml (1)
39-41: Double-check getrandom feature name; likely unnecessary/incorrect herePlease 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.
📜 Review details
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.
📒 Files selected for processing (3)
.cargo/config.toml(1 hunks)common/Cargo.toml(1 hunks)wasm/lib/.cargo/config.toml(0 hunks)
💤 Files with no reviewable changes (1)
- wasm/lib/.cargo/config.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- 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.
fdf5f3e to
51acf58
Compare
September 14, 2025 08:07
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 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)
💤 Files with no reviewable changes (1)
- wasm/lib/.cargo/config.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .cargo/config.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T05:06:59.971Z
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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- 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.
51acf58 to
2e36ff8
Compare
September 14, 2025 08:22
aa0eb4b
into
RustPython:main
Sep 17, 2025
Summary by CodeRabbit
Bug Fixes
Chores