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
next sibling parent reply "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
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 parent reply "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
next sibling 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 parent reply "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
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 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 reply 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
parent reply "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
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 "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 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 reply "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
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 reply "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
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 parent reply "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
next sibling parent reply "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
parent reply "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
parent reply "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
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 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 reply "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
next sibling parent reply "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
next sibling parent reply "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
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 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 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 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 "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
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 reply "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
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 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 reply "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
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 next sibling parent reply 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
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 parent reply "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
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 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 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 "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
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
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 parent reply "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
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 reply "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
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 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 parent reply 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
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=




y

 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 fo=


r  =


 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