[process] Formal review results
The Boost.Process library is accepted. Congratulations to Klemens, and thanks to everybody who reviewed the library! The votes were as follows: + Johannes - Niall Douglas +/- Tom Kent +/- Nat Goodspeed + Paul A. Bristow +/- Bjorn Reese + Edward Diener + Boris Schäling +/- Raphaël Londeix + Klaim - Joël Lamotte Legend: + accept - decline +/- accept conditionally Separate thanks to Gavin Lambert for providing a solution to deadlocking problem. Additional thanks to Nat Goodspeed, Niall Douglas, Gavin Lambert, Raphaël Londeix and Bjorn Reese for spending significant amount of time on discussing, reviewing or testing the library. -- Quick review summary -- During the review multiple technical issues were raised and as far as I know, Klemens already working on them. The biggest win of the review - is a fix for possible deadlock with pipes. The most controversial part of the library is it's interface. Many comments were addressing: * passing arguments * starting process right in `system(...)` function Current approaches have benefits and drawbacks, other approaches have different benefits and drawbacks. At this point some people agree that the current approach is the lesser evil. Reviewers also wished to have better/richer documentation. P.S.: Please continue with the discussion and report issues that would help to improve the library. -- Best regards, Antony Polukhin
Am 09.11.2016 um 19:45 schrieb Antony Polukhin:
The Boost.Process library is accepted. Awesome. Congratulations to Klemens, and thanks to everybody who reviewed the library!
The votes were as follows: + Johannes - Niall Douglas +/- Tom Kent +/- Nat Goodspeed + Paul A. Bristow +/- Bjorn Reese + Edward Diener + Boris Schäling +/- Raphaël Londeix + Klaim - Joël Lamotte
Legend: + accept - decline +/- accept conditionally
Separate thanks to Gavin Lambert for providing a solution to deadlocking problem.
Additional thanks to Nat Goodspeed, Niall Douglas, Gavin Lambert, Raphaël Londeix and Bjorn Reese for spending significant amount of time on discussing, reviewing or testing the library. And not to forget, thank you for managing the process!
-- Quick review summary --
During the review multiple technical issues were raised and as far as I know, Klemens already working on them. The biggest win of the review - is a fix for possible deadlock with pipes. I'm glad to report that this is already fixed. That did however allow one additional feature: with the pipe automatically closing, you can now use a part of the asio stuff with bp::spawn.
The most controversial part of the library is it's interface. Many comments were addressing: * passing arguments * starting process right in `system(...)` function
Current approaches have benefits and drawbacks, other approaches have different benefits and drawbacks. At this point some people agree that the current approach is the lesser evil. I'm very glad to hear that. Reviewers also wished to have better/richer documentation. I'll keep working on that. Additionally, Nat will help me create a useful extension-interface, so that will become public and be documented. With that in, there will also be more information on how the
asio::io_service ios; std::futurestd::string fut; bp::spawn("foo", std_out > fut, ios); Not sure this is the smartest thing to do, but it could make sense for some simple applications. library works internally.
P.S.: Please continue with the discussion and report issues that would help to improve the library.
On 10/11/2016 09:00, Klemens Morgenstern wrote:
That did however allow one additional feature: with the pipe automatically closing, you can now use a part of the asio stuff with bp::spawn.
asio::io_service ios; std::futurestd::string fut; bp::spawn("foo", std_out > fut, ios);
Not sure this is the smartest thing to do, but it could make sense for some simple applications.
That's quite nice, actually -- a large number of applications would just want to pull the complete output from a process, either with no input or with completely provided input. It's probably less common to need to hold an interactive conversation, so having a nice quick syntax that hides the stream/pipe work internally is a pretty good win. Having said that, is there an inverse of this for providing the complete input? Presumably std_in < std::string(x) can't work, as this would be confused with a filename. (And while wrapping it in a future is possible, it'd be a bit silly.) This sort of thing kinda makes me wish that the originally proposed future_stream survived into Boost or the standard. Though you can simulate one with a composite future (a future that returns a value and a new future); I have an implementation kicking around somewhere. It's not the prettiest of things nor would it be a simple drop-in though, I think.
Am 09.11.2016 um 23:26 schrieb Gavin Lambert:
On 10/11/2016 09:00, Klemens Morgenstern wrote:
That did however allow one additional feature: with the pipe automatically closing, you can now use a part of the asio stuff with bp::spawn.
asio::io_service ios; std::futurestd::string fut; bp::spawn("foo", std_out > fut, ios);
Not sure this is the smartest thing to do, but it could make sense for some simple applications.
That's quite nice, actually -- a large number of applications would just want to pull the complete output from a process, either with no input or with completely provided input. It's probably less common to need to hold an interactive conversation, so having a nice quick syntax that hides the stream/pipe work internally is a pretty good win.
Having said that, is there an inverse of this for providing the complete input? Presumably std_in < std::string(x) can't work, as this would be confused with a filename. (And while wrapping it in a future is possible, it'd be a bit silly.) It's "std_in < asio::buffer(x)", whereby buffer is also pulled into the
Well the problem is the potential deadlock, since you cannot terminate the process if it hangs. But that's a risk conciously taken if you don't store the handle. Makes sense for some processes though. process namespace. And you can also use a future which tells when the write is complete: std::string x; std::future<void> f; bp::spawn("foo", std_in < asio::buffer(x) > f); and yeah, that looks a bit weird, but I couldn't think of a better operator for this.
This sort of thing kinda makes me wish that the originally proposed future_stream survived into Boost or the standard. Though you can simulate one with a composite future (a future that returns a value and a new future); I have an implementation kicking around somewhere. It's not the prettiest of things nor would it be a simple drop-in though, I think.
If there was some facility like that to use with boost.asio (which has a streambuf implementation) you could use it. I could add lazy syntax for a popular use-case. That one currently works asio::streambuf buf; bp::spawn("foo", std_out > buf); std::istream(&buf); //but here I don't know how thread-safe that is, that would be a question for boost.asio.
participants (3)
-
Antony Polukhin -
Gavin Lambert -
Klemens Morgenstern