Re: [boost] [Boost-users] [process] Formal review results
On 11/09/2016 07:45 PM, Antony Polukhin wrote:
The Boost.Process library is accepted.
Unconditionally? Please notice that some of the votes were conditional, and if those conditions are not met, they should be counted as no votes.
The most controversial part of the library is it's interface. Many comments were addressing: * passing arguments * starting process right in `system(...)` function
The comments were about starting the process in the child constructor.
2016-11-13 4:35 GMT-08:00 Bjorn Reese
On 11/09/2016 07:45 PM, Antony Polukhin wrote:
The Boost.Process library is accepted.
Unconditionally?
Conditional acceptance means that the mini-review is required. I can see no issues that require fixing in mini-review: * Currently there's no consensus on how to improve the arguments passing, so a mini review for changed interface would not improve the consensus. * There's no big unsolvable technical issues at this point and the library author is going to address all the comments before the library would appear in Boost. * Interface is controversial but it does not limit the library functionality. Separating the parameters and process start is still possible: auto params = std::make_tuple("foo", "bar", bp::std_out > stdout, bp::args += {"baz"}); std::make_from_tuplebp::child(params); So it does not seem to be a show-stopper.
Please notice that some of the votes were conditional, and if those conditions are not met, they should be counted as no votes.
Let's change all the +/- to -. Then there'll be equal count of + and - and review manager is allowed to weight the votes: * vote of the previous author of the library will have more weight and the library will pass. * even without the vote of the previous library author, removal of all the controversial interface change requests will result in library acceptance.
The most controversial part of the library is it's interface. Many comments were addressing: * passing arguments * starting process right in `system(...)` function
The comments were about starting the process in the child constructor.
Yes, sorry. I meant `child(...)`. And it is still solvable, see the code above. -- Best regards, Antony Polukhin
On 13 Nov 2016 at 8:03, Antony Polukhin wrote:
2016-11-13 4:35 GMT-08:00 Bjorn Reese
: On 11/09/2016 07:45 PM, Antony Polukhin wrote:
The Boost.Process library is accepted.
Unconditionally?
Conditional acceptance means that the mini-review is required. I can see no issues that require fixing in mini-review: * There's no big unsolvable technical issues at this point and the library author is going to address all the comments before the library would appear in Boost.
The way I've understood these reviews is that if there are unsolvable technical issues with the proposed design, the library is rejected. If there are solvable technical issues commonly agreed by the community, the review manager lists them as conditions for acceptance which as you mention implies a subsequent mini review or at least that the author asks for a quick look by the community to verify those problems have been addressed before the library enters the main Boost distro. If everyone agrees there are technical issues but nobody can reach consensus, it's up to the review manager to guide the community to a consensus in my opinion. I had taken the unconditional acceptance as meaning you felt no one in the community reviews had identified as a consensus problems you felt serious enough to be technical issues - there were only a few i's to be dotted and a few t's to be crossed, otherwise the library was Boost quality. I found that quite surprising personally, I feel the library as proposed as fundamentally flawed as Boost.Iostreams which is a travesty of bad design in my opinion. But others whom I know do know this domain very well did not object as substantially as I did (they also do not object to Iostreams as I do), so I defer to their judgment. Do you think a consensus list of issues needing to be fixed can be drawn up? Perhaps Klemens has been keeping a list he can simply copy and paste in here? Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Am 13.11.2016 um 20:59 schrieb Niall Douglas: > On 13 Nov 2016 at 8:03, Antony Polukhin wrote: > >> 2016-11-13 4:35 GMT-08:00 Bjorn Reese: >>> On 11/09/2016 07:45 PM, Antony Polukhin wrote: >>>> The Boost.Process library is accepted. >>> >>> Unconditionally? >> Conditional acceptance means that the mini-review is required. I can >> see no issues that require fixing in mini-review: >> * There's no big unsolvable technical issues at this point and the >> library author is going to address all the comments before the library >> would appear in Boost. > The way I've understood these reviews is that if there are unsolvable > technical issues with the proposed design, the library is rejected. > If there are solvable technical issues commonly agreed by the > community, the review manager lists them as conditions for acceptance > which as you mention implies a subsequent mini review or at least > that the author asks for a quick look by the community to verify > those problems have been addressed before the library enters the main > Boost distro. If everyone agrees there are technical issues but > nobody can reach consensus, it's up to the review manager to guide > the community to a consensus in my opinion. > > I had taken the unconditional acceptance as meaning you felt no one > in the community reviews had identified as a consensus problems you > felt serious enough to be technical issues - there were only a few > i's to be dotted and a few t's to be crossed, otherwise the library > was Boost quality. > > I found that quite surprising personally, I feel the library as > proposed as fundamentally flawed as Boost.Iostreams which is a > travesty of bad design in my opinion. But others whom I know do know > this domain very well did not object as substantially as I did (they > also do not object to Iostreams as I do), so I defer to their > judgment. > > Do you think a consensus list of issues needing to be fixed can be > drawn up? Perhaps Klemens has been keeping a list he can simply copy > and paste in here? > I did not, since the mails are archived. I've collected them, I hope I got this right. I'll address the met conditions when I have it implemented. Out of ten votes, four people gave a conditional acceptance, while there had be one rejection. ± Tom Kent ± Nat Goodspeed ± Bjorn Reese ± Raphaël Londeix Tom Kent wanted more documentation, which will be addressed, I'll that it when I'll commit it. Raphaël Londeix wanted a reworked interface, which will not happen, giving another rejection. Nat Goodspeed wanted a better interface for extensions and that will be worked out. Actually he will be the one helping me with that, since he got experience in extending boost.process 0.5, so I'm quite certain that that will fulfill his conditions. Bjorn had a longer list: > I vote for conditional acceptance, provided the following conditions are > met: > > 1. The interface style must be redesigned such that: > a. There is a clear separation between arguments and attributes. > b. Arguments and attributes should not be arbitrarily interspersed > in the parameter list. > c. A set of attributes can be build in advance, and reused for > different child processes. > d. Attributes can be optionally to the attribute-set. > > 2. There should be no I/O deadlocks by default. > > 3. An Overview section that introduces the concepts and the various > parts of the library must be added to the documentation. 1.a and 1.b will not be met, 1.c was always possible, through the wonders of std::tuple and 1.d is also part of the design. 2. is fixed, 3. will be done. Thus Raphaël would be a rejection, while it's sort of on the fence with Bjorn; so if we cound him as a rejection that leaves us at 7/10 for acceptance.
participants (4)
-
Antony Polukhin
-
Bjorn Reese
-
Klemens Morgenstern
-
Niall Douglas