Re-adding execvpe support, conditional on platform #683

panzertime · 2017-07-17T20:08:35Z

Fixes #682

Susurrus · 2017-07-17T22:22:17Z

You'll need to do a few more things:

replace super::ffi::execvpe with libc::execvpe
Change the last line to use Error::result like in the function above this one.
There should be good documentation for this function. See some of the other PRs that were recently merged for good examples.

Susurrus

Looking pretty good. Also needs a CHANGELOG entry and to have all commits except for the one that changes the libc dependency to be squashed into one (see the git rebase command).

Susurrus · 2017-07-18T17:03:31Z

src/unistd.rs

+    /// * `env`: array of environment variables for the new execution environment
+    /// * returns: Ok or libc error code.
+    ///
+    /// Err is returned if the executable file does not exist, cannot be accessed, or 


Instead of "Err" use "An error"

Susurrus · 2017-07-18T17:03:40Z

src/unistd.rs

        };

-        Err(Error::Sys(Errno::last()))
+	Errno::result(res).map(drop)


Indentation here is off by 1.

panzertime · 2017-07-18T17:39:20Z

I have made all the requested changes, but I cannot squash all the changes into two commits as described. I've tried git rebase -i HEAD~6 and marking everything except the first commit as squash, but then git tells me it can't do it

Susurrus · 2017-07-18T17:46:47Z

src/unistd.rs

+    /// Replaces the current process image with a new one.
+    /// ([see exec(3)](http://man7.org/linux/man-pages/man3/exec.3.html))
+    ///
+    /// * `filename`: filename of executable to run


Can you give a title or quick sentence to introduce this bulleted list? It doesn't read nicely just being a stand alone list.

Susurrus · 2017-07-18T17:46:48Z

src/unistd.rs

+    /// * `env`: array of environment variables for the new execution environment
+    /// * returns: Ok or libc error code.
+    ///
+    /// An error is returned if the executable file does not exist, cannot be accessed, or 


There is a special section called Errors that you should use for these kinds of things. See the book.

Susurrus · 2017-07-18T17:47:06Z

src/unistd.rs

+    /// * `filename`: filename of executable to run
+    /// * `args`: array of arguments as strings; first must be the filename
+    /// * `env`: array of environment variables for the new execution environment
+    /// * returns: Ok or libc error code.


This can be handled by the Errors section below.

Susurrus · 2017-07-18T18:01:36Z

I would like to get these commits squashed, you said you couldn't do that, is that because you aren't familiar with git rebase? Here's a quick overview if that's all you're missing.

panzertime · 2017-07-18T18:04:10Z

I followed the exact procedure described there and the rebase failed.

Susurrus · 2017-07-18T18:05:22Z

You'll need to fix merge commits when you rebase sometimes, so if there's a conflict it isn't a "failure" it just means you need to manually merge some of the code.

panzertime · 2017-07-18T18:10:52Z

Well, if that's the case, someone else needs to do it.

Susurrus · 2017-07-18T18:25:14Z

This isn't that bad to fix and it's probably worth getting a little bit more familiar with git if you're going to be doing more software dev work. And I've been where you are and had to struggle through it a bit, but the solution to this is pretty easy: undo all of your commits locally, rebase to the latest upstream/master (I'm assuming the primary repo is your upstream here), and then make your two commits.

This looks like:

git fetch upstream - Make sure you have the latest nix code to reference
git reset upstream/master - This will keep all your changes but unstage them and undo all of the commits. You'll now be at the same commit as upstream/master but with none of your changes applied.
git commit CHANGELOG.md -m"Switch back to using libc git"
`git commit -a -m"Re-add execvpe for Haiku, OpenBSD, and Linux"
git push origin -f - Overwrite the remote branch. Since this replaces existing commits you need to use the -f. Thinking about this now, I'm wondering if this isn't the failure you were talking about above.

Now, if more changes need to be made:

git commit -a --amend - Just amend the existing commit with any new changes.
git push origin -f - Since you'll have rewritten the git history, you'll need to force-push again here.

Let me know if you get stuck doing any of that. Hopefully it all makes sense now!

panzertime · 2017-07-18T18:35:58Z

fatal: 'upstream' does not appear to be a git repository

Susurrus · 2017-07-18T18:40:36Z

You may have it called "origin"; do git remote -v to see the names of things. I have "origin" set to my fork of a repository and I use "upstream" to refer to the upstream repo.

panzertime · 2017-07-18T18:45:32Z

There was no remote set for the upstream repo.

Susurrus · 2017-07-18T18:46:56Z

Yeah, the default would be to have "origin" set to your repo. So let's add an upstream:

git remote add upstream https://github.com/nix-rust/nix
git fetch upstream

panzertime · 2017-07-18T18:47:31Z

I already did that.

Susurrus · 2017-07-18T18:48:43Z

What's the output of git remote -v for you then?

panzertime · 2017-07-18T18:49:33Z

Have you noticed that there are new commits?

Susurrus · 2017-07-18T18:56:16Z

Oh, perfect, I did not. I've been squatting on this page and GitHub's terrible at dealing with force-pushing things, so I was still seeing 12 commits or whatever.

So this wraps in some changes that are part of the commits in #681 into your own commits, which isn't ideal, but fine. What we'll do is wait until that's merged, rebase this (which will get rid of all of those code changes) and then this'll be ready to merge!

Thanks for sticking with this, git can be really frustrating sometimes, but I've found it quite powerful now that I've really gotten the hang of it.

Susurrus · 2017-07-25T00:27:06Z

@panzertime Want to rerebase this and clean up the remaining issues? This is pretty close to merging.

panzertime · 2017-07-25T14:46:22Z

What issues remain?

Susurrus · 2017-07-25T15:21:55Z

CHANGELOG.md

@@ -24,6 +24,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  and nix::Error::UnsupportedOperation}`
  ([#614](https://github.com/nix-rust/nix/pull/614))
 - Added `cfmakeraw`, `cfsetspeed`, and `tcgetsid`. ([#527](https://github.com/nix-rust/nix/pull/527))
+- Readded execvpe support, conditional on platform.  libc now provides this for Haiku, Linux, and OpenBSD.


Change to "Added unistd::execvpe for Haiku, Linux, and OpenBSD."

Susurrus · 2017-07-25T15:23:46Z

Just the CHANGELOG entry should be more clear & concise. Otherwise this needs a rebase and then it should be good. Though I want to take another look over it once it's rebased as it's hard to tell what the actual changes are here since there are some other things that seem mixed in here.

Susurrus · 2017-08-01T21:39:00Z

@panzertime Just pinging you on this since we were getting pretty close to merging this. Can you rebase and address my comments?

Susurrus · 2017-09-05T01:02:04Z

@panzertime It's been a month since I last pinged you, so I wanted to do that again and see if we could get this merged.

Susurrus · 2017-11-18T02:12:50Z

@panzertime I think it's just the changelog entry now and a rebase and this is good to go. Still interested in merging this?

Susurrus · 2017-12-04T05:14:02Z

It's been almost 5 months on this with several pings, so I'm going to go ahead and close this PR.

Susurrus suggested changes Jul 18, 2017

View reviewed changes

panzertime added 2 commits July 18, 2017 14:44

Switch back to using libc git

f7eec4e

Readd execvpe for Haiku, Linux, OpenBSD

6d74714

Susurrus suggested changes Jul 25, 2017

View reviewed changes

Susurrus closed this Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-adding execvpe support, conditional on platform #683

Re-adding execvpe support, conditional on platform #683

panzertime commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus left a comment

Susurrus Jul 18, 2017

Susurrus Jul 18, 2017

panzertime commented Jul 18, 2017 •

edited

Loading

Susurrus Jul 18, 2017

Susurrus Jul 18, 2017

Susurrus Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

Susurrus commented Jul 25, 2017

panzertime commented Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus commented Jul 25, 2017

Susurrus commented Aug 1, 2017

Susurrus commented Sep 5, 2017

Susurrus commented Nov 18, 2017

Susurrus commented Dec 4, 2017

Re-adding execvpe support, conditional on platform #683

Re-adding execvpe support, conditional on platform #683

Conversation

panzertime commented Jul 17, 2017

Susurrus commented Jul 17, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

panzertime commented Jul 18, 2017 • edited Loading

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

Susurrus Jul 18, 2017

Choose a reason for hiding this comment

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

panzertime commented Jul 18, 2017

Susurrus commented Jul 18, 2017

Susurrus commented Jul 25, 2017

panzertime commented Jul 25, 2017

Susurrus Jul 25, 2017

Choose a reason for hiding this comment

Susurrus commented Jul 25, 2017

Susurrus commented Aug 1, 2017

Susurrus commented Sep 5, 2017

Susurrus commented Nov 18, 2017

Susurrus commented Dec 4, 2017

panzertime commented Jul 18, 2017 •

edited

Loading