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