-
Notifications
You must be signed in to change notification settings - Fork 681
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
make execvpe available for all platforms #349
Conversation
☔ The latest upstream changes (presumably #351) made this pull request unmergeable. Please resolve the merge conflicts. |
- execvpe is as GNU extension of libc. - redone in Rust to make it available on all systems. - this also makes the feature flag obsolete.
done |
anything else left? |
Hi @Mic92 sorry for the delay. Taking a quick look! |
Yay! And thanks for picking our project!
I'm not sure. I haven't messed with Cargo features that much! |
|
||
let paths = match env::var_os("PATH") { | ||
Some(val) => val, | ||
None => OsString::from("/usr/local/bin:/bin:/usr/bin"), |
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.
Is this default specified somewhere?
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 took the same value as musl libc.
If you want to stick to the POSIX standard, it is only "/bin/:usr/bin" according to confstr(3).
Ok having taken a look: I've always been unsure of this function in nix, and have considered proposing its removal. In particular, it doesn't match the string passing style of other parts of the library. On the other hand, it is somewhat convenient. I'd like feedback from a couple of other maintainers on how they feel about it. @fiveop @posborne? |
This function also does more work than most nix functions. It's not wrapping an existing library function but actually reimplementing it. |
The reason I have not using an existing one is, because it is not available in all libc implementations. My first version was using the libc one, but it is actually more code |
Yeah, this makes me wary about including it as more than just a libc wrapper... Also that one important difference between this and the glibc version is that glibc will use the stack /
Note that this applies to the whole |
I was already wondering, why exec* wants CString. Stack allocated arrays is something what I do not want to implement. Well, then I use my own implementation of execvpe. |
It might make sense to move this code out as a separate crate (in the new signature of course) |
NOTE: