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