-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Implement dir_fd in os.open #2515
Conversation
Now pass `test_posix.py PosixTester.test_open_dir_fd` Implement `dir_fd` in `make_path` and `os.open(path, flags, mode=0o777, *, dir_fd=None)`
vm/src/stdlib/os.rs
Outdated
return Ok(path.into_os_string()); | ||
} | ||
|
||
if cfg!(target_os = "wasi") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can use cfg_if!
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cfg_if!
in cdcc191
Related to #1175 |
@coolreader18 Sorry for pinging you out of nowhere but could you please take a look at this? Is there anything I could change to make this better? Thanks in advanced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good idea for a "quick and dirty" handling of dir_fd! I think eventually it"d be good to use stuff like openat
that handle dir_fd natively, but for now this definitely addresses the lack of support.
vm/src/stdlib/os.rs
Outdated
) -> PyResult<std::borrow::Cow<'a, ffi::OsStr>> { | ||
let path = &path.path; | ||
if dir_fd.0.is_none() | path.is_absolute() { | ||
return Ok(std::borrow::Cow::Borrowed(path.as_os_str().into())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the unnecessary .into()
here is causing the CI failure
vm/src/stdlib/os.rs
Outdated
} | ||
|
||
cfg_if::cfg_if! { | ||
if #[cfg(target_os = "wasi")] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the cfg
s you added here should match the cfg in Cargo.toml
vm/src/stdlib/os.rs
Outdated
Ok(path.path.as_os_str()) | ||
) -> PyResult<std::borrow::Cow<'a, ffi::OsStr>> { | ||
let path = &path.path; | ||
if dir_fd.0.is_none() | path.is_absolute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if dir_fd.0.is_none() | path.is_absolute() { | |
if dir_fd.0.is_none() || path.is_absolute() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for contributing!
Now pass
test_posix.py PosixTester.test_open_dir_fd
Implement dir_fd in
make_path
andos.open(path, flags, mode=0o777, *, dir_fd=None)