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
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. You've got bp::on_exit for that if you use boost.asio, i.e. pass a io_service. 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. The only annoying thing there are the redirections, because there you'd need a variant. I really don't want to add variant to the std_in/std_err/std_out initializer, since would include them everytime
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 :) Thanks for the idea: option objects might be the best solution for that, and I think the best way would be to allow an easy implementation of
Am 04.11.2016 um 17:21 schrieb Raphaël Londeix: then. But: I could add it as an optional feature. Now that thing could be used like this: auto var = bp::make_variant(bp::std_out); bp::pipe p; var = p; bp::child c("foo", var); that. Maybe something like that: auto options = bp::make_options(bp::exe, bp::args, bp::std_err); options.set_exe("gcc"); if (verbose) options.add_arg("-v"); if (ignore_errors) options.std_err(bp::null); bp::child c(options); I'll need to check if I can come up with a syntax for that, but that would be a way to easily build an option type, without changing anything underneath. And that would also be extensible, which the other ideas were not. And if that works, I might even be able to automatically add the io_service if you do not pass it, hence reducing the pain caused by the fusion-errors. auto opt = bp::make_options(bp::std_err & bp::std_out); //requires an io_service, since it might be async. opt.get_io_service(); //and there it is. Now if you combine these two you'd have no problem making your own child-implementation. struct my_child { asio::io_service ios; decltype(bp::make_options(bp::exe, bp::args, bp::std_err) opt{ios}; //so it's only one bp::async_pipe out(ios); //because you already know what that's going to be. bp::child c; void operator()() {c = child(opt, std_out > out); }; }; This is also extensible. I would however not make this the default, since it incurrs run- and compile-time overhead.
* 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.
Yep, those are really ugly even if you know fusion. But I've got no idea how to create a good error message there, if anybody has an idea, let me know. SFINAE doesn't really help, because that just tells you, that no matching function could be called. And deep nested static_asserts also don't help anybody, especially since they get buried in the template error messages.
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.
Well I've got several tests for that, and the particular example (with echo) also can fail when only used with one - that's why I assume(d) there's some other issue. But since we found a design bug during the review, which is actually caused by a faulty analysis on my part, the behaviour of pipes will change. That means that the way the async_pipe handles the exit of the child process will be different and we'll see if that fixes it.
- 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,
participants (2)
-
Klemens Morgenstern -
Raphaël Londeix