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 for Windows #4

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Implement for Windows #4

wants to merge 5 commits into from

Conversation

wfraser
Copy link

@wfraser wfraser commented Jun 24, 2020

Windows has the _wexecvp function in the C runtime which is the same as Unix's execvp but takes Windows wide strings for arguments instead.

The biggest difference is that instead of using CString to make FFI-ready strings, we use std::os::windows::ffi::OsStrExt::encode_wide which builds a Vec<u16> for us, but we need to check for interior nulls manually, and can't return the same NulError that comes from CString. 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 deprecated description() implementation and use source() instead of the deprecated cause().

@wfraser
Copy link
Author

wfraser commented Jun 24, 2020

re: #3

Copy link
Contributor

@emk emk left a 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:

  1. 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?
  2. 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?
  3. 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.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@wfraser
Copy link
Author

wfraser commented Jun 25, 2020

Thanks for the review! I'll address your main points:

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 was in the process of upgrading a program which used this crate, and discovered precisely what you point out, that the functionality in std is Unix-only, which means this crate has an opportunity to implement cross-platform functionality beyond what the standard library provides, making it useful again! :)

1. The [docs say that](https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/execvp-wexecvp?view=vs-2019) "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?

Windows is one platform as far as Rust's cfg flags are concerned, even though Win32 and UWP are quite different. The main difference is that programs get linked with different libraries for the different environments. Win32 programs get the C runtime and other win32 DLLs, whereas UWP gets a whole different set of things.

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 _wexecvp function.

2. 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?

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.

3. 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?

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.
@wfraser
Copy link
Author

wfraser commented Jun 26, 2020

I've added wexecvp (and family) to libc. Once it is released, I can remove it from here and update the libc dependency.

@emk
Copy link
Contributor

emk commented Jul 5, 2020

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.

@gustavosimon
Copy link

gustavosimon commented Nov 17, 2021

Congratulations to developed this wrapper about the execvp function. I used it in a project, and it worked perfectly.
The use of the lib is very simple.
But, I faced a limit that the lib just work to Linux OS. So I found this PR implementing the function to Windows OS.
Do you have any prevision to merge this PR and homologate the changes to use in production?

@emk emk self-assigned this Dec 16, 2021
@gustavosimon
Copy link

@emk have you a prevision to review this PR?

@gustavosimon
Copy link

@wfraser @emk

@Mart-Bogdan Mart-Bogdan mentioned this pull request Dec 20, 2023
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