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

make execvpe available for all platforms #349

Closed
wants to merge 1 commit into from

Conversation

Mic92
Copy link
Contributor

@Mic92 Mic92 commented Apr 13, 2016

  • execvpe is as GNU extension of libc.
  • redone in Rust to make it available on all systems.
  • this also makes the feature flag obsolete.

NOTE:

  • this my first contribution to a rust project, so watch out for beginner's mistake :)
  • is there a way to deprecate feature flags, so the code will still compile with a warning?

@homu
Copy link
Contributor

homu commented Apr 14, 2016

☔ 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.
@Mic92
Copy link
Contributor Author

Mic92 commented Apr 14, 2016

done

@Mic92
Copy link
Contributor Author

Mic92 commented Apr 17, 2016

anything else left?

@kamalmarhubi
Copy link
Member

Hi @Mic92 sorry for the delay. Taking a quick look!

@kamalmarhubi
Copy link
Member

  • this my first contribution to a rust project, so watch out for beginner's mistake :)

Yay! And thanks for picking our project!

  • is there a way to deprecate feature flags, so the code will still compile with a warning?

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"),
Copy link
Member

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?

Copy link
Contributor Author

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

@kamalmarhubi
Copy link
Member

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?

@kamalmarhubi
Copy link
Member

This function also does more work than most nix functions. It's not wrapping an existing library function but actually reimplementing it.

@Mic92
Copy link
Contributor Author

Mic92 commented Apr 18, 2016

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
and requires users to set a feature flag, which will not work on any platform.

@arcnmx
Copy link
Contributor

arcnmx commented Apr 18, 2016

This function also does more work than most nix functions. It's not wrapping an existing library function but actually reimplementing it.

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 / alloca to avoid allocations when possible.

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.

Note that this applies to the whole exec family of functions, which do need a bit of a revamp. execvpe isn't necessarily unique here, though it has bitrot in particular because the CI doesn't test that feature flag. I'd been holding off on fixing them while waiting for #230 to be resolved, but just threw up #358 now with some of my old work to gather feedback.

@Mic92
Copy link
Contributor Author

Mic92 commented Apr 21, 2016

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.

@Mic92 Mic92 closed this Apr 21, 2016
@Artoria2e5
Copy link

It might make sense to move this code out as a separate crate (in the new signature of course)

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.

None yet

5 participants