◐ Shell
reader mode source ↗
Skip to content

wasm_js rustlfags on top workspace#6116

Merged
youknowone merged 3 commits into
RustPython:mainfrom
youknowone:rustflag
Sep 17, 2025
Merged

wasm_js rustlfags on top workspace#6116
youknowone merged 3 commits into
RustPython:mainfrom
youknowone:rustflag

Conversation

@youknowone

@youknowone youknowone commented Aug 31, 2025

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of WebAssembly builds by ensuring a browser-compatible random backend is selected for wasm32 targets, reducing initialization errors in non-WASI WASM environments.
  • Chores

    • Consolidated WASM build configuration at the workspace level for consistent behavior across platforms.
    • Added a workspace-level WASM feature and updated package feature mappings to streamline cross-package builds and reduce duplication.

@coderabbitai

coderabbitai Bot commented Aug 31, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Adds 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

Cohort / File(s) Change summary
Top-level Cargo config
/.cargo/config.toml
Adds [target.wasm32-unknown-unknown] with rustflags = ["--cfg=getrandom_backend=\"wasm_js\""].
Common crate features
/common/Cargo.toml
Adds feature wasm_js = ["getrandom/wasm_js"] under [features].
WASM library local config cleanup
/wasm/lib/.cargo/config.toml
Removes the local [target.wasm32-unknown-unknown] rustflags entry; keeps [build] target = "wasm32-unknown-unknown".
VM crate feature update
/vm/Cargo.toml
Updates wasmbind feature to include rustpython-common/wasm_js in its feature list (replacing direct getrandom/wasm_js).

Sequence Diagram(s)

(omitted — changes are configuration-only and do not alter runtime control flow)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

I nibble flags beneath the moon,
A tiny hop, a compile tune.
Features stitched and flags set free,
getrandom wanders, safe and merry.
From burrow to the browser sea—hooray for me! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title refers to moving wasm_js rustflags to the top-level workspace, which matches the PR objective and the changeset, but it contains a spelling error ("rustlfags") and awkward phrasing that reduce clarity.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 51acf58 and 2e36ff8.

📒 Files selected for processing (2)
  • common/Cargo.toml (1 hunks)
  • vm/Cargo.toml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • vm/Cargo.toml
  • common/Cargo.toml

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide 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 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"']
common/Cargo.toml (1)

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.

📜 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.

📥 Commits

Reviewing 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)
💤 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)

@coolreader18

Copy link
Copy Markdown
Member

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing 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)
💤 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

@youknowone

Copy link
Copy Markdown
Member Author

@coolreader18 better?

Hide details View details @youknowone youknowone merged commit aa0eb4b into RustPython:main Sep 17, 2025
12 checks passed
@youknowone youknowone deleted the rustflag branch September 17, 2025 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants