www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Formal Review of std.process

reply "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
Hello,

I have decided to take on Review Manager for std.process and 
hopefully will keep it up and get through the review queue. This 
is slightly ignoring a "first come, first serve" approach, but 
that is mostly because I didn't want to look into it. (So don't 
be too upset).

std.process has been under review for some time having discussion 
on the pull request and this thread:
http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue forum.dlang.org

I would like to propose the Formal review is one week (and one 
week voting). If there is objection speak up.

Docs:
http://www.kyllingen.net/code/std-process2/phobos-prerelease/std_process.html

Source:
https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d

Pull:
https://github.com/D-Programming-Language/phobos/pull/1151
Apr 04 2013
next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
Oops, forgot the project description:

std.process is improvements to the existing std.process and is a 
complete change to the API. The original API remains but these 
will be going through deprecation.

The major change is support for redirecting stdin/stdout/stderr

For those interested I took the unittests from the original 
std.process and modified them to the new API (back when it was 
under std.process2) so this diff gives an idea for changes (not 
done for Windows sorry).

https://gist.github.com/JesseKPhillips/5298688/revisions
Apr 04 2013
prev sibling next sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
 Source:
 https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d

Dead link.
 Docs:
 http://www.kyllingen.net/code/std-process2/phobos-prerelease/std_process.html

<quote> Unless a directory is specified in args[0] or program, spawnProcess will search for the program in the directories listed in the PATH environment variable. To run an executable in the current directory, use "./executable_name". </quote> This does not apply for Windows, so it should state that.
Apr 04 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-04-05 07:37, Lars T. Kyllingstad wrote:

 In light of this, here's a suggestion:  How about we, rather than
 updating the old std.process, create a new module called
 std.sys.process?  Or even better, IMO, std.sys.environment and
 std.sys.pipe as well as std.sys.process?

I like that, to have a "sys" package. -- /Jacob Carlborg
Apr 04 2013
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
 Docs:
 http://www.kyllingen.net/code/std-process2/phobos-prerelease/std_process.html

<quote> Win32-specific warning: The mechanisms for process termination are incredibly badly specified in Win32. This function may therefore produce unexpected results, and should be used with the utmost care. </quote> It then links to a note about XP. So does this only apply to XP or all Windows systems? "win32" isn't very specific, does it not apply for 64bit systems?
Apr 04 2013
prev sibling next sibling parent "Jesse Phillips" <Jessekphillips+D gmail.com> writes:
Actual Code location.

Source:
https://github.com/kyllingstad/phobos/blob/std-process2/std/process.d
Apr 04 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Thu, 04 Apr 2013 12:50:01 -0400, Andrej Mitrovic  
<andrej.mitrovich gmail.com> wrote:

 On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
 Source:
 https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d

Dead link.

https://github.com/kyllingstad/phobos/blob/std-process2/std/process.d At the last minute I insisted we change std.process2 to std.process since it was agreed to incorporate the original API instead of redesigning it slightly. -Steve
Apr 04 2013
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
On Thu, Apr 04, 2013 at 01:04:52PM -0400, Steven Schveighoffer wrote:
 On Thu, 04 Apr 2013 12:50:01 -0400, Andrej Mitrovic
 <andrej.mitrovich gmail.com> wrote:
 
On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
Source:
https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d

Dead link.

https://github.com/kyllingstad/phobos/blob/std-process2/std/process.d At the last minute I insisted we change std.process2 to std.process since it was agreed to incorporate the original API instead of redesigning it slightly.

Hooray! Looking forward to *not* having "std.process2". Didn't like that name from the start. I'll take a closer look at the code/docs later, for the review. T -- Государство делает вид, что платит нам зарплату, а мы делаем вид, что работаем.
Apr 04 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 4 April 2013 at 16:50:15 UTC, Andrej Mitrovic wrote:
 <quote>
 Unless a directory is specified in args[0] or program, 
 spawnProcess
 will search for the program in the directories listed in the 
 PATH
 environment variable. To run an executable in the current 
 directory,
 use "./executable_name".
 </quote>

 This does not apply for Windows, so it should state that.

It does apply to Windows. It just isn't necessary to do it within std.process, as Windows' own CreateProcess() takes care of it. It is a bit imprecise, though, because CreateProcess() does in fact search a lot of other directories first (current working dir, parent process dir, system32, etc.). In fact, PATH is last on that list. ;) I'll clarify this in the documentation. Lars
Apr 04 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 4 April 2013 at 16:57:58 UTC, Andrej Mitrovic wrote:
 On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
 Docs:
 http://www.kyllingen.net/code/std-process2/phobos-prerelease/std_process.html

<quote> Win32-specific warning: The mechanisms for process termination are incredibly badly specified in Win32. This function may therefore produce unexpected results, and should be used with the utmost care. </quote> It then links to a note about XP. So does this only apply to XP or all Windows systems? "win32" isn't very specific, does it not apply for 64bit systems?

Did you read the article? In the second paragraph it says: "Many of the details of how processes exit are left unspecified in Win32, so different Win32 implementations can follow different mechanisms. For example, Win32s, Windows 95, and Windows NT all shut down processes differently." BTW, this is written by one of Microsoft's Windows developers, so I'm assuming it to be authoritative. I don't know whether it applies to Win64, though. I've tried to investigate, but I couldn't find anything. I agree the docs should probably be changed to "Windows APIs" rather than "Win32". Lars
Apr 04 2013
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 4/4/13, Lars T. Kyllingstad <public kyllingen.net> wrote:
 Did you read the article?

I did some years ago. There seem to be /some/ workarounds in various places on the web, with alternatives to TerminateProcess, but it's usually hacky. I don't see this as a blocker personally, we can investigate for a safer implementation later.
Apr 04 2013
prev sibling next sibling parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
04-Apr-2013 18:22, Jesse Phillips пишет:
 Hello,

 I have decided to take on Review Manager for std.process and hopefully
 will keep it up and get through the review queue. This is slightly
 ignoring a "first come, first serve" approach, but that is mostly
 because I didn't want to look into it. (So don't be too upset).

At last! And being somewhere near the top of the queue with std.uni I have no objections. Let's just make some progress :) -- Dmitry Olshansky
Apr 04 2013
prev sibling next sibling parent "Kagamin" <spam here.lot> writes:
escapeShellCommand, escapeWindowsArgument, escapeShellFileName - 
in which way differ and why it's not one function? Why 
escapeWindowsArgument exists without posix counterpart?
Apr 04 2013
prev sibling next sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Thursday, 4 April 2013 at 21:59:01 UTC, Kagamin wrote:
 escapeShellCommand, escapeWindowsArgument, escapeShellFileName 
 - in which way differ and why it's not one function?

Because they do different things. The escaping rules are different. See the example on escapeShellCommand for how to use them.
 Why escapeWindowsArgument exists without posix counterpart?

escapeWindowsArgument is needed for building e.g. DMD response files on any platform. escapePosixArgument exists, but it is private/undocumented. It could be made public if someone requests it. Note that these functions aren't new - I've written them for the old std.process a while ago.
Apr 04 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 4 April 2013 at 17:04:53 UTC, Steven Schveighoffer 
wrote:
 On Thu, 04 Apr 2013 12:50:01 -0400, Andrej Mitrovic 
 <andrej.mitrovich gmail.com> wrote:

 On 4/4/13, Jesse Phillips <Jesse.K.Phillips+D gmail.com> wrote:
 Source:
 https://github.com/kyllingstad/phobos/blob/std-process2/std/process2.d

Dead link.

https://github.com/kyllingstad/phobos/blob/std-process2/std/process.d At the last minute I insisted we change std.process2 to std.process since it was agreed to incorporate the original API instead of redesigning it slightly.

There was a recent thread [1] about deepening Phobos' module hierarchy, and it seems that most people agree this is something that should eventually happen. In light of this, here's a suggestion: How about we, rather than updating the old std.process, create a new module called std.sys.process? Or even better, IMO, std.sys.environment and std.sys.pipe as well as std.sys.process? This would also allow us to give developers an earlier heads-up that the old std.process is going the way of the dodo, by using a pragma(msg) in the module. (The functions in std.process are not templated, so we can't do the nice trick of putting the pragma(msg) inside the functions themselves.) Lars [1] http://forum.dlang.org/thread/ugmacrokqghrrwpfovam forum.dlang.org
Apr 04 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
The following is a message posted by Johannes Pfau to the other 
(pre-)review thread which didn't seem to get noticed, along with 
an edited version of my reply.

On Sunday, 31 March 2013 at 13:14:52 UTC, Johannes Pfau wrote:
 Reposted from github:
 I think it would be nice if the high level functions would also 
 allow
 using custom environment variables. So the execute and 
 executeShell
 functions should have overloads which accept a string[string] 
 with
 environment variables (and probably accept a Config as well).

 The execute and executeShell functions would be trivial to 
 implement
 though if pipeProcess / pipeShell had an overload with 
 environment
 and Config parameters. So it's probably more important that
 pipeProcess / pipeShell get these overloads.

I have to say, I am really warming to the idea. This would come at no cost to the actual usage of these functions; you could still call execute like this: execute("my_app"); The cost would be in the form of more complex function signatures (and consequently documentation): ProcessPipes pipeProcess( string[] args, Redirect redirectFlags = Redirect.all, string[string] env = null, Config config = Config.none); Tuple execute( string[] args, string[string] env = null, Config config = Config.none); Implementation-wise, it is a trivial task to add this functionality to both pipeProcess/pipeShell and execute/executeShell. It's a simple matter of forwarding the extra arguments to spawnProcess, with some filtering of 'config'. Lars
Apr 04 2013
prev sibling next sibling parent "Tobias Pankrath" <tobias pankrath.net> writes:
 There was a recent thread [1] about deepening Phobos' module 
 hierarchy, and it seems that most people agree this is 
 something that should eventually happen.

 In light of this, here's a suggestion:  How about we, rather 
 than updating the old std.process, create a new module called 
 std.sys.process?  Or even better, IMO, std.sys.environment and 
 std.sys.pipe as well as std.sys.process?

Why std.sys? sys is used to group platform specific stuff in the runtime. So I would expect to have a std.sys.linux.process and a std.sys.posix.process etc.
Apr 04 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Friday, 5 April 2013 at 06:33:24 UTC, Tobias Pankrath wrote:
 There was a recent thread [1] about deepening Phobos' module 
 hierarchy, and it seems that most people agree this is 
 something that should eventually happen.

 In light of this, here's a suggestion:  How about we, rather 
 than updating the old std.process, create a new module called 
 std.sys.process?  Or even better, IMO, std.sys.environment and 
 std.sys.pipe as well as std.sys.process?

Why std.sys? sys is used to group platform specific stuff in the runtime. So I would expect to have a std.sys.linux.process and a std.sys.posix.process etc.

I'd prefer to have an std.process package, but that is currently not possible. I agree std.sys is a bit misleading. Nevermind my suggenstion, then. If DIP16 gets implemented, we can always split std.process into std.process.manage, std.process.environment, std.process.old, etc. Lars
Apr 06 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
I wonder if we should change the name of Config.gui to 
Config.noConsole.  It corresponds to the CREATE_NO_WINDOW flag in 
the Windows API, for which the documentation says:

"The process is a console application that is being run without a 
console window. Therefore, the console handle for the application 
is not set. This flag is ignored if the application is not a 
console application [...]"

I know some people don't like negative flags, but in this case it 
seems more precise.  It doesn't create a GUI, it prevents the 
creation of a console.

Lars
Apr 06 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Sat, 06 Apr 2013 05:13:10 -0400, Lars T. Kyllingstad  
<public kyllingen.net> wrote:

 I wonder if we should change the name of Config.gui to  
 Config.noConsole.  It corresponds to the CREATE_NO_WINDOW flag in the  
 Windows API, for which the documentation says:

 "The process is a console application that is being run without a  
 console window. Therefore, the console handle for the application is not  
 set. This flag is ignored if the application is not a console  
 application [...]"

 I know some people don't like negative flags, but in this case it seems  
 more precise.  It doesn't create a GUI, it prevents the creation of a  
 console.

This is true. Even the windows switch is misleading. The origin of the name 'gui' is from when one wanted to start a windows GUI application from another windows GUI application, and you did it without this flag, it would pop up an annoying console window. So you can read it as "I'm starting a GUI application" If we could, I'd fix Windows so the process you were starting made the determination of whether it should start a console or not, that makes more sense to me. As far as a positive flag, what about suppressConsole? Little verbose I suppose... -Steve
Apr 06 2013
prev sibling next sibling parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Saturday, 6 April 2013 at 15:26:01 UTC, Steven Schveighoffer 
wrote:
 The origin of the name 'gui' is from when one wanted to start a 
 windows GUI application from another windows GUI application, 
 and you did it without this flag, it would pop up an annoying 
 console window.  So you can read it as "I'm starting a GUI 
 application"

When is this true?
 If we could, I'd fix Windows so the process you were starting 
 made the determination of whether it should start a console or 
 not, that makes more sense to me.

Um, that's exactly how it works. There is a value in the PE header which determines this. The corresponding linker flag is /SUBSYSTEM:WINDOWS for a GUI program or /SUBSYSTEM:CONSOLE for a console program. The flag specifies that you want to run a console application, but want to suppress its console window.
Apr 06 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Sat, 06 Apr 2013 11:38:14 -0400, Vladimir Panteleev  
<vladimir thecybershadow.net> wrote:

 Um, that's exactly how it works. There is a value in the PE header which  
 determines this. The corresponding linker flag is /SUBSYSTEM:WINDOWS for  
 a GUI program or /SUBSYSTEM:CONSOLE for a console program.

 The flag specifies that you want to run a console application, but want  
 to suppress its console window.

Shit, I didn't remember this correctly at all! Yes, I remember now. It was for GUI applications that wanted to run a console application. Because there was no console window for the parent process, windows creates one. Like an application that wanted to run a script or something. I ran into this a million years ago when I was using Tango, and I had a GUI agent that ran programs on behalf of a remote process, and annoying console windows would pop up if the application was a script, seemingly at random. Sorry I misrepresented! In light of this, there really is no good reason it was called gui! I probably was this confused when I picked that name to begin with ;) -Steve
Apr 06 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
 On Sunday, 31 March 2013 at 13:14:52 UTC, Johannes Pfau wrote:
 Reposted from github:
 I think it would be nice if the high level functions would 
 also allow
 using custom environment variables.


I think that is probably a good idea. The exec functions allowed for this. I've never made use of it myself but if it can be done for all OSs we may as well.
Apr 06 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Saturday, 6 April 2013 at 15:26:01 UTC, Steven Schveighoffer 
wrote:
 On Sat, 06 Apr 2013 05:13:10 -0400, Lars T. Kyllingstad 
 <public kyllingen.net> wrote:
 I know some people don't like negative flags, but in this case 
 it seems more precise.  It doesn't create a GUI, it prevents 
 the creation of a console.

[...] As far as a positive flag, what about suppressConsole? Little verbose I suppose...

No, that's great. It's even more precise than noConsole. I've changed it now. Lars
Apr 06 2013
prev sibling next sibling parent "Jesse Phillips" <Jesse.K.Phillips+D gmail.com> writes:
On Saturday, 6 April 2013 at 09:13:11 UTC, Lars T. Kyllingstad 
wrote:
 I wonder if we should change the name of Config.gui to 
 Config.noConsole.  It corresponds to the CREATE_NO_WINDOW flag 
 in the Windows API, for which the documentation says:

 "The process is a console application that is being run without 
 a console window. Therefore, the console handle for the 
 application is not set. This flag is ignored if the application 
 is not a console application [...]"

 I know some people don't like negative flags, but in this case 
 it seems more precise.  It doesn't create a GUI, it prevents 
 the creation of a console.

 Lars

I think noConsole or suppressConsole is acceptable, with preference to the latter. It seems to me gui is very mis-representative of what it is.
Apr 06 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Friday, 5 April 2013 at 05:54:39 UTC, Lars T. Kyllingstad 
wrote:
 The following is a message posted by Johannes Pfau to the other 
 (pre-)review thread which didn't seem to get noticed, along 
 with an edited version of my reply.

 On Sunday, 31 March 2013 at 13:14:52 UTC, Johannes Pfau wrote:
 Reposted from github:
 I think it would be nice if the high level functions would 
 also allow
 using custom environment variables. So the execute and 
 executeShell
 functions should have overloads which accept a string[string] 
 with
 environment variables (and probably accept a Config as well).

 The execute and executeShell functions would be trivial to 
 implement
 though if pipeProcess / pipeShell had an overload with 
 environment
 and Config parameters. So it's probably more important that
 pipeProcess / pipeShell get these overloads.


I have implemented this now. I think this was a great improvement. pipeProcess(/-Shell) and execute(Shell) now wield more of the power of spawnProcess(/-Shell) without complicating their usage. Please comment. Lars
Apr 07 2013
prev sibling next sibling parent "Jesse Phillips" <Jessekphillips+d gmail.com> writes:
On Sunday, 7 April 2013 at 18:52:12 UTC, Lars T. Kyllingstad 
wrote:
 I have implemented this now.  I think this was a great 
 improvement.  pipeProcess(/-Shell) and execute(Shell) now wield 
 more of the power of spawnProcess(/-Shell) without complicating 
 their usage.

 Please comment.

 Lars

From the docs it looks like in order to pass a Config you'd also need to pass an Environment, since all overloads take one and Config comes last.
Apr 07 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Sunday, 7 April 2013 at 19:28:12 UTC, Jesse Phillips wrote:
 On Sunday, 7 April 2013 at 18:52:12 UTC, Lars T. Kyllingstad 
 wrote:
 I have implemented this now.  I think this was a great 
 improvement.  pipeProcess(/-Shell) and execute(Shell) now 
 wield more of the power of spawnProcess(/-Shell) without 
 complicating their usage.

 Please comment.

 Lars

From the docs it looks like in order to pass a Config you'd also need to pass an Environment, since all overloads take one and Config comes last.

I suspect that people will modify the environment more often than the config parameter, which is why I've placed them in that order. If you just want to inherit the parent's environment, simply set env to null. Lars
Apr 07 2013
prev sibling next sibling parent "Graham St Jack" <graham.stjack internode.on.net> writes:
Not sure if this is the right place for this, but I have found a 
problem in my fairly old version of the new process module. The 
problem was in the POSIX spawnProcessImpl() just after the fork() 
call in the child process branch.

The child would occasionally block forever on an attempt by the D 
code to lock a mutex.

The problem went away when I moved the toStringx() and toArgz() 
calls to before the fork() call.

I have no idea what is going on here, but it seems like fork() 
leaves the child in bad shape, so my 'fix' is very much a 
band-aid.
Apr 07 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Monday, 8 April 2013 at 05:22:09 UTC, Graham St Jack wrote:
 Not sure if this is the right place for this, but I have found 
 a problem in my fairly old version of the new process module. 
 The problem was in the POSIX spawnProcessImpl() just after the 
 fork() call in the child process branch.

 The child would occasionally block forever on an attempt by the 
 D code to lock a mutex.

 The problem went away when I moved the toStringx() and toArgz() 
 calls to before the fork() call.

 I have no idea what is going on here, but it seems like fork() 
 leaves the child in bad shape, so my 'fix' is very much a 
 band-aid.

This is definitely the right place for it. :) That said, the bug has already been reported and fixed, in exactly the way you suggest. Lars
Apr 07 2013
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-04-04 16:22, Jesse Phillips wrote:
 Hello,

 I have decided to take on Review Manager for std.process and hopefully
 will keep it up and get through the review queue. This is slightly
 ignoring a "first come, first serve" approach, but that is mostly
 because I didn't want to look into it. (So don't be too upset).

 std.process has been under review for some time having discussion on the
 pull request and this thread:
 http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue forum.dlang.org

 I would like to propose the Formal review is one week (and one week
 voting). If there is objection speak up.

Why does all functions taking stdin/stdout/stderr append an underscore to these parameter names? -- /Jacob Carlborg
Apr 07 2013
parent reply Jacob Carlborg <doob me.com> writes:
On 2013-04-08 11:17, Lars T. Kyllingstad wrote:

 Because there are symbols with those names defined in both std.stdio and
 core.stdc.stdio, and I got naming conflicts all over the place.  This
 was a while ago, though, and the compiler may have seen improvements in
 this area since then.  I'll try it out later.

I would say that something's wrong if one needs to come up with special naming schemes for function arguments. -- /Jacob Carlborg
Apr 08 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-04-08 19:32, Lars T. Kyllingstad wrote:

 You sound surprised that there is something wrong in the world of
 software development. :)

Hehe, oh no, I would be surprised if it wasn't :)
 I've changed the parameter names now.  The auto-tester seems to approve,
 so I guess the compiler bugs have been fixed.  Thanks for pointing this
 out!

Great. -- /Jacob Carlborg
Apr 08 2013
prev sibling next sibling parent Johannes Pfau <nospam example.com> writes:
Am Sun, 07 Apr 2013 20:52:11 +0200
schrieb "Lars T. Kyllingstad" <public kyllingen.net>:

 On Friday, 5 April 2013 at 05:54:39 UTC, Lars T. Kyllingstad 
 wrote:
 The following is a message posted by Johannes Pfau to the other 
 (pre-)review thread which didn't seem to get noticed, along 
 with an edited version of my reply.

 On Sunday, 31 March 2013 at 13:14:52 UTC, Johannes Pfau wrote:
 Reposted from github:
 I think it would be nice if the high level functions would 
 also allow
 using custom environment variables. So the execute and 
 executeShell
 functions should have overloads which accept a string[string] 
 with
 environment variables (and probably accept a Config as well).

 The execute and executeShell functions would be trivial to 
 implement
 though if pipeProcess / pipeShell had an overload with 
 environment
 and Config parameters. So it's probably more important that
 pipeProcess / pipeShell get these overloads.


I have implemented this now. I think this was a great improvement. pipeProcess(/-Shell) and execute(Shell) now wield more of the power of spawnProcess(/-Shell) without complicating their usage. Please comment. Lars

Thanks! Looks good to me :-)
Apr 08 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Monday, 8 April 2013 at 06:48:43 UTC, Jacob Carlborg wrote:
 Why does all functions taking stdin/stdout/stderr append an 
 underscore to these parameter names?

Because there are symbols with those names defined in both std.stdio and core.stdc.stdio, and I got naming conflicts all over the place. This was a while ago, though, and the compiler may have seen improvements in this area since then. I'll try it out later. Lars
Apr 08 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Monday, 8 April 2013 at 11:17:27 UTC, Jacob Carlborg wrote:
 On 2013-04-08 11:17, Lars T. Kyllingstad wrote:

 Because there are symbols with those names defined in both 
 std.stdio and
 core.stdc.stdio, and I got naming conflicts all over the 
 place.  This
 was a while ago, though, and the compiler may have seen 
 improvements in
 this area since then.  I'll try it out later.

I would say that something's wrong if one needs to come up with special naming schemes for function arguments.

You sound surprised that there is something wrong in the world of software development. :) I've changed the parameter names now. The auto-tester seems to approve, so I guess the compiler bugs have been fixed. Thanks for pointing this out! Lars
Apr 08 2013
prev sibling next sibling parent reply "Steven Schveighoffer" <schveiguy yahoo.com> writes:
A couple minor comments:

1. I have two issues with Error being used.  One is that we should have a  
specific type that is thrown, not raw Error type.  Second is that I think  
in situations where the error is due to an incorrect parameter, it should  
be an exception not an error (and not a straight Exception either!).
2. For the documentation of inheritFDs, it says that "On Windows, this  
option has no effect."  While this is true, it may give the impression  
that we don't set the bInheritHandles flag, which we do.  I think there  
should be a note on that somewhere.

Otherwise, the API and functionality looks good!

-Steve
Apr 11 2013
next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-04-11 17:43, Steven Schveighoffer wrote:
 A couple minor comments:

 1. I have two issues with Error being used.  One is that we should have
 a specific type that is thrown, not raw Error type.  Second is that I
 think in situations where the error is due to an incorrect parameter, it
 should be an exception not an error (and not a straight Exception either!).

Does it throw an Error, that's bad. Code should basically never create and throw an Error or Exception. It should always be a derived type from either of these classes. -- /Jacob Carlborg
Apr 11 2013
prev sibling next sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-04-11 20:12, Lars T. Kyllingstad wrote:

 I agree for the most part.  The 'environment' functions do throw
 straight Exceptions, and I'm not happy about that.  I just don't like
 the idea of adding even more module-specific Exceptions and Errors, when
 what we really should do is decide on a systematic organisation for the
 entire library.

 Then again, maybe such a reorganisation will cause enough breakage that
 one more module makes little difference?  Any good name suggestions for
 the errors and exceptions in question?

EnvironmentException, SystemException, ProcessException. -- /Jacob Carlborg
Apr 11 2013
prev sibling parent Jacob Carlborg <doob me.com> writes:
On 2013-04-12 13:31, Lars T. Kyllingstad wrote:

 I used to like enforce(), but now I
 think it has given D programmers a way too lax attitude towards error
 handling.)

What's wrong with "enforce" is that it's possible to not specify what kind of exception to thrown. Otherwise I kind of like it.
 kill() throws Error if the code/signal is negative.  I suspect the cases
 where this number comes directly from user input are so few and far
 between that it is reasonable to expect the programmer to ensure that it
 is nonnegative.  In principle, on POSIX we don't need the check, because
 POSIX kill() will return an "invalid signal" error if you try to give it
 a negative number.

What do you thrown when POSIX kill() returns an "invalid signal" error? -- /Jacob Carlborg
Apr 12 2013
prev sibling next sibling parent "David Nadlinger" <see klickverbot.at> writes:
The doc build linked in the first post says that the execute* 
return type might change to 
std.typecons.Tuple!(int,"status",bool,"output") – I suppose this 
should be »…, string, "output"«.

Unfortunately, I probably won't manage to do an actual review 
before the deadline, but I didn't notice any obvious major issues.

David
Apr 11 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 11 April 2013 at 15:43:18 UTC, Steven Schveighoffer 
wrote:
 A couple minor comments:

 1. I have two issues with Error being used.  One is that we 
 should have a specific type that is thrown, not raw Error type.
  Second is that I think in situations where the error is due to 
 an incorrect parameter, it should be an exception not an error 
 (and not a straight Exception either!).

I agree for the most part. The 'environment' functions do throw straight Exceptions, and I'm not happy about that. I just don't like the idea of adding even more module-specific Exceptions and Errors, when what we really should do is decide on a systematic organisation for the entire library. Then again, maybe such a reorganisation will cause enough breakage that one more module makes little difference? Any good name suggestions for the errors and exceptions in question?
 2. For the documentation of inheritFDs, it says that "On 
 Windows, this option has no effect."  While this is true, it 
 may give the impression that we don't set the bInheritHandles 
 flag, which we do.  I think there should be a note on that 
 somewhere.

I'll add one. Lars
Apr 11 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 11 April 2013 at 17:56:17 UTC, David Nadlinger wrote:
 The doc build linked in the first post says that the execute* 
 return type might change to 
 std.typecons.Tuple!(int,"status",bool,"output") – I suppose 
 this should be »…, string, "output"«.

That's right. I'll fix it. Lars
Apr 11 2013
prev sibling next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-04-04 16:22, Jesse Phillips wrote:

 I have decided to take on Review Manager for std.process and hopefully
 will keep it up and get through the review queue. This is slightly
 ignoring a "first come, first serve" approach, but that is mostly
 because I didn't want to look into it. (So don't be too upset).

1. Why is there functionality for handing environment variables in std.process? 2. The class "environment" should be capitalized -- /Jacob Carlborg
Apr 11 2013
parent reply Jacob Carlborg <doob me.com> writes:
On 2013-04-11 22:31, Lars T. Kyllingstad wrote:

 It's implemented using a class, but you use it like an object. I think
 that is more important.

I see. I know there are some people here that are very picky about these things. -- /Jacob Carlborg
Apr 11 2013
parent Jacob Carlborg <doob me.com> writes:
On 2013-04-12 13:05, Lars T. Kyllingstad wrote:

 Then let me ask you/them this:  Would you prefer it was implemented as a
 module-level  property function that returns a singleton class instance
 instead?  In other words, would you have the same API with slightly
 worse performance and more complicated implementation, *just* for the
 sake of a strict naming convention?

 Note that 'environment' (with that name) was added to std.process years
 ago.  As Jesse mentioned, the fact that it was a class was just hidden
 from documentation with an alias.  Unfortunately, this prevented proper
 documentation of its members, so changed it.

Ok, I see. I have don't have _that_ strong feelings about this. I also wouldn't have any problem of using it as class, like it is: Environment["foo"] = "bar"; -- /Jacob Carlborg
Apr 12 2013
prev sibling next sibling parent "Jesse Phillips" <Jessekphillips+D gmail.com> writes:
On Thursday, 11 April 2013 at 18:32:23 UTC, Jacob Carlborg wrote:
 On 2013-04-04 16:22, Jesse Phillips wrote:

 I have decided to take on Review Manager for std.process and 
 hopefully
 will keep it up and get through the review queue. This is 
 slightly
 ignoring a "first come, first serve" approach, but that is 
 mostly
 because I didn't want to look into it. (So don't be too upset).

1. Why is there functionality for handing environment variables in std.process? 2. The class "environment" should be capitalized

Because it is manipulating the environment your process is running in? True, though I'd then say do what the original did and alias the class to its lowercase. Not that the result is any different though.
Apr 11 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 11 April 2013 at 18:32:23 UTC, Jacob Carlborg wrote:
 2. The class "environment" should be capitalized


It's implemented using a class, but you use it like an object. I think that is more important. Lars
Apr 11 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 11 April 2013 at 18:20:02 UTC, Jacob Carlborg wrote:
 Does it throw an Error, that's bad. Code should basically never 
 create and throw an Error or Exception. It should always be a 
 derived type from either of these classes.

I completely agree. However, Phobos already throws straight Exceptions all over the place (just grep for "enforce"), so the few places where std.process does so is just in keeping with tradition. ;) Jokes aside, using Error/Exception until we have decided on a proper exception hierarchy will help reduce the breakage when we do. Lars
Apr 12 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Friday, 12 April 2013 at 06:36:31 UTC, Jacob Carlborg wrote:
 On 2013-04-11 22:31, Lars T. Kyllingstad wrote:

 It's implemented using a class, but you use it like an object. 
 I think
 that is more important.

I see. I know there are some people here that are very picky about these things.

Then let me ask you/them this: Would you prefer it was implemented as a module-level property function that returns a singleton class instance instead? In other words, would you have the same API with slightly worse performance and more complicated implementation, *just* for the sake of a strict naming convention? Note that 'environment' (with that name) was added to std.process years ago. As Jesse mentioned, the fact that it was a class was just hidden from documentation with an alias. Unfortunately, this prevented proper documentation of its members, so changed it. Lars
Apr 12 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Thursday, 11 April 2013 at 15:43:18 UTC, Steven Schveighoffer 
wrote:
 A couple minor comments:

 1. I have two issues with Error being used.  One is that we 
 should have a specific type that is thrown, not raw Error type.
  Second is that I think in situations where the error is due to 
 an incorrect parameter, it should be an exception not an error 
 (and not a straight Exception either!).

Let's go through the places where an Error or Exception is thrown: spawnProcess() throws RangeError when args[] is empty, but this is just the normal behaviour of arrays, and with -release/-noboundscheck it just segfaults. As such, there is little point in specifying this in the documentation. I'll remove it. (Honestly, I don't know why I put it in there in the first place. It may have had something to do with me being thoroughly annoyed over other the lack of exception specifications in Phobos documentation at large. I used to like enforce(), but now I think it has given D programmers a way too lax attitude towards error handling.) kill() throws Error if the code/signal is negative. I suspect the cases where this number comes directly from user input are so few and far between that it is reasonable to expect the programmer to ensure that it is nonnegative. In principle, on POSIX we don't need the check, because POSIX kill() will return an "invalid signal" error if you try to give it a negative number. On Windows, however, TerminateProcess() takes an unsigned integer (which is why I added the check in the first place), and I think we should strive to have the same behaviour on all platforms to the extent possible. I would not be strongly opposed to making this an Exception of some kind, though, if you think there is a good reason to do so. pipeProcess throws Error on an invalid combination of Redirect flags, and ProcessPipes does the same on an attempt to access a non-redirected stream. Are we in agreement that these two are always programming errors? escapeShellCommand() throws Error if the input contains \0, \r or \n. Here, it is very likely that the arguments are user input, but it would be very strange application code that somehow allowed those three characters to enter the input. Even so, it may be better to make it an Exception, just to be safe. Lars
Apr 12 2013
prev sibling next sibling parent "Lars T. Kyllingstad" <public kyllingen.net> writes:
On Friday, 12 April 2013 at 11:31:29 UTC, Lars T. Kyllingstad 
wrote:
 Let's go through the places where an Error or Exception is 
 thrown:

 [...]

I forgot environment, whose methods throw straight Exceptions. As I've said before, I am not happy about this at all, but it *is* how environment is implemented in the old std.process too. Thus, leaving it like this until we figure out the whole exception mess is, IMO, better than breaking it twice. Lars
Apr 12 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Fri, 12 Apr 2013 07:31:28 -0400, Lars T. Kyllingstad  
<public kyllingen.net> wrote:

 On Thursday, 11 April 2013 at 15:43:18 UTC, Steven Schveighoffer wrote:
 A couple minor comments:

 1. I have two issues with Error being used.  One is that we should have  
 a specific type that is thrown, not raw Error type.
  Second is that I think in situations where the error is due to an  
 incorrect parameter, it should be an exception not an error (and not a  
 straight Exception either!).

Let's go through the places where an Error or Exception is thrown: spawnProcess() throws RangeError when args[] is empty, but this is just the normal behaviour of arrays, and with -release/-noboundscheck it just segfaults. As such, there is little point in specifying this in the documentation. I'll remove it. (Honestly, I don't know why I put it in there in the first place. It may have had something to do with me being thoroughly annoyed over other the lack of exception specifications in Phobos documentation at large. I used to like enforce(), but now I think it has given D programmers a way too lax attitude towards error handling.)

Oh, this really needs clarification! I was under the assumption that you are specifically checking this and throwing RangeError, even in release mode. Please update the docs to reflect this. I think it's worth noting in the docs that it will throw RangeError or segfault. It may not be obvious that the function will not do this check for you in release mode. I'd almost lean towards making the check even in release mode, but you can still throw RangeError.
 kill() throws Error if the code/signal is negative.  I suspect the cases  
 where this number comes directly from user input are so few and far  
 between that it is reasonable to expect the programmer to ensure that it  
 is nonnegative.  In principle, on POSIX we don't need the check, because  
 POSIX kill() will return an "invalid signal" error if you try to give it  
 a negative number.  On Windows, however, TerminateProcess() takes an  
 unsigned integer (which is why I added the check in the first place),  
 and I think we should strive to have the same behaviour on all platforms  
 to the extent possible.  I would not be strongly opposed to making this  
 an Exception of some kind, though, if you think there is a good reason  
 to do so.

Think of re-implementing command-line kill :) In the end, an exception and error are quite the same, except for the fact that you SHOULDN'T catch errors. The place where this makes a difference is whether the parameter comes from user input or not. In the case of a kill signal, I can potentially see a user specifying a non-existent signal. This should be caught and a nice error message explaining the usage or whatnot should be displayed.
 pipeProcess throws Error on an invalid combination of Redirect flags,  
 and ProcessPipes does the same on an attempt to access a non-redirected  
 stream.  Are we in agreement that these two are always programming  
 errors?

These two are separate. I think the invalid combination of redirect flags is not beyond user-specification (think of processing 'cmd > out.txt 1>&2' ) Accessing a non-existent member is always a programming error, there is no recourse to that, error is fine here.
 escapeShellCommand() throws Error if the input contains \0, \r or \n.   
 Here, it is very likely that the arguments are user input, but it would  
 be very strange application code that somehow allowed those three  
 characters to enter the input.  Even so, it may be better to make it an  
 Exception, just to be safe.

Again, it's whether it's catchable or not. Errors should only be caught by the runtime (IMO). In order to do this correctly then (and print a nice error message for the user), you have to first verify the arguments before passing into the function, and not call the function. Essentially you have to duplicate the checking code in the application, and then have the library run the same thing! I think exception here as well. -Steve
Apr 12 2013
prev sibling next sibling parent reply Martin Nowak <code dawg.eu> writes:
The templated pipeProcessImpl only contains a single unshared line of 
code [1]. It might be worthwhile to reduce the amount of duplication by 
using delegates instead of alias parameters.

pipeProcess
{
     return pipeProcessImpl(
         (SpawnFuncArgs args) => spawnProcess(program, args),
         redirectFlags, env, config);
}

pipeShell
{
     return pipeProcessImpl(
         (SpawnFuncArgs args) => spawnShell(command, args),
         redirectFlags, env, config);
}

private alias TypeTuple!(File, File, File, const string[string], Config) 
SpawnFuncArgs;

private ProcessPipes pipeProcessImpl(scope Pid delegate(SpawnFuncArgs) 
spawnFunc,
                                      Redirect redirectFlags,
                                      const string[string] env = null,
                                      Config config = Config.none)
{
     // ... shared code
     auto pid = spawnFunc(childStdin, childStdout, childStderr, env, 
config);
}

[1]: 
https://github.com/kyllingstad/phobos/blob/994437169f20907f30515cfef54538710f3fc1fe/std/process.d#L1636
Apr 13 2013
parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Sat, 13 Apr 2013 16:16:09 -0400, Martin Nowak <code dawg.eu> wrote:

 The templated pipeProcessImpl only contains a single unshared line of  
 code [1]. It might be worthwhile to reduce the amount of duplication by  
 using delegates instead of alias parameters.

I think it's a valid point, but I also think the duplication is not that bad at the moment, there are only two instantiations. However, if we ever got to the point where we were templating on the command string type, it would be more savings by using a delegate. This optimization is also fully internal, so it can be submitted as an enhancement after acceptance. -Steve
Apr 15 2013
prev sibling next sibling parent Dmitry Olshansky <dmitry.olsh gmail.com> writes:
04-Apr-2013 18:22, Jesse Phillips пишет:
 Hello,

 I have decided to take on Review Manager for std.process and hopefully
 will keep it up and get through the review queue. This is slightly
 ignoring a "first come, first serve" approach, but that is mostly
 because I didn't want to look into it. (So don't be too upset).

 std.process has been under review for some time having discussion on the
 pull request and this thread:
 http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue forum.dlang.org

Getting late on this one. What is a plan (if any) to migrate towards something else then File for streams once we have it (e.g. std.io by Steve)? -- Dmitry Olshansky
Apr 16 2013
prev sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
On Tue, 16 Apr 2013 05:23:32 -0400, Dmitry Olshansky  =

<dmitry.olsh gmail.com> wrote:

 04-Apr-2013 18:22, Jesse Phillips =D0=BF=D0=B8=D1=88=D0=B5=D1=82:
 Hello,

 I have decided to take on Review Manager for std.process and hopefull=


 will keep it up and get through the review queue. This is slightly
 ignoring a "first come, first serve" approach, but that is mostly
 because I didn't want to look into it. (So don't be too upset).

 std.process has been under review for some time having discussion on =


 pull request and this thread:
 http://forum.dlang.org/thread/stxxtfwfrwllkcpunhue forum.dlang.org

Getting late on this one. What is a plan (if any) to migrate towards something else then File fo=

 streams once we have it (e.g. std.io by Steve)?

My library will provide a replacement for the back-end of File (i.e. FIL= E = *), so it will just work. -Steve
Apr 16 2013