The new std.process is ready for review

[Vladimir Panteleev]

On Monday, 25 February 2013 at 06:41:32 UTC, Lars T. Kyllingstad 
wrote:
> Sure, I can think of another example.  But I wouldn't read too 
> much into this one; it was never meant as a demonstration of 
> the "correct" way to open a web page.   It was just a simple 
> example of spawnProcess() usage that uses a cross-platform 
> application everyone's heard of.
>
> After all, you *could* argue this way about almost any kind of 
> application which wasn't just invented for the sake of the 
> example.  (In the last one, shouldn't we open the user's 
> preferred word processor, etc?)

The question is, what is the intent? Is it to just open some URL, 
or to specifically start Firefox? The same applies to the word 
processor case - if the document is in a file format understood 
by several applications, is the intent to simply open the 
document, or to open the document in that specific application?

Now, the documentation clearly says that the example specifically 
launches Firefox. However, that doesn't mean that someone won't 
reach out for that example when hastily putting together an 
application that needs to open an URL. After all, it's at the top 
of the file, and they may not even know about the existence of 
the browse function which actually does what they intend.

How about using "lynx -dump http://dlang.org/"? Dumping a text 
representation of a webpage is a feature specific to lynx, so the 
intent is clearer.

>> 2. (Nitpick) The grep example uses a POSIX quoting syntax 
>> (single quotes). Would be better to use double quotes, or pass 
>> as array to avoid one more needless OS-specific element.
>
> Actually, the quotes can just be removed altogether.

OK, and now it's worse: your example uses syntax that's specific 
to std.process2. If you type that command in the shell, you'll 
get different behavior (the backslash will escape the . as a 
shell escape, not a RE escape).

> If anyone has a good idea for sample code which will be 
> familiar to users of all platforms, please speak up.

If we restrict ourselves to programs that would already work for 
all users, there's not much to pick from: the standard Windows 
and POSIX command-line utilities barely overlap, although there's 
also the programs included with D. Maybe include dmd, rdmd or 
dman in some examples?

> Personally, I don't think they should be part of the public 
> API.  They are inherently platform-specific, and we've tried to 
> keep the module as platform-agnostic as possible.

Constructing scripts is bound to be platform-specific. The 
current module version allows constructing batch files on POSIX.

Here's a practical use case example for this feature: DMD uses 
the same syntax for response files on all platforms, and it 
follows the Windows command-line parsing rules. Currently, rdmd 
uses escapeWindowsArgument to build that response file on all 
platforms.

> Besides, they are not really usable with any of the other 
> functions, and I am afraid it will be interpreted that way if 
> we make them public.

This is actually a design problem in the new module, which I 
haven't discussed yet. Have a look at the very last example in 
the current std.process docs. How do you accomplish that 
correctly in the new version, without manually piping the inputs 
yourself? You can't.

>> 5. The spawnProcess versions which take a single string 
>> command simply call std.string.split on the command. I believe 
>> this approach to be fundamentally wrong, as passing an 
>> argument in quotes will not work as expected. Furthermore, on 
>> Windows (where process arguments are passed as a single 
>> string), splitting the string in spawnProcess and then putting 
>> it back together in spawnProcessWindows will result in a final 
>> command line that is different from the one supplied by the 
>> user.
>
> The whole point was to avoid any kind of arcane 
> platform-specific quoting and escaping rules.  If you have 
> spaces inside your command line arguments, use the functions 
> that take an array, and spawnProcess() will properly quote 
> them.  If you have any other funky characters in there, don't 
> worry about it, spawnProcess() will properly escape them.
>
> The way it is now, the rules (if you can call it that) are 
> exceedingly simple, and they are the same on all platforms.  
> This has the added benefit of discouraging platform-dependent 
> client code.

OK, then picture the following situation.

A user of the new module starts using the module, and invokes a 
specific command using the spawnProcess overload that takes it as 
a single string. Convenient, right? Then, as the program evolves, 
the string becomes an enum, then a config variable, which the 
user can adjust.

Then, a end-user tries setting the config variable to a path that 
contains spaces, and everything breaks. Wrapping the path in 
quotes does not help either. Due to the way the function is 
designed, it is IMPOSSIBLE for the end-user to configure the 
application to launch a program located at a path containing 
spaces. To end-users, this comes off as a classical problem in 
badly written applications that don't handle command-line 
escaping properly.

This problem is as with any case of an interface which works in 
simple cases, but behaves unexpectedly in more complicated cases: 
it is bad design (convenience or not), and must be avoided.

I suggest that either the overloads which take a single string be 
removed, or that they spawn a shell instead, and let the shell do 
the command-line splitting. Together with my command and filename 
escaping functions, they should allow the user to achieve any 
combination of executing commands with arbitrary punctuation in 
the program path or arguments, as well as redirecting the output 
to files (again, with correctly-escaped filenames) or other 
programs using the existing shell syntax present on both 
platforms.