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: Jeff Flinn (TriumphSprint2000_at_[hidden])
Date: 2011-02-11 09:01:35

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.

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. 
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" 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. It also minimizes copying and heap 
allocated handles.

The default constructed initializers allow me to launch apps that don't 
have std io, such as GUI apps, without carrying that baggage around. The 
path overload opens the appropriate file_descriptor sink/source, keeping 
it open until initialization is complete. 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.

>> 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.

Additionally, I'd like to see the find_executable_in_path from 
process/operations.hpp generalized into a facility in boost::filesystem 
which allows the iteration of a string of delimited directories, which 
can come from an environment variable. Besides PATH, I've needed search 
other environment variables like 'include', 'lib', etc.

And as stated above using boost::iostreams encapsulates file_descriptor 
/ handle management, reducing the needed code within the process 
library. I see postream/pistream as duplicating boost iostream facilities.

>> Lots of additional 'stuff' in the header implementing create_child.
>> Implementation is obfuscated by all of the ifdef'd platform dependencies
>> within a single function.
> 
> +1 if you meant operations.hpp

Yes, as well as child.hpp, context.hpp, handle.hpp, pid_type.hpp, 
pipe.hpp, self.hpp, stream_behavior.hpp, stream_id.hpp

>> 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 started with a fresh slate over the last couple of weeks and have an
>> initial high level design of the process launch part that alleviates
>> many of the above problems. It's key aspect is that the stock facilities
>> themselves are built using the same customization points that client
>> code can use to extend the library. Initial high level docs can be
>> found at: goo.gl/NvRAH and a rough initial implementation is in the
>> attached zip file. This is still very preliminary, but I thought it
>> important to add to the discussion now.
>>
>> Thanks, Jeff
> 
> Thank you for the review.

I look forward to boost process being the best that it can be.

Thanks, Jeff