Issue 15276: Allow users to specify shell for executeShell, pipeShell, spawnShell #3900
Conversation
|
| Cmd commandLine, | ||
| const string[string] env = null, | ||
| Config config = Config.none, | ||
| size_t maxOutput = size_t.max, | ||
| in char[] workDir = null) | ||
| in char[] workDir = null, | ||
| TList values = TList.init) |
There was a problem hiding this comment.
The default argument here is meaningless AFAICS.
I think the use of variadics makes sense since it's just forwarding a number of arguments to the generic pipeFunc. It would probably have taken me less time to understand if it was named something proper like ExtraArgs extraArgs though.
There was a problem hiding this comment.
The default argument is unfortunately necessary because:
- This must be the very last argument in the function because it is an unknown quantity of optional arguments
- There are arguments before it that have default arguments; anything after an argument with a default value must also have a default value.
|
Overall looks good to me. |
…, spawnShell This diff: This diff makes it possible to specify your preferred shell for the various `*shell` functions in `std.process`. By default it uses the new `nativeShell` function. `nativeShell` to always returns the native system shell (see the "some history" section for why this is the default value). I chose to create `nativeShell` rather than changeing `userShell` because I think it is still valuable to have something like `userShell` around in Phobos. The one part of this diff I'm not super thrilled by is the use of variadic arguments (easier to see than to explain); if you have a suggestion for something better I'd love to hear it :). Note: The default shell is a breaking change for Windows, which probably *should have* been broken same as posix in the diffs listed in the history section below, but I suspect was overlooked. Find rationale behind making a breaking change at all here: https://issues.dlang.org/show_bug.cgi?id=15000. Some history: a524a35 and f537cb5 Tried to move us from a world where we preferred the shell specified in the SHELL environment variable to a world where we always went with the native system shell, /bin/sh for posix. The former of those revisions was reverted in: 5b2b1fb Because the documentation was not updated to reflect the change. Presently: we are in a state of schism, whereby `userShell` has behavior that does not align with what is actually used in `executeShell`, `pipeShell`, and `spawnShell`. This is something we *must* resolve. Personal pain: This bit me when one of my scripts at work stopped working because it dependend on features that don't exist in `sh` (but do in `bash`, `zsh`, etc). It was really frustrating that there was no simple way to work around this and specify my preferred shell; it is, of course, possible to leverage the various `escape*` functions from `std.process` and `spawnProcess` to use whichever shell I prefer, but this seems like something warranting first class support. I was sad to not have something like this after a breaking change.
|
Thanks for the review @JakobOvrum :). Updated the diff to reflect your suggestions. |
|
Auto-merge toggled on |
|
thx! |
Issue 15276: Allow users to specify shell for executeShell, pipeShell, spawnShell
| variable, if it exists and is non-empty. Otherwise, it returns | ||
| $(D "/bin/sh"). | ||
| variable, if it exists and is non-empty. Otherwise, it returns the result of | ||
| $(LREF nativeShell). |
There was a problem hiding this comment.
This comment says "it returns the result of nativeShell," but the various shells are actually hard-coded in: any reason why? I'm preparing a PR for Android, any reason not to have the code below actually do what the comment says? If not, I'll make userShell call nativeShell, so we don't have the same list of shells duplicated in two methods.
There was a problem hiding this comment.
Feel free to change.
|
I know I'm late with this, but I think this pull request should not have been merged. I kept thinking back on it since I learned about it, and just now formulated my thoughts on it. Standard library modules which interface with the OS, such as std.process, attempt to fulfill the following needs:
This PR doesn't fix any of the above problems.
Yes, the goal is to do what the C
The breakage is unfortunate, but it also indicates incorrect code. Essentially, it means that your code depended on a user preference of the machine it's running on. If your code needs to run commands with non-POSIX shell syntax, then you must explicitly specify the interpreter. And yes, this is what this PR is supposed to address. However...
No
This is subjective, and I disagree. I don't feel strongly about this though I would like to see the functionality added in this PR deprecated and removed, barring any mistakes in my understanding of the situation. The functions in this module have too many overloads and parameters already. BTW, looking forward to automatic notification of when someone submits a PR against your code. I wish I was notified for this one... |
This diff: This diff makes it possible to specify your preferred shell for the various
*shellfunctions instd.process. By default it uses the newnativeShellfunction.nativeShellto always returns the native system shell (see the "some history" section for why this is the default value). I chose to createnativeShellrather than changeinguserShellbecause I think it is still valuable to have something likeuserShellaround in Phobos.The one part of this diff I'm not super thrilled by is the use of variadic arguments (easier to see than to explain); if you have a suggestion for something better I'd love to hear it :).
Note: The default shell is a breaking change for Windows, which probably should have been broken same as posix in the diffs listed in the history section below, but I suspect was overlooked. Find rationale behind making a breaking change at all here: https://issues.dlang.org/show_bug.cgi?id=15000.
Some history:
a524a35
and
f537cb5
Tried to move us from a world where we preferred the shell specified in the SHELL environment variable to a world where we always went with the native system shell, /bin/sh for posix. The former of those revisions was reverted in:
5b2b1fb
Because the documentation was not updated to reflect the change.
Presently: we are in a state of schism, whereby
userShellhas behavior that does not align with what is actually used inexecuteShell,pipeShell, andspawnShell. This is something we must resolve.Personal pain: This bit me when one of my scripts at work stopped working because it dependend on features that don't exist in
sh(but do inbash,zsh, etc). It was really frustrating that there was no simple way to work around this and specify my preferred shell; it is, of course, possible to leverage the variousescape*functions fromstd.processandspawnProcessto use whichever shell I prefer, but this seems like something warranting first class support. I was sad to not have something like this after a breaking change.