std.process: Add workDir parameter

[CyberShadow]

https://d.puremagic.com/issues/show_bug.cgi?id=11363

Sometimes, it is useful to create a new process with a working directory different from the parent process. For example, git submodule and git bisect insist on being run from the repository root.

This currently cannot be done in a nice way when using std.process:

• Setting the current directory just before calling the program is not thread-safe, since the current directory is shared across all threads.
• Although one can run the target program from a shell, and prefix the command with "cd " ~ escapeShellFilename(cwd) ~ " && ", this requires spawning a shell, which has a non-negligible performance cost on Windows.

This change adds yet another default parameter to the overloads of the 6 relevant functions. I don't see a better way without e.g. replacing the overloads with a function that takes a tuple and looking at each argument's types, similar to what std.socket.getAddressInfo does.

[CyberShadow]

I'm not sure about this change. If the chdir fails, the exception will be thrown in the forked process. Even if we use the C function, we still need to communicate failure somehow to the original process. Checking for the directory's existence in the main process is out as it would be a race condition.

[CyberShadow]

@kyllingstad @schveiguy

[CyberShadow]

Just stumbled upon fchdir, and reimplemented the POSIX side to use it. Explained in comments.

[JakobOvrum]

It looks like this file descriptor can leak if some error occurs before the two calls to close. Maybe refactor it in terms of a transaction, i.e. with scope(exit) or similar?

[kyllingstad]

Apart from the file descriptor leak that @JakobOvrum discovered, this looks good to me. It kind of sucks that we have to add another parameter when there are already so many, but I think it's worth it in this case.

[kyllingstad]

Not too happy about the cwd name, though. It doesn't refer to the "current working directory", it refers to a different working directory for the new process. I suggest changing it to workDir or something similar.

[CyberShadow]

Thanks! Addressed all concerns and rebased.

[JakobOvrum]

Does it need the immutable guarantee? If not, in char[].

[CyberShadow]

Fixed! We convert the string to something else anyway, which involves a reallocation, so parameter constness doesn't matter.

[JakobOvrum]

LGTM.

I like this solution, it looks quite robust.

[kyllingstad]

Agreed. However, since this is an API change, I'd like to wait a couple of days and see if @schveiguy has any comments before we merge it.

[CyberShadow]

Ping? @schveiguy ?

[schveiguy]

2 things:

1. I had a shitty mail filter that was pushing all github traffic to a neglected folder on my email, so I missed this and I apologize (now filter will put emails with @schveiguy in my inbox, so I got the latest ping)
2. I have a staff meeting shortly, and after that I will look at this.

[schveiguy]

Looks good to me, not a breaking API change from what I can tell. One thing that is a little concerning, but not enough to cancel the pull request, is the ordering of the parameters is sort of random, and pretty much arbitrary.

Wondering if having a variadic template at least for the last 3 arguments (which are independently typed) would be a useful way of allowing one to specify only the things you need. We already forward to an implementation function, this would just be for the wrappers. (not for this pull request, of course, but a different one).

[kyllingstad]

Auto-merge toggled on

[kyllingstad]

All right, since we all agree, I've scheduled this for an auto merge.

@schveiguy:

Wondering if having a variadic template at least for the last 3 arguments (which are independently typed) would be a useful way of allowing one to specify only the things you need.

Maybe not a bad idea. I can't recall seeing this design used anywhere else in Phobos, though. (This is one of the very few times I've wished we had named parameters in D.)

[CyberShadow]

I can't recall seeing this design used anywhere else in Phobos, though.

As already mentioned in this thread, std.socket.getAddressInfo does this.

[kyllingstad]

Oh, you're right. That's what happens when a conversation stretches out over several weeks – I forget what was said in the beginning. ;-)

[schveiguy]

As already mentioned in this thread, std.socket.getAddressInfo does this.

I think it's a decent idea when functions get out of control with parameters and each parameter can be identified by its type. Tango worked around this by creating an object and then using methods to configure it before calling the execute method, but not an option here.

[kyllingstad]

However, if we go with that design, we're in trouble if we later want to add another string parameter, for example.

I guess we could look at process creation modules in other languages and libraries—Python, Qt, Tango, Win32, etc.—and make an exhaustive list of all possible parameters we might ever want to add to spawnProcess(). If that list turns out to contain only parameters that are best represented with different types, then we're good.

[schveiguy]

Good point. Maybe we should hold off on that idea then.