Skip to content
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

Merged
merged 5 commits into from
Mar 20, 2021
Merged

Implement dir_fd in os.open #2515

merged 5 commits into from
Mar 20, 2021

Conversation

deantvv
Copy link
Contributor

@deantvv deantvv commented Mar 8, 2021

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)

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)`
return Ok(path.into_os_string());
}

if cfg!(target_os = "wasi") {
Copy link
Contributor

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.

Copy link
Contributor Author

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

@deantvv
Copy link
Contributor Author

deantvv commented Mar 13, 2021

Related to #1175

@deantvv
Copy link
Contributor Author

deantvv commented Mar 15, 2021

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

Copy link
Member

@coolreader18 coolreader18 left a 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/Cargo.toml Outdated Show resolved Hide resolved
vm/src/stdlib/os.rs Outdated Show resolved Hide resolved
@deantvv deantvv requested a review from coolreader18 March 17, 2021 12:57
) -> 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()));
Copy link
Member

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

}

cfg_if::cfg_if! {
if #[cfg(target_os = "wasi")] {
Copy link
Member

Choose a reason for hiding this comment

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

Also, the cfgs you added here should match the cfg in Cargo.toml

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() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if dir_fd.0.is_none() | path.is_absolute() {
if dir_fd.0.is_none() || path.is_absolute() {

Copy link
Member

@coolreader18 coolreader18 left a 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!

@coolreader18 coolreader18 merged commit cba3a25 into RustPython:master Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants