Re: [boost] [gsoc] Boost.Process done

Subject: Re: [boost] [gsoc] Boost.Process done
From: Boris Schaeling (boris_at_[hidden])
Date: 2010-09-10 18:57:57

On Fri, 10 Sep 2010 05:02:54 +0200, Jeremy Maitin-Shepard  
<jeremy_at_[hidden]> wrote:

Jeremy,

I'll go through your feedback quickly as it's pretty late here. :)

> [...]   - The std::istream/std::ostream interface should be decoupled  
> from the facility for retrieving the file descriptor (int) or Windows  
> HANDLE, to avoid the overhead of allocating a streambuf for those users  
> that wish to access the file descriptor/HANDLE directly or use it with  
> asio.   Instead, the std::istream/std::ostream interface to a file  
> descriptor/HANDLE could be provided as a separate, completely  
> independent facility (designed so that exactly the same syntax could be  
> used on both Windows and POSIX), and likewise a separate facility for  
> providing an asio interface to a file descriptor/HANDLE (which also  
> allows exactly the same syntax to be used on POSIX and Windows).

Can you show some sample code you would like to write? Then I think I  
understand better how Boost.Process would need to be changed.

>    - It seems it would be cleaner for the parent side of these pipes to  
> be returned in the course of specifying that pipes should be used (in  
> the current terminology, in the course of specifying the pipe stream  
> behavior).  This prevents context from being reusable, but it seems that  
> it would be reusable only a very limited set of circumstances anyway.  
> (E.g. if the context::setup method needs additional invocation-specific

Not sure what you mean - can you show some sample code again? :)

> parameters, etc.)  Furthermore, it seems inconsistent that certain  
> things are part of the "reusable" context, like the environment and  
> process_name, but other things, like the executable path and arguments,  
> are not.

So far reusing a context means launching the very same application a  
second time. This didn't work first with the stream behavior class  
hierarchy but works now with the delegates.

> - On POSIX, due to the complicated semantics of waitpid, etc. and  
> SIGCHLD, and the general desire to avoid leaving around zombie child  
> processes, any code that deals with child processes needs to coordinate  
> with all other code that deals with child processes.  The library should  
> explicitly document what it does with respect to SIGCHLD signals and  
> waiting on child processes (and possibly change the behavior if the  
> current behavior doesn't handle all cases properly), so that other code  
> can correctly interact with it.

I agree that the documentation should be improved. It was created from  
scratch for Boost.Process 0.4 and doesn't cover yet really everything.

> - As was observed in the discussion regarding setting up  
> stdin/stdout/stderr file descriptors on POSIX, it is somewhat tricky to  
> correctly set up child file descriptors.  As I believe it is fairly  
> common on POSIX to want to set up child file descriptors other than 0,  
> 1, and 2, it would be best for the library to provide explicit support  
> for this, rather than forcing the user to implement it manually in  
> context::setup.  A reasonable interface might be e.g.:
>    context::map_file_descriptor(int child_fileno, file_descriptor_t  
> existing_descriptor);

Once we start to support platform-specific features the question is where  
do we end? The context class in previous Boost.Process versions grew more  
and more as support for uid, euid, gid, eguid, chroot etc. was added.  
Instead of arguing about each and every feature whether it's justified to  
hardcode it into the library we decided it might be better to support  
cross-platform features only and provide extension points.

> [...]- One potential issue is that the Windows extension interface only  
> exposes a (relatively) small subset of the functionality that can be  
> configured on Windows.  There seem to be numerous flags to CreateProcess  
> that are not exposed by Boost.Process, and there are also two other  
> functions, CreateProcessAsUser and CreateProcessWithLoginW, which allow  
> specifying some additional options.  I am not a Windows programmer, so I  
> don't know how important this additional functionality is in practice,  
> but it seems it would be unfortunate for users that would like to use  
> other conveniences provided by Boost.Process (such as pipe setup,  
> perhaps) to have to revert to invoking the Windows API directly in order  
> to make use of additional functionality not exposed.  In contrast, it is  
> hard to imagine the POSIX extension interface being insufficient.  I'm  
> not sure how this could be solved, exactly.

Good point! Indeed a bit more flexibility would be nice here.

> - I think it would be cleaner for the POSIX and Windows extension  
> interfaces to be in the form of a callback function (templated or  
> boost::function), rather than a subclass of context that overrides the  
> setup function.

Makes also sense to me!

> - It seems that natively on Windows, unlike on POSIX, there is no notion  
> of an array of command line arguments --- there is just a single string  
> command line.  It seems it would be useful for Boost Process to allow a  
> single string command line to be specified directly, in addition to the  
> existing array (Container) interface which presumably does some sort of  
> encoding into a single string on Windows.  Perhaps create_child can  
> simply distinguish (at compile-time) between being passed a range of  
> char and being passed a range of range of char.

Again sounds good. :)

> - On Windows, it seems there should be both a char and wchar_t versionof  
> everything that involves characters.  I suggest that the wchar_t version  
> simply not exist on POSIX, as it wouldn't make much sense nor would it  
> be useful.

I don't remember anymore all the details which made it difficult to add  
support for wide chars in previous Boost.Process versions. But this is  
something I still want to look into (probably after everything else has  
been settled).

Thanks for your feedback,
Boris