Skip to content

Implement os.waitpid on Windows #2048

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

Merged
merged 1 commit into from
Aug 1, 2020

Conversation

BasixKOR
Copy link
Contributor

This PR implements os.waitpid using MSVCRT's _cwait function.

@BasixKOR
Copy link
Contributor Author

BasixKOR commented Jul 31, 2020

This function has a corresponding CPython test suite, although it's not possible to run it on Windows yet.

Comment on lines 2465 to 2469
#[cfg(all(windows, target_env = "msvc"))]
extend_module!(vm, module, {
"wait" => ctx.new_function(os_wait),
"waitpid" => ctx.new_function(os_waitpid),
})
Copy link
Member

Choose a reason for hiding this comment

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

This is placed in unix extend_module_platform_specific for now. Moving this code a few lines below will be perfect.

_ => unreachable!(),
}
} else {
Ok((pid, status))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, this should be lshifted by a byte.

@BasixKOR BasixKOR requested a review from youknowone July 31, 2020 16:22
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

It looks a bit different to CPython implementation but good enough to work . I wish we had a test for this. We really need to import os test from CPython.

I rebased this due to #2041

Comment on lines +2310 to +2320
let mut errno = 0;
unsafe { _get_errno(&mut errno) };
match errno {
ECHILD => Err(vm.new_exception_msg(
vm.ctx.exceptions.os_error.clone(),
"ECHILD: No spawned processes".to_owned(),
)),
EINVAL => Err(vm.new_exception_msg(
vm.ctx.exceptions.os_error.clone(),
"EINVAL: Invalid argument".to_owned(),
)),
Copy link
Member

Choose a reason for hiding this comment

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

Following unix convert_nix_error, here could be convert_nt_error

Copy link
Contributor Author

@BasixKOR BasixKOR Aug 2, 2020

Choose a reason for hiding this comment

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

I'd love to use that kind of error handler, but it seems like in _cwait Microsoft uses some kind of POSIX-like errno which means it's neither WinAPI error nor POSIX error.

@youknowone youknowone merged commit 0a8df91 into RustPython:master Aug 1, 2020
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.

2 participants