Move dependencies to workspace for stdlib crate#7747
Move dependencies to workspace for stdlib crate#7747youknowone merged 2 commits intoRustPython:mainfrom
stdlib crate#7747Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRoot workspace dependencies are consolidated with explicit version and feature pins for hashing, cryptography, compression, TLS implementations, GUI, and system utilities. Crate-level dependencies in ChangesDependency Version Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
60c99ec to
e6c16a3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
271-275: Consider explicitly pinning git dependencies with immutablerevfor clarity.Lines 271 and 275 use
tag = "v0.2.0"without explicit commit hash pinning. WhileCargo.lockalready resolves and pins these to commit198fc35b1f18f4eda401f97a641908f321b1403a, explicitly specifyingrevinCargo.tomlmakes the immutable reference explicit and improves supply-chain clarity:-tcl-sys = { git = "https://github.com/arihant2math/tkinter.git", tag = "v0.2.0" } +tcl-sys = { git = "https://github.com/arihant2math/tkinter.git", rev = "198fc35b1f18f4eda401f97a641908f321b1403a" } -tk-sys = { git = "https://github.com/arihant2math/tkinter.git", tag = "v0.2.0" } +tk-sys = { git = "https://github.com/arihant2math/tkinter.git", rev = "198fc35b1f18f4eda401f97a641908f321b1403a" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 271 - 275, Update the git dependency declarations for tcl-sys and tk-sys to include the immutable commit hash: locate the entries referencing tag = "v0.2.0" for the tcl-sys and tk-sys dependencies and add rev = "198fc35b1f18f4eda401f97a641908f321b1403a" (or replace the tag with that rev) so the Cargo.toml explicitly pins the exact commit used by Cargo.lock for better supply-chain clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Cargo.toml`:
- Around line 271-275: Update the git dependency declarations for tcl-sys and
tk-sys to include the immutable commit hash: locate the entries referencing tag
= "v0.2.0" for the tcl-sys and tk-sys dependencies and add rev =
"198fc35b1f18f4eda401f97a641908f321b1403a" (or replace the tag with that rev) so
the Cargo.toml explicitly pins the exact commit used by Cargo.lock for better
supply-chain clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 134e9704-089a-498a-aed0-a11ed2fc8098
📒 Files selected for processing (2)
Cargo.tomlcrates/stdlib/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/Cargo.toml`:
- Line 65: Remove the unused dependency entry "rand = { workspace = true }" from
the crates/stdlib Cargo.toml; this dependency is not referenced by the crate
(code uses rand_core and getrandom) so delete that line and then run cargo
check/cargo build to verify the unused_dependency lint is resolved.
- Line 64: The crate uses getrandom::SysRng in crates/stdlib/src/random.rs but
the workspace dependency for getrandom only enables std; update the getrandom
dependency entry (the getrandom = { workspace = true } line) to enable the
sys_rng feature as well (e.g., add features ["std","sys_rng"]) so SysRng is
available at compile time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 12ab32a4-e80f-4bee-9166-6b78f0b6e5c0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlcrates/literal/src/float.rscrates/stdlib/Cargo.tomlcrates/stdlib/src/random.rs
|
@coderabbitai Why is the |
|
Tip For best results, initiate chat on the files or code changes.
Here's what's happening: The To enable In this PR, The fix is to conditionalize the [target.'cfg(target_arch = "wasm32")'.dependencies]
getrandom = { workspace = true, features = ["wasm_js"] }And keeping the workspace declaration without the [workspace.dependencies]
getrandom = { version = "0.4" } # no wasm_js here |
|
@coderabbitai Why is the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the current state of the relevant files to identify the remaining issue. 🏁 Script executed: Length of output: 510 🏁 Script executed: Length of output: 4599
Here's what's happening:
The fix is to update [dependencies]
# Updated from 0.3 to match workspace; "unsupported" backend is correct for
# wasm32-unknown-unknown without JS — getrandom is not called at runtime here.
getrandom = { version = "0.4", features = ["unsupported"] }
rustpython-vm = { path = "../../../crates/vm", default-features = false, features = ["compiler"] }Alternatively, you can use a # example_projects/wasm32_without_js/.cargo/config.toml
[target.wasm32-unknown-unknown]
rustflags = ['--cfg', 'getrandom_backend="unsupported"']The |
|
@coderabbitai Why is the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me inspect the current state of the relevant files to identify what's still causing the failure. 🏁 Script executed: Length of output: 7234 Let me check getrandom 0.4's API to verify the Let me dig deeper into the literal crate and check for any missing 🏁 Script executed: Length of output: 1973 Let me look at the 🏁 Script executed: Length of output: 3748 That 🏁 Script executed: Length of output: 2381 The Critical finding: for There are three versions of 🏁 Script executed: Length of output: 1649 Let me check whether 🏁 Script executed: Length of output: 4243 This is very revealing. Let me check whether 🏁 Script executed: Length of output: 5925 Let me check whether
The issue is with Why this is different from In In Your current setup only enables the Why the without-js build is likely fine: For the The fix for the wasm crate: Step 1 — Force the [target.'cfg(target_arch = "wasm32")'.dependencies]
# Force wasm_js feature on getrandom 0.3.x used transitively via ahash -> indexmap
getrandom03 = { package = "getrandom", version = "0.3", features = ["wasm_js"] }Step 2 — Create [target.wasm32-unknown-unknown]
rustflags = ['--cfg', 'getrandom_backend="wasm_js"']The Note: for Quick dependency chain summary: |
29205d9 to
8dea442
Compare
|
@ShaharNaveh tried again from scratch, making sure |
cc @ShaharNaveh
Conflicts with #7744.Rebased after #7744 was merged.Summary by CodeRabbit