-
Notifications
You must be signed in to change notification settings - Fork 6
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 for Windows #4
base: master
Are you sure you want to change the base?
Conversation
using _wexecvp() in Windows' CRT
re: #3 |
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.
Interesting! Thank you for working on this patch.
This is a fairly old library, and since Rust 1.9.0, exec
has been built into Rust, but only for Unix-like systems. So this library had been largely deprecated. (I somehow missed updating the Cargo.toml
file to indicate this.)
I think there are a few important questions here:
- The docs say that "This API cannot be used in applications that execute in the Windows Runtime. For more information, see CRT functions not supported in Universal Windows Platform apps." Are normal Rust Windows apps supposed to work with the Windows Runtime? Are there any special
cfg
flags that we need to check or document? - Would it make sense to propose this feature for the Rust standard library, because Rust already supports
exec
on Unix systems? If it doesn't make sense to propose this for the Rust standard library, would it make sense to convert all the Unix code in this library to a thin wrapper over the standard library version? - On Unix, if process A runs process B, and process B execs process C, then the final exit code of process C will be returned to process A as if it were the exit code of process B. This is a key feature of
exec
. Does this work the same on Windows?
So basically, I have a bunch of "This is cool, but where do we actually go from here?" questions. If we do decide that this is the right way to proceed, then I also have a few minor comments on the implementation itself.
Once again, many thanks for tackling this! exec
on Windows has been hard to do in Rust.
Thanks for the review! I'll address your main points:
I was in the process of upgrading a program which used this crate, and discovered precisely what you point out, that the functionality in
Windows is one platform as far as Rust's The effect of trying to build this code on UWP will be to get a linker error when it can't find any library that implements the
Ideally yes, this functionality would be in the standard library, but adding things to the standard library is a really high bar that I don't think I'm up to meeting :P Implementing the functionality in an external crate is often a desirable first step before proposing standard library changes anyway.
Yes, the behavior is just as in Unix, including propagation of the exec'd process' exit code. |
while the NulError is nice in that it includes where the NUL is, it prevents us from having the same API on Windows as on Unix, and doesn't really add enough value to overcome that, so it's now replaced with a nullary Error variant 'NullByteInArgument'. Bumping version to 0.4.0 because this is a breaking change.
I've added |
This is great! Thank you for your responses. Obviously we're going to want to remove the Unix implementation as well, and replace it with a call to Rust's standard library version. |
Congratulations to developed this wrapper about the execvp function. I used it in a project, and it worked perfectly. |
@emk have you a prevision to review this PR? |
Windows has the
_wexecvp
function in the C runtime which is the same as Unix'sexecvp
but takes Windows wide strings for arguments instead.The biggest difference is that instead of using
CString
to make FFI-ready strings, we usestd::os::windows::ffi::OsStrExt::encode_wide
which builds aVec<u16>
for us, but we need to check for interior nulls manually, and can't return the sameNulError
that comes fromCString
. So I've changed the error type slightly on Windows accordingly.This isn't a breaking change because this crate didn't build on Windows before anyway :)
Unix functionality is unaffected.
As a slight drive-by improvement, I fixed up the
std::error::Error
method implementations to remove the deprecateddescription()
implementation and usesource()
instead of the deprecatedcause()
.