Fix wasm32-unknown-unknown target build #6278

moreal · 2025-11-15T23:35:08Z

This pull request fixes wasm32-unknown-unknown target build in the following 2 points.

It discards wasm-bindgen dependency because it needs to provide _wbg_xxx functions through imports object to use WASM build output.
Adds #[unsafe(no_mangle)] line for wasm/wasm-unknown-test/src/lib.rs file because the build without the line, doesn't include eval file in the build output. You can check it by running cd wasm/wasm-unknown-test/; cargo build --target wasm32-unknown-unknown; wasm2wat target/wasm32-unknown-unknown/debug/wasm_unknown_test.wasm. There may be no $eval function if the line doesn't exist.

Links

Summary by CodeRabbit

Chores
- Updated dependency configuration to refine feature flags and ensure consistent dependency resolution across the workspace.
- Simplified platform-specific dependency declarations to streamline WebAssembly target builds.
- Adjusted WebAssembly export annotations to improve interoperability and runtime stability for wasm consumers.

coderabbitai · 2025-11-15T23:35:14Z

Walkthrough

Workspace chrono dependency changed from a simple version to an explicit table (version, disabled default features, enabled features). crates/vm removed chrono's wasmbind feature and deleted a wasm32-specific getrandom dependency block. Added #[unsafe(no_mangle)] to the exported eval function in the wasm test crate.

Changes

Cohort / File(s)	Change Summary
Workspace Cargo `Cargo.toml`	`chrono` changed from `chrono = "0.4.42"` to `chrono = { version = "0.4.42", default-features = false, features = ["clock", "oldtime", "std"] }` under `[workspace.dependencies]`.
VM crate Cargo `crates/vm/Cargo.toml`	`chrono` dependency changed from `{ workspace = true, features = ["wasmbind"] }` to `{ workspace = true }`. Removed `getrandom = { workspace = true }` from the wasm32 target-specific dependencies.
WASM test export `wasm/wasm-unknown-test/src/lib.rs`	Annotated the public exported function `pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> u32` with `#[unsafe(no_mangle)]`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Verify that the selected chrono features (clock, oldtime, std) and default-features = false satisfy all crate consumers.
Confirm removing wasmbind from the VM crate doesn't break any wasm-specific APIs or build flags.
Inspect removal of the wasm32 getrandom entry to ensure randomness support for wasm targets remains provided elsewhere.
Check that #[unsafe(no_mangle)] is the intended attribute and compatible with the wasm toolchain and downstream bindings.

Possibly related PRs

wasm_js rustlfags on top workspace #6116 — touches wasm-related Cargo configuration and wasm/getrandom/wasmbind handling; likely related to the workspace/wasm dependency adjustments.

Suggested reviewers

youknowone

Poem

🐰
I nibbled through Cargo lines at dawn,
swapped a version for features, tidy and drawn.
I pruned a wasm flag, let getrandom rest,
pinned eval's export for safety's best.
Hoppity-hop — the build hops on its quest. 🥕

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 'Fix wasm32-unknown-unknown target build' directly and accurately summarizes the main objective of the pull request, which focuses on fixing builds for the wasm32-unknown-unknown target.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

🧪 Generate unit tests (beta)

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 d6c72a5 and 5d51df6.

📒 Files selected for processing (1)

crates/vm/Cargo.toml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

crates/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). (10)

GitHub Check: Check the WASM package and demo
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Check Rust code with clippy
GitHub Check: Ensure compilation on various targets
GitHub Check: auto_format

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/vm/Cargo.toml (1)
27-27: Critical: Remove chrono/wasmbind from the feature definition.

The wasmbind feature references chrono/wasmbind, but the workspace configuration for chrono (line 165 in root Cargo.toml) explicitly disables default features and only enables ["clock", "oldtime", "std"]. The wasmbind feature is not included, so this will cause a build error when the wasmbind feature is enabled.

Apply this diff to fix the feature definition:
-wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "wasm-bindgen"]
+wasmbind = ["rustpython-common/wasm_js", "wasm-bindgen"]

🧹 Nitpick comments (1)

wasm/wasm-unknown-test/src/lib.rs (1)
1-11: Consider renaming the exported function to avoid shadowing the imported module.

The exported function eval shares the same name as the imported eval module, which can be confusing when reading the code (e.g., line 8: eval::eval(...)).

Consider renaming the exported function for clarity:
 use rustpython_vm::{Interpreter, eval};
 
 #[unsafe(no_mangle)]
-pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> u32 {
+pub unsafe extern "C" fn python_eval(s: *const u8, l: usize) -> u32 {
     let src = std::slice::from_raw_parts(s, l);
     let src = std::str::from_utf8(src).unwrap();
     Interpreter::without_stdlib(Default::default()).enter(|vm| {
         let res = eval::eval(vm, src, vm.new_scope_with_builtins(), "<string>").unwrap();
         res.try_into_value(vm).unwrap()
     })
 }
Note: If external code depends on the eval symbol name, this change would be breaking.

📜 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 6991a80 and c0d426f.

📒 Files selected for processing (3)

Cargo.toml (1 hunks)
crates/vm/Cargo.toml (2 hunks)
wasm/wasm-unknown-test/src/lib.rs (1 hunks)

🔇 Additional comments (3)

crates/vm/Cargo.toml (1)

49-49: LGTM: Dependency reorganization aligns with wasm32-unknown-unknown target support.

The changes correctly:

Simplify chrono to use workspace configuration

Make wasm-bindgen optional and broaden its target condition to cfg(target_arch = "wasm32")

Add getrandom specifically for cfg(all(target_arch = "wasm32", target_os = "unknown"))

This allows wasm32-unknown-unknown builds without wasm-bindgen while keeping it available for other wasm32 targets.

Also applies to: 151-155

Cargo.toml (1)

165-165: LGTM: Explicit chrono feature configuration improves wasm32-unknown-unknown compatibility.

The change to explicitly disable default features and enable only ["clock", "oldtime", "std"] removes the implicit wasmbind feature dependency, which aligns with the PR objective of removing wasm-bindgen requirements for the wasm32-unknown-unknown target.

wasm/wasm-unknown-test/src/lib.rs (1)

3-3: The #[unsafe(no_mangle)] syntax is properly supported.

The workspace rust-version is 1.89.0, which exceeds the 1.82 requirement for this attribute syntax. The code change correctly prevents the function symbol from being omitted in the WASM output.

ShaharNaveh · 2025-11-16T08:57:05Z

⚠️ Outside diff range comments (1)
crates/vm/Cargo.toml (1)> 27-27: Critical: Remove chrono/wasmbind from the feature definition.
The wasmbind feature references chrono/wasmbind, but the workspace configuration for chrono (line 165 in root Cargo.toml) explicitly disables default features and only enables ["clock", "oldtime", "std"]. The wasmbind feature is not included, so this will cause a build error when the wasmbind feature is enabled.
Apply this diff to fix the feature definition:
-wasmbind = ["rustpython-common/wasm_js", "chrono/wasmbind", "wasm-bindgen"]
+wasmbind = ["rustpython-common/wasm_js", "wasm-bindgen"]

@moreal

youknowone · 2025-11-16T10:08:52Z

@ShaharNaveh Then adding wasmbind feature to vm will not enable chrono/wasmbind. @coderabbitai is wrong about it

coderabbitai · 2025-11-16T10:09:46Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the root Cargo.toml to see the chrono workspace dependency configuration
echo "=== Root Cargo.toml (around line 165) ==="
sed -n '160,170p' Cargo.toml

echo -e "\n=== crates/vm/Cargo.toml (around line 27 and wasmbind feature) ==="
# Find the wasmbind feature definition
grep -n "wasmbind" crates/vm/Cargo.toml -A 2 -B 2

Length of output: 710

@youknowone You're absolutely correct, and I apologize for the error in the previous review comment.

The chrono/wasmbind reference in the feature definition is correct and necessary. Here's why:

In Cargo, workspace dependency configurations set a baseline of features, but individual crates can enable additional features on top of that baseline through their own feature definitions.

In this case:

The workspace config sets chrono with features ["clock", "oldtime", "std"] as the baseline
When the wasmbind feature is enabled on the vm crate, the line chrono/wasmbind additively enables chrono's wasmbind feature in addition to the workspace baseline
Removing chrono/wasmbind from the feature definition would indeed prevent the chrono wasmbind feature from being enabled, which is not the desired behavior

Thank you for the correction!

crates/vm/Cargo.toml

youknowone

👍 Thank you!

moreal changed the title ~~Fix wasm32-unknown-unknown~~ Fix wasm32-unknown-unknown target build Nov 15, 2025

moreal force-pushed the try-wasm32-unknown branch from e3800b0 to c0d426f Compare November 16, 2025 03:28

moreal marked this pull request as ready for review November 16, 2025 05:07

coderabbitai bot reviewed Nov 16, 2025

View reviewed changes

youknowone requested changes Nov 16, 2025

View reviewed changes

crates/vm/Cargo.toml Outdated Show resolved Hide resolved

crates/vm/Cargo.toml Outdated Show resolved Hide resolved

Fix wasm32-unknown-unknown

1a68f42

moreal force-pushed the try-wasm32-unknown branch from c0d426f to d6c72a5 Compare November 16, 2025 13:01

moreal added 2 commits November 16, 2025 22:02

Revert unnecessary changes

e75cc92

Dedupe getrandom dependency

5d51df6

moreal force-pushed the try-wasm32-unknown branch from d6c72a5 to 5d51df6 Compare November 16, 2025 13:02

moreal requested a review from youknowone November 16, 2025 13:07

youknowone approved these changes Nov 16, 2025

View reviewed changes

youknowone merged commit 7c4c1ea into RustPython:main Nov 16, 2025
13 checks passed

coderabbitai bot mentioned this pull request Nov 17, 2025

Test wasm32 without js #6283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix wasm32-unknown-unknown target build #6278

Fix wasm32-unknown-unknown target build #6278

Uh oh!

moreal commented Nov 15, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

ShaharNaveh commented Nov 16, 2025

Uh oh!

youknowone commented Nov 16, 2025

Uh oh!

coderabbitai bot commented Nov 16, 2025

Uh oh!

Uh oh!

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix wasm32-unknown-unknown target build #6278

Fix wasm32-unknown-unknown target build #6278

Uh oh!

Conversation

moreal commented Nov 15, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh commented Nov 16, 2025

Uh oh!

youknowone commented Nov 16, 2025

Uh oh!

coderabbitai bot commented Nov 16, 2025

Uh oh!

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moreal commented Nov 15, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 15, 2025 •

edited

Loading