The new std.process is ready for review

[Vladimir Panteleev]

On Monday, 25 February 2013 at 20:06:19 UTC, Lars T. Kyllingstad 
wrote:
> That is also incredibly obscure.  I'd venture a guess that only 
> ~10% of D's user base have even heard of Lynx.  Everyone knows 
> firefox, and will understand what the example is supposed to 
> illustrate.  (I admit that the ls/grep examples will also be 
> rather incomprehensible to someone not familiar with the *NIX 
> command line, and I will replace them with something else.  The 
> D toolchain, as you suggest below, is a very good idea.)

I still think using Firefox is a bad idea, but I've already 
presented my arguments.

> BTW, browse() should never have been added to std.process, in 
> my opinion.  Maybe to some other utility module, but then it 
> should at least be done right, and be properly documented.

What would you improve about it?

I have no opinion on its location in Phobos, but std.process is 
the most fitting one if you don't consider creating a new module.

> What does it actually do?  There is no way to tell unless you 
> read the source.

I don't see why the documentation needs to be burdened with 
implementation details, other than perhaps mentioning that it 
returns immediately. The implementation is rather OS-specific... 
if we find out that there is a better way of accomplishing the 
task on a given platform, the documentation would need to be 
updated. Isn't the documentation considered part of the contract 
with the library user?

>  (And then, it turns out that it spawns a new process for the 
> browser and returns immediately, but it does not return a 
> process ID that you can poll or wait for.  Bah.)

That's exactly how it should work... due to technical reasons. 
Most browsers will communicate the need to open a web page in a 
new tab to an existing instance and exit immediately, if there is 
an existing instance. There is no practical way to wait for the 
user to close the web browser.

> It is not worse.  It is a lot simpler, because the programmer 
> does not need to know anything about the underlying platform.  
> They only need to know one rule:  If your arguments contain 
> spaces, use the array functions.  I don't think the generic 
> process-spawning functions in std.process should be bound by, 
> or tied to, the syntax of whatever shell the programmer (or the 
> end user) prefers.

Yes, I agree that tying it to a certain shell syntax is bad. 
However, introducing a third syntax incompatible with either two 
major ones, which additionally is less expressive, is IMHO worse. 
The "universal" way to distinguish / quote arguments is to use an 
array.

> If your arguments contain spaces, use the array functions.

Not just arguments: programs as well. On Windows, all third-party 
software is expected to install itself under the "Program Files" 
directory. Unless whatever you're launching is expected to be in 
PATH, splitting the string by spaces won't get you far on Windows.

>> 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.
>
> Exposing the specifics of whatever programming language you are 
> using to the end user?

I don't understand what you mean here. It's not exposing any 
specifics if you don't implement anything in a way specific to D.

> I would just call that bad application programming.  If 
> anything, you should be using one of the 'shell' functions in 
> this case, not spawnProcess.

Yes, if the user is expected to customize the arguments as well. 
Otherwise it could very well be the spawnProcess overload that 
takes an array of arguments.

> You almost have me convinced that the single-string non-shell 
> functions must go.  In the case of pipeProcess() and execute(), 
> pipeShell() and shell() do the same job, with (arguably, still) 
> less surprises.  Maybe it would then be a good idea to add a 
> spawnShell() function to go with spawnProcess().

How about something that converts a ProcessPipes instance into a 
Tuple!(int, "status", string, "output") as returned by shell? 
Then you could do e.g.

auto result = shell("command").collectOutput();
// use result.status or result.output

> The escape*() functions need to be better documented.  Am I 
> correct that they quote according to 'cmd.exe' rules on Windows 
> and 'sh' rules on POSIX?

Yes. On Windows, though, the rules are defined by 
CommandLineToArgvW for splitting/escaping the individual 
arguments, and cmd.exe for the whole string when it's passed to 
the shell. Check the reference URLs in the comments in 
escapeWindowsArgumentImpl.