|
Boost : |
Subject: [boost] [process] Formal Review
From: Raphaël Londeix (raphael.londeix_at_[hidden])
Date: 2016-11-04 12:21:25
>
> We encourage your participation in this review. At a minimum, kindly state:
> - Whether you believe the library should be accepted into Boost
>
YES
> * Conditions for acceptance
>
The interface used to spawn processes should be reworked.
> - Your knowledge of the problem domain
>
I have implemented some process wrappers many times. I have used the last
release of Boost.Process (0.5 I think?).
> - What is your evaluation of the library's:
> * Design
>
I think the similarity with the boost::thread api is a great choice. The
lifetime of process
is handled by small functors that will be called at specified times. I
think is a very good
design as it allows one to write small "plugins" in order to affect the
setup or the tear down
of a process. The integration with Boost.Asio follows the previous choice
of using boost::thread
as a reference, i.e. the child process is not the resource you read
from/write to, you need
some separate stream objects whose lifetime are tied to the child process:
struct MyProcess
{
bp::async_pipe std_in;
bp::async_pipe std_out;
bp::async_pipe std_err;
bp::child child;
};
I haven't seen a way however to be asynchronously notified when a child
exited, so I'm not
sure how one would handle gracefully multiple async_read/write while the
process could exit
at any time.
The choice of having a nice interface to spawn processes have been
discussed already, but I
still think that the library should aim to have the simplest and lowest
level interface.
IMO, C++ users do not need to have a nice and expressive syntax to spawn
process, I
cannot imagine a code base where many processes are launched at many places
in the
code. In other words, I think that either you rarely need to spawn a
process (a hack for example),
or your job is to spawn processes (a shell, a make clone, ...). In both
cases, you will
spawn processes in very few places in the code.
Moreover, this interface style has an implication on build times, errors
seen at build time
and makes it hard to use in cases where your program needs to spawn
processes based on
runtime decisions. if one decided to implement a shell that supports
redirections, pipes,
environment modification and background jobs, they would certainly need to
make runtime
adjustments to the arguments given to child constructor. As of today, this
has to be
worked around by the user ; I think this should be the primary interface.
A suggestion has been made to have a more idiomatic C++ construction, I
would extend it
with a specific options type:
auto options = bp::options().args("gcc");
if (verbose) options.append_arg("-v");
if (ignore_errors) options.std_err.redirect(bp::null_device);
bp::child child(io_service, std::move(options));
This way you make it impossible to modify the options after the child
process creation.
But that's just an idea :)
* Implementation
>
The implementation is clear, but the support for the aforementioned
constructor interface
"leaks" into the implementation everywhere. As the previous version of
Boost.Process,
the different OS support is nicely separated and each stage of the process
creation is
clearly implemented in its own header.
> * Documentation
>
The asynchronous section feels a bit short given the many possibilities the
interface
gives us.
> * Tests
>
Didn't check
> * Usefulness
>
I think it would be very useful to have something we can rely on instead of
our own
half baked implementations.
> - Did you attempt to use the library? If so:
> * Which compiler(s)
>
g++4.8 using c++11
> * What was the experience? Any problems?
>
I had many problems and decided to fallback on popen().
The first kind of problems were some compile errors. But the author kindly
stated
that I needed the HEAD of boost (which is not an option for me) but I found
a
workaround by not using some niceties of the interface:
https://github.com/klemens-morgenstern/boost-process/issues/20
https://github.com/klemens-morgenstern/boost-process/issues/21
As said previously, for someone not used to boost.fusion, the errors are
hard to
read.
The other problem that made me give up (temporarily) on using Boost.Process
was the fact that asynchronous reads where unreliable when more than one
process
is spawned.
https://github.com/klemens-morgenstern/boost-process/issues/22
This issue has been closed despite the fact that it is not fixed.
- How much effort did you put into your evaluation of the review?
>
I'd say half a day counting this mail and the testing I made.
Cheers,
-- Raphaël Londeix http://hotgloupi.fr
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk