host_env: os.mkdir for Windows, Redox#8128
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR unifies the directory-creation API in ChangesUnified make_dir API refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/host_env/src/posix_wasi.rs`:
- Around line 4-10: The make_dir function uses &Path as a parameter type, but
Path is not imported from the standard library, causing a compilation error. Add
path::Path to the existing std imports at the top of the file where ffi::OsStr
and io are already imported to bring Path into scope.
In `@crates/vm/src/stdlib/os.rs`:
- Line 191: The MKDIR_DIR_FD constant in the os.rs file is currently configured
only for unix systems using cfg!(unix), but since WASI now supports the optional
dir_fd parameter in make_dir function as implemented in posix_wasi.rs, the cfg
condition needs to be updated. Modify the MKDIR_DIR_FD constant definition to
also include WASI targets in the condition so that dir_fd functionality is
enabled on WASI systems where it is now supported.
🪄 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: 9e70d5e8-58e8-4b98-9525-1ee325ec8978
📒 Files selected for processing (3)
crates/host_env/src/posix.rscrates/host_env/src/posix_wasi.rscrates/vm/src/stdlib/os.rs
| use std::{ffi::OsStr, io}; | ||
|
|
||
| use crate::os::CheckLibcResult; | ||
| use crate::{crt_fd, os::CheckLibcResult}; | ||
|
|
||
| pub fn make_dir(path: &CStr, mode: u32) -> io::Result<()> { | ||
| unsafe { libc::mkdir(path.as_ptr(), mode as _) }.check_libc_neg()?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn make_dir_at(dir_fd: i32, path: &CStr, mode: u32) -> io::Result<()> { | ||
| unsafe { libc::mkdirat(dir_fd, path.as_ptr(), mode as _) }.check_libc_neg()?; | ||
| Ok(()) | ||
| pub fn make_dir( | ||
| dir_fd: Option<crt_fd::Borrowed<'_>>, | ||
| path: &Path, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -i posix_wasi.rs | head -n1)"
echo "Using file: $file"
echo "== make_dir signature references Path =="
rg -n 'pub fn make_dir|path:\s*&Path' "$file"
echo "== imports that bring Path into scope (should have at least one match) =="
rg -n 'use\s+std::.*Path|use\s+std::path::Path' "$file" || trueRepository: RustPython/RustPython
Length of output: 265
🏁 Script executed:
head -n 20 crates/host_env/src/posix_wasi.rsRepository: RustPython/RustPython
Length of output: 657
🏁 Script executed:
# Check what's in the CheckLibcResult import
fd -i checkclient.rs -o chelibcresult.rs || rg -l "CheckLibcResult" crates/host_env/src/ | head -n 3Repository: RustPython/RustPython
Length of output: 322
Import Path for make_dir to compile.
Line 10 uses &Path, but Path is not in scope. Add path::Path to the standard library imports.
🔧 Proposed fix
-use std::{ffi::OsStr, io};
+use std::{ffi::OsStr, io, path::Path};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use std::{ffi::OsStr, io}; | |
| use crate::os::CheckLibcResult; | |
| use crate::{crt_fd, os::CheckLibcResult}; | |
| pub fn make_dir(path: &CStr, mode: u32) -> io::Result<()> { | |
| unsafe { libc::mkdir(path.as_ptr(), mode as _) }.check_libc_neg()?; | |
| Ok(()) | |
| } | |
| pub fn make_dir_at(dir_fd: i32, path: &CStr, mode: u32) -> io::Result<()> { | |
| unsafe { libc::mkdirat(dir_fd, path.as_ptr(), mode as _) }.check_libc_neg()?; | |
| Ok(()) | |
| pub fn make_dir( | |
| dir_fd: Option<crt_fd::Borrowed<'_>>, | |
| path: &Path, | |
| use std::{ffi::OsStr, io, path::Path}; | |
| use crate::{crt_fd, os::CheckLibcResult}; | |
| pub fn make_dir( | |
| dir_fd: Option<crt_fd::Borrowed<'_>>, | |
| path: &Path, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/host_env/src/posix_wasi.rs` around lines 4 - 10, The make_dir function
uses &Path as a parameter type, but Path is not imported from the standard
library, causing a compilation error. Add path::Path to the existing std imports
at the top of the file where ffi::OsStr and io are already imported to bring
Path into scope.
|
|
||
| const OPEN_DIR_FD: bool = cfg!(not(any(windows, target_os = "redox"))); | ||
| pub(crate) const MKDIR_DIR_FD: bool = cfg!(not(any(windows, target_os = "redox"))); | ||
| pub(crate) const MKDIR_DIR_FD: bool = cfg!(unix); |
There was a problem hiding this comment.
Re-enable WASI dir_fd for mkdir capability flags.
Line [191] sets MKDIR_DIR_FD to cfg!(unix), which is false on WASI, so mkdir(..., dir_fd=...) is rejected even though crates/host_env/src/posix_wasi.rs now supports optional dir_fd in make_dir.
🔧 Proposed fix
- pub(crate) const MKDIR_DIR_FD: bool = cfg!(unix);
+ pub(crate) const MKDIR_DIR_FD: bool = cfg!(any(unix, target_os = "wasi"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) const MKDIR_DIR_FD: bool = cfg!(unix); | |
| pub(crate) const MKDIR_DIR_FD: bool = cfg!(any(unix, target_os = "wasi")); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/stdlib/os.rs` at line 191, The MKDIR_DIR_FD constant in the
os.rs file is currently configured only for unix systems using cfg!(unix), but
since WASI now supports the optional dir_fd parameter in make_dir function as
implemented in posix_wasi.rs, the cfg condition needs to be updated. Modify the
MKDIR_DIR_FD constant definition to also include WASI targets in the condition
so that dir_fd functionality is enabled on WASI systems where it is now
supported.
Python supports `mkdir` on Windows. I deferred to calling Rust's implementation for now. However, like `rename`, Python's implementation supports additional features that are currently unsupported on RustPython. I added a note for future reference. Redox supports `mkdirat` which significantly cleans up the implementation.
9e4a98a to
40dfeee
Compare
Python supports
mkdiron Windows. I deferred to calling Rust's implementation for now. However, likerename, Python's implementation supports additional features that are currently unsupported on RustPython. I added a note for future reference.Redox supports
mkdiratwhich significantly cleans up the implementation.Summary
On a related note, I think the WASI Posix and Unix implementations can be combined eventually. 🤔
Summary by CodeRabbit