Skip to content

upgrade shutil #6558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 4 commits into from
Dec 28, 2025
Merged

upgrade shutil #6558

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3ca264b
Update shutil from v3.13.11
youknowone Dec 28, 2025
d6d5b66
fix os.remove
youknowone Dec 28, 2025
c30f150
fix os.fchmod
youknowone Dec 28, 2025
3f89316
fix is_file
youknowone Dec 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix os.remove
  • Loading branch information
@youknowone
youknowone committed Dec 28, 2025
commit d6d5b66b7f0fd634ac2b6c7e5fe9ff00e9972c93
52 changes: 52 additions & 0 deletions crates/vm/src/stdlib/nt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,58 @@ pub(crate) mod module {
|| attr & FileSystem::FILE_ATTRIBUTE_DIRECTORY != 0))
}

#[pyfunction]
#[pyfunction(name = "unlink")]
pub(super) fn remove(
path: OsPath,
dir_fd: DirFd<'static, 0>,
vm: &VirtualMachine,
) -> PyResult<()> {
// On Windows, use DeleteFileW directly.
// Rust's std::fs::remove_file may have different behavior for read-only files.
// See Py_DeleteFileW.
use windows_sys::Win32::Storage::FileSystem::{
DeleteFileW, FindClose, FindFirstFileW, RemoveDirectoryW, WIN32_FIND_DATAW,
};
use windows_sys::Win32::System::SystemServices::{
IO_REPARSE_TAG_MOUNT_POINT, IO_REPARSE_TAG_SYMLINK,
};

let [] = dir_fd.0;
let wide_path = path.to_wide_cstring(vm)?;
let attrs = unsafe { FileSystem::GetFileAttributesW(wide_path.as_ptr()) };

let mut is_directory = false;
let mut is_link = false;

if attrs != FileSystem::INVALID_FILE_ATTRIBUTES {
is_directory = (attrs & FileSystem::FILE_ATTRIBUTE_DIRECTORY) != 0;

// Check if it's a symlink or junction point
if is_directory && (attrs & FileSystem::FILE_ATTRIBUTE_REPARSE_POINT) != 0 {
let mut find_data: WIN32_FIND_DATAW = unsafe { std::mem::zeroed() };
let handle = unsafe { FindFirstFileW(wide_path.as_ptr(), &mut find_data) };
if handle != INVALID_HANDLE_VALUE {
is_link = find_data.dwReserved0 == IO_REPARSE_TAG_SYMLINK
|| find_data.dwReserved0 == IO_REPARSE_TAG_MOUNT_POINT;
unsafe { FindClose(handle) };
}
}
}

let result = if is_directory && is_link {
unsafe { RemoveDirectoryW(wide_path.as_ptr()) }
} else {
unsafe { DeleteFileW(wide_path.as_ptr()) }
};

if result == 0 {
let err = io::Error::last_os_error();
return Err(OSErrorBuilder::with_filename(&err, path, vm));
}
Ok(())
}

#[pyfunction]
pub(super) fn _supports_virtual_terminal() -> PyResult<bool> {
// TODO: implement this
Expand Down
34 changes: 0 additions & 34 deletions crates/vm/src/stdlib/os.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,40 +296,6 @@ pub(super) mod _os {
data.with_ref(|b| crt_fd::write(fd, b))
}

#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
#[cfg(windows)]
let is_dir_link = {
// On Windows, we need to check if it's a directory symlink/junction
// using GetFileAttributesW, which doesn't follow symlinks.
// This is similar to CPython's Py_DeleteFileW.
use windows_sys::Win32::Storage::FileSystem::{
FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, GetFileAttributesW,
INVALID_FILE_ATTRIBUTES,
};
let wide_path: Vec<u16> = path.path.as_os_str().to_wide_with_nul();
let attrs = unsafe { GetFileAttributesW(wide_path.as_ptr()) };
if attrs != INVALID_FILE_ATTRIBUTES {
let is_dir = (attrs & FILE_ATTRIBUTE_DIRECTORY) != 0;
let is_reparse = (attrs & FILE_ATTRIBUTE_REPARSE_POINT) != 0;
is_dir && is_reparse
} else {
false
}
};
#[cfg(not(windows))]
let is_dir_link = false;

let res = if is_dir_link {
fs::remove_dir(&path)
} else {
fs::remove_file(&path)
};
res.map_err(|err| OSErrorBuilder::with_filename(&err, path, vm))
}

#[cfg(not(windows))]
#[pyfunction]
fn mkdir(
Expand Down
7 changes: 7 additions & 0 deletions crates/vm/src/stdlib/posix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,13 @@ pub mod module {
}
}

#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
fs::remove_file(&path).map_err(|err| OSErrorBuilder::with_filename(&err, path, vm))
}

#[cfg(not(target_os = "redox"))]
#[pyfunction]
fn fchdir(fd: BorrowedFd<'_>, vm: &VirtualMachine) -> PyResult<()> {
Expand Down
10 changes: 9 additions & 1 deletion crates/vm/src/stdlib/posix_compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,24 @@ pub(crate) mod module {
use crate::{
PyObjectRef, PyResult, VirtualMachine,
builtins::PyStrRef,
convert::IntoPyException,
ospath::OsPath,
stdlib::os::{_os, DirFd, SupportFunc, TargetIsDirectory},
};
use std::env;
use std::{env, fs};

#[pyfunction]
pub(super) fn access(_path: PyStrRef, _mode: u8, vm: &VirtualMachine) -> PyResult<bool> {
os_unimpl("os.access", vm)
}

#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
fs::remove_file(&path).map_err(|err| err.into_pyexception(vm))
}
Comment on lines +28 to +33
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 28, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Consider using OSErrorBuilder for consistency and better error messages.

The error handling here uses IntoPyException directly, while the equivalent function in posix.rs (lines 479-482) uses OSErrorBuilder::with_filename(&err, path, vm) to include the filename in error messages. This inconsistency means users on posix_compat platforms will get less helpful error messages.

🔎 Suggested fix 
+use crate::exceptions::OSErrorBuilder;

 #[pyfunction]
 #[pyfunction(name = "unlink")]
 fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
     let [] = dir_fd.0;
-    fs::remove_file(&path).map_err(|err| err.into_pyexception(vm))
+    fs::remove_file(&path).map_err(|err| OSErrorBuilder::with_filename(&err, path, vm))
 }
📝 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.

Suggested change
#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
fs::remove_file(&path).map_err(|err| err.into_pyexception(vm))
}
use crate::exceptions::OSErrorBuilder;
#[pyfunction]
#[pyfunction(name = "unlink")]
fn remove(path: OsPath, dir_fd: DirFd<'_, 0>, vm: &VirtualMachine) -> PyResult<()> {
let [] = dir_fd.0;
fs::remove_file(&path).map_err(|err| OSErrorBuilder::with_filename(&err, path, vm))
}
🤖 Prompt for AI Agents 
In crates/vm/src/stdlib/posix_compat.rs around lines 28 to 33, the remove()
function maps fs::remove_file errors using err.into_pyexception(vm) which omits
the filename; replace that mapping with OSErrorBuilder::with_filename(&err,
path, vm).map_err(...) (i.e., construct the OSError via
OSErrorBuilder::with_filename using the original io::Error and the path, then
return that as the PyErr) so error messages match posix.rs and include the
filename for consistency.


#[derive(FromArgs)]
#[allow(unused)]
pub(super) struct SymlinkArgs<'a> {
Expand Down