Re: [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow

Subject: Re: [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Ilya Sokolov (ilyasokol_at_[hidden])
Date: 2011-02-14 01:28:07

On Fri, 11 Feb 2011 19:01:35 +0500, Jeff Flinn  
<TriumphSprint2000_at_[hidden]> wrote:
> Ilya Sokolov wrote:
>> On Thu, 10 Feb 2011 19:41:25 +0500, Jeff Flinn  
>> <TriumphSprint2000_at_[hidden]> wrote:
>>
>>> I've used the process library as a replacement for Qt's QProcess(which
>>> interfers with the main event loop in our application on Mac OSX). We
>>> were able to make boost process work across Windows and Mac OSX, but  
>>> did
>>> not have that sense of satisfaction at the end that usually comes with
>>> using most boost libraries.
>>>
>>> I do not think the library should be accepted into boost for the
>>> following reasons.
>>>
>>> Problems:
>>> [Ilya Sokolov: I've reordered some items]
>>
>>> Still had to create wrapper function and classes and use ifdef's to
>>> handle platform dependencies in client code.
>>> Stung multiple times when passing a relative path when setting the
>>> working directory on unix systems.
>>> Customization points lack access to many of the required Windows
>>> CreateProcess arguments.
>>> Overly complex io specification.
>>  Could you tell us where? Please show also how your library would solve  
>> it.
>
> The myriad of stream behaviour, stream ends, and methods of  
> specification to me do not best represent the problem domain.

We disagree here. The only problem I see is absence of the 'Rationale'
section in the docs.

> Thinking about this last night, I came up with the concept of a Ray,  
> which is a connected pair of boost iostreams' Source and Sink concepts.
> Pipe is too general in that it conveys flow can be in either direction.

The concept of pipe is known to almost every programmer.

> So assuming unix still: "uses just one internal buffer for temporary  
> storage of the flowing data, hence what goes in one direction overwrites  
> whatever is a naive programmer tries to send backward"

I never saw such errors.

> as stated in: http://goo.gl/RXY7p, makes Ray a more accurate term than  
> pipe. Then a model of a Ray would be file_descriptor_ray, which  
> comprises a boost iostream file_descriptor_sink and a  
> file_descriptor_source.
>
> So having stdin, stdout, stderr Initializers with constructors or  
> factory functions:
>
> stdin_from()
> stdin_from(const path&)
> stdin_from(const file_descriptor_source&)
> stdin_from(const file_descriptor_ray&)
>
> stdout_to()
> stdout_to(const path&)
> stdout_to(const file_descriptor_sink&)
> stdout_to(const file_descriptor_ray&)
>
> stdout_to()
> stderr_to(const path&)
> stderr_to(const file_descriptor_sink&)
> stderr_to(const file_descriptor_ray&)
>
> is to me conceptually much simpler than dealing with stream behaviour,  
> stream end and context classes.

The code above has the problem outlined here:
http://article.gmane.org/gmane.comp.lib.boost.devel/207711

> It also minimizes copying and heap allocated handles.

CreateProcess() and fork()/exec() calls are much more greedy than any
copying or heap allocation.

> The default constructed initializers allow me to launch apps that don't  
> have std io, such as GUI apps, without carrying that baggage around.

Current interface allows you the same.

> The path overload opens the appropriate file_descriptor sink/source,  
> keeping it open until initialization is complete.

If all paths in your library is of type fs::path, then what is the type(s)
of the arguments and environment of the child process?

> The file_descriptor_source / file_descriptor_sink overloads ensure at  
> compile time that the handle has been opened as a source/sink as  
> appropriate. The file_descriptor_ray overloads know whether to get a  
> source/sink handle. It may be that file_descriptor_ray belongs to boost  
> iostreams.
>
>>> Lack of synchronicity when launching.
>>> Pay for process handles even when not used.
>>  Not sure what that means.
>
> Sorry, my thoughts didn't make it all the way out of my finger tips. I  
> meant boost::process::child has a std::map<stream_id, handle>  data  
> member. Once the child process has been initialized/launched there is no  
> need to know about any handles. Only the process id/handle is required  
> when one needs to wait on the child process to complete. My view is that  
> any stream definition info needing to be accessed after initialization  
> does not belong in the child process class.

It might be better to return something like
boost::tuple<process, std::map<stream_id, handle> > from create_child().

>>> Different semantics regarding success/failure of CreateProcess/exec.
>>> Lack of launch error reporting.
>>  That could be solved by using a separate pipe for error reporting.
>> The only library where I've seen this technique is subprocess.py.
>>
>>> Replication of existing boost library functionality, and functionality
>>> that better belongs to other libraries.
>>  Where and what functionality, exactly?
>
> Boost filesystem IMO, is the proper currency when dealing with paths. It  
>    provides path decomposition which is done manually in operations.hpp.  
> It also insulates it's clients from the whole unicode/utf8 issue(or at  
> the least mitigates the issue) as described in several of the longest  
> mailing list threads in my memory.

See above. IMHO, the only problem that fs::path solves is the lack
of string type with known encoding. fs::path doesn't have any useful  
invariant
and all of its methods could be implemented as free functions.

> [snip]
>
>>> Lack of separation of concerns in create_child
>>> The monolithic code lacks ability to properly unit test components.
>>  I'm not sure that the 'separation of concerns' would help us to
>> unit-test better.
>
> How you can unit test all that's going on within create_child as it's  
> not exposed?

I'd add more extension points to the bp::context class, something like
boost::function<void()> createproces, boost::function<void()> fork and
boost::function<void()> exec members. It would allow to call  
shellexecute(ex)()
instead of CreateProcess() on windows and vfork() or spawn() instead of
fork()/exec() on posix.

> [snip]