www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - deprecation message std.process.system=>executeShell often not what's

reply Timothee Cour via Digitalmars-d <digitalmars-d puremagic.com> writes:
The deprecation message in 2.066(rc): 'Deprecation: function
std.process.system is deprecated - Please use executeShell instead' is
often not what user wants.

The behavior of the deprecated std.process should most closely be:

int systemNew(in char[] args){
auto pipes = pipeShell(args, cast(Redirect)0);
return wait(pipes.pid);
}

which could be a function in std.process if we deprecate std.process.system
(possibly with a few extra parameters such as environment etc).

Indeed, system doesn't capture output (which could potentially require lots
of memory), and doesn't wait until completion, unlike executeShell.
To make a user's code upgrade from std.process, he most likely will want
something like systemNew instead.
Aug 12 2014
next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Tuesday, 12 August 2014 at 07:53:44 UTC, Timothee Cour via 
Digitalmars-d wrote:
 int systemNew(in char[] args){
 auto pipes = pipeShell(args, cast(Redirect)0);
 return wait(pipes.pid);
 }
Why use pipeShell instead of spawnShell? And why `cast(Redirect)0`? It's not necessary. Also note that shell commands (incl. system) take one string argument, not an array. This can be a simpler one-liner: wait(executeShell(cmd)) So, the deprecation message is not far off, perhaps missing a `.wait()`.
 Indeed, system [..] doesn't wait until completion, unlike 
 executeShell.
You mean it does wait until completion?
Aug 12 2014
parent reply Timothee Cour via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Tue, Aug 12, 2014 at 3:41 PM, Vladimir Panteleev via Digitalmars-d <
digitalmars-d puremagic.com> wrote:

 On Tuesday, 12 August 2014 at 07:53:44 UTC, Timothee Cour via
 Digitalmars-d wrote:

 int systemNew(in char[] args){
 auto pipes = pipeShell(args, cast(Redirect)0);
 return wait(pipes.pid);
 }
Why use pipeShell instead of spawnShell?
spawnShell works too.
 And why `cast(Redirect)0`? It's not necessary.
it is, otherwise it defaults to Redirect.all which has different semantics than 'system' (so it's not a replacement of it). Btw, I don't like the cast either; shouldn't there be a 'none=0' value in the enum?
 Also note that shell commands (incl. system) take one string argument, not
 an array.
Please re-read my post carefully. pipeShell indeed takes 'in char[] command', not string (char array, not string array nor string).
 This can be a simpler one-liner: wait(executeShell(cmd))
Please re-read my original post more carefully. This is not what 'system' does. system doesn't redirect stdin/stdout. executeShell on the other hand does this: "The functions capture what the child process prints to both its standard output and standard error streams, and return this together with its exit code".
 So, the deprecation message is not far off, perhaps missing a `.wait()`.
The deprecation message is indeed wrong. It should be wait(spawnShell(cmd)) instead of executeShell(cmd). Different function and missing wait.
  Indeed, system [..] doesn't wait until completion, unlike executeShell.

 You mean it does wait until completion?
Bad wording; what I meant is we have to wait until completion to get any stdout output with executeShell (it returns it) whereas for system/systemNew/wait(spawnShell) the output is displayed in realtime via stdout.
Aug 12 2014
next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Wednesday, 13 August 2014 at 01:58:15 UTC, Timothee Cour via 
Digitalmars-d wrote:
 On Tue, Aug 12, 2014 at 3:41 PM, Vladimir Panteleev via 
 Digitalmars-d <
 digitalmars-d puremagic.com> wrote:

 And why `cast(Redirect)0`? It's not necessary.
it is, otherwise it defaults to Redirect.all which has different semantics than 'system' (so it's not a replacement of it). Btw, I don't like the cast either; shouldn't there be a 'none=0' value in the enum?
Not necessary, because pipeShell("foo", Redirect.none) would be equivalent to spawnShell("foo").
Aug 12 2014
prev sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Wednesday, 13 August 2014 at 01:58:15 UTC, Timothee Cour via 
Digitalmars-d wrote:
 Please re-read my post carefully. pipeShell indeed takes 'in 
 char[]
 command', not string (char array, not string array nor string).
Right. Sorry, shouldn't forum tired.
Aug 13 2014
prev sibling parent reply "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Tuesday, 12 August 2014 at 07:53:44 UTC, Timothee Cour via 
Digitalmars-d wrote:
 The deprecation message in 2.066(rc): 'Deprecation: function
 std.process.system is deprecated - Please use executeShell 
 instead' is
 often not what user wants.
Wait, what? The pull request that deprecated system() was only merged a few days ago! How can it already be in a release candidate? I thought that once the beta cycle started only bug fixes would be incorporated. Deprecations are breaking changes! But anyway, I agree that the deprecation message is somewhat off the mark. The expression that most closely matches system(cmd) is wait(spawnShell(cmd)).
 The behavior of the deprecated std.process should most closely 
 be:

 int systemNew(in char[] args){
 auto pipes = pipeShell(args, cast(Redirect)0);
 return wait(pipes.pid);
 }

 which could be a function in std.process if we deprecate 
 std.process.system
 (possibly with a few extra parameters such as environment etc).
No, there is no point in using pipeShell() if you're not redirecting the streams. Then you should use spawnShell(). pipeXyz() is just spawnXyz() plus automatic pipe creation. wait(spawnShell(cmd)) is simple enough that I don't think it needs to be a separate function in std.process.
 Indeed, system doesn't capture output (which could potentially 
 require lots
 of memory) [...]
Note that you can set executeShell's maxOutput parameter to zero, in which case it will not collect any output and thus not consume any memory. But it still swallows the output, which you are right that system() does not. I will submit a pull request that changes the deprecation message. Thanks for pointing it out! Lars
Aug 12 2014
parent reply Timothee Cour via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Tue, Aug 12, 2014 at 11:09 PM, Lars T. Kyllingstad via Digitalmars-d <
digitalmars-d puremagic.com> wrote:

 On Tuesday, 12 August 2014 at 07:53:44 UTC, Timothee Cour via
 Digitalmars-d wrote:

 The deprecation message in 2.066(rc): 'Deprecation: function
 std.process.system is deprecated - Please use executeShell instead' is
 often not what user wants.
Wait, what? The pull request that deprecated system() was only merged a few days ago! How can it already be in a release candidate? I thought that once the beta cycle started only bug fixes would be incorporated. Deprecations are breaking changes!
clarification: actually that was from git master, not from rc.
 But anyway, I agree that the deprecation message is somewhat off the mark.
  The expression that most closely matches system(cmd) is
 wait(spawnShell(cmd)).


  The behavior of the deprecated std.process should most closely be:
 int systemNew(in char[] args){
 auto pipes = pipeShell(args, cast(Redirect)0);
 return wait(pipes.pid);
 }

 which could be a function in std.process if we deprecate
 std.process.system
 (possibly with a few extra parameters such as environment etc).
No, there is no point in using pipeShell() if you're not redirecting the streams. Then you should use spawnShell(). pipeXyz() is just spawnXyz() plus automatic pipe creation. wait(spawnShell(cmd)) is simple enough that I don't think it needs to be a separate function in std.process. Indeed, system doesn't capture output (which could potentially require
 lots
 of memory) [...]
Note that you can set executeShell's maxOutput parameter to zero, in which case it will not collect any output and thus not consume any memory. But it still swallows the output, which you are right that system() does not. I will submit a pull request that changes the deprecation message. Thanks for pointing it out! Lars
Aug 12 2014
parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Wednesday, 13 August 2014 at 06:29:11 UTC, Timothee Cour via 
Digitalmars-d wrote:
 On Tue, Aug 12, 2014 at 11:09 PM, Lars T. Kyllingstad via 
 Digitalmars-d <
 digitalmars-d puremagic.com> wrote:

 On Tuesday, 12 August 2014 at 07:53:44 UTC, Timothee Cour via
 Digitalmars-d wrote:

 The deprecation message in 2.066(rc): 'Deprecation: function
 std.process.system is deprecated - Please use executeShell 
 instead' is
 often not what user wants.
Wait, what? The pull request that deprecated system() was only merged a few days ago! How can it already be in a release candidate? [...]
clarification: actually that was from git master, not from rc.
Ok, that's good. Here's a fix anyway: https://github.com/D-Programming-Language/phobos/pull/2422
Aug 12 2014