GeezerVison

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

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

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 @coderabbitai help to get the list of available commands and usage tips.}

⚠️ 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

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

#!/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

@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!