Re: [boost] [process] Formal Review starts today, 27 October

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-10-28 04:04:16

Am 28.10.2016 um 02:12 schrieb Tom Kent:
> On Thu, Oct 27, 2016 at 1:26 AM, Antony Polukhin <antoshkka_at_[hidden]>
> wrote:
>
>
>> Dear Boost community,
>>
>> The formal review of Klemens David Morgenstern's Process library
>> begins today, 27th October and ends on 5th November.
>>
>> We encourage your participation in this review. At a minimum, kindly
> state:
>> - Whether you believe the library should be accepted into Boost
>>    * Conditions for acceptance
> I believe this is borderline, with some serious quality issues, but should
> be accepted.
> With the following conditions:
> * Documentation is cleaned up substantially (see issues and pull request I
> submitted in github)
> * The documentation indicates that "There is no guarantee in which order
> the arguments will be applied!". This either needs to be fixed so that the
> order is always guaranteed or it needs to be greatly elaborated on as to
> when the order is kept. Process argument order is very often important.
It seems this statement is too ambiguous, meant are the arguments of the 
function, not the process. If you mix the property and overload style, 
the library will resort them in some way. I.e.:

bp::system("foo", "bar1", std_out > bp::null, "bar");

This will keep the arg-vector in order, i.e. {"bar1", "bar2"} but you 
have no guarantee if the std_out or the argv is applied first. This is 
only important for user extensions.
> * There needs to be a warning (or at least explanation of the implications
> of or resilience to) user-input going to command line calls with relation
> to shell injection vulnerabilities[1]. The python subprocess module has a
> good warning message we could probably borrow [2] if necessary.
Will be added to the shell reference.
> * Needs to specify setup (explain that this is a header-only library) and
> dependencies (the basic example needs boost::filesystem, even though it
> just uses strings?).
> * Need to fix introduction/tutorial examples (probably the library
> implementation) so they run on windows (and use the path to find exes).
That's not a good idea, because it incurs overhead (searching the path).
>
>
>> - Your knowledge of the problem domain
> Medium, from a user perspective. I've worked extensively with Python's
> similar Subprocess module.
>
>
>
>>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>>    * Design
> I like the design very much. The one big issue that I'm unsure about is
> that it is difficult to break out of a loop based on a pstream when the
> program exits. Maybe some EOF signal needed?
>
> I like the basic system/spawn/child design.
>
> Though I didn't dig too deep, the Sync/Async IO seems good.
>
> It seems weird that terminating a process doesn't terminate its children by
> default and you need to use a group. It seems like that should be an option
> of the child object (defaulted to on).
That's just the behaviour of the OS, which this library tries to mirror 
closely. Grouping is unecessary for many applications, that's why it's 
not the default.
>
> Environment is OK, is there a reason this_process isn't in the process
> namespace?
Well, for the same reason it's std::this_thread; 
boost::process::this_process would look weird, at least to me.
>
>>    * Implementation
>  From what I've seen running on windows, there are some prominent holes in
> the implementation. These seem to be bugs that could be fixed quickly.
>
>
>
>>    * Documentation
> Needs lots of help. Could also use more complex examples. I'm also left
> with the suspicion that there is more to the library than is covered by the
> docs. Some of the things mentioned in the tutorial seem to have other ways
> to use them, these should be called out in specific documentation for the
> items.
Well the reference is rather detailed, but if you have any common 
examples please let me know. It's not that easy to find a tool that 
everyone knows.
>
>
>>    * Tests
> I didn't actually look at the tests, but was disappointed to see that there
> was only 44% coverage.
Well gcov or lcov isn't happy with to deep template instanciations. The 
on_exec_setup is here is guaranteed to be executed, but for some reason 
that doesn't show: 
https://coveralls.io/builds/8547269/source?filename=%2Fhome%2Ftravis%2Fboost-local%2Fboost%2Fprocess%2Fdetail%2Fposix%2Fpipe_out.hpp

My local build is > 70%.
>
>>    * What was the experience? Any problems?
> msvc-12.0 - Visual Studio 2013 - Test suite failed.
> msvc-14.0 - Visual Studio 2015
>
> I ran into issues with some pretty basic examples.
>
>          boost::filesystem::path p = "python.exe";
>          std::system("python.exe --version"); // Works
>          bp::system("python.exe --version"); // Works
>          bp::system("python.exe", "--version"); // Fails
>          bp::system(p, "--version");  // Fails
>          bp::system("C:\\Python27-32\\python.exe", "--version"); // Works
>
> It seems that the library isn't correctly working with the path on windows,
> when the arguments are split from the executable.
It's not working at all with path, it depends on the OS. The cmd-style 
searches path, while he exe-args style does not. That's the weird 
platform dependent behaviour, I'm not elaborating on enough.
Boost.Process does not search the path automatically, it just uses the 
defaults of the OS. If you want to search the path, you have the 
search_path function for that, i.e. turn the first line into

boost::filesystem::path p = bp::search_path("python.exe");
>> - How much effort did you put into your evaluation of the review?
> A few hours.
>
> I hope these issues can get resolved and this can be included with boost!
> Tom
>
> [1] https://en.wikipedia.org/wiki/Shell_injection#Shell_injection
> [2]
> https://docs.python.org/3/library/subprocess.html#security-considerations
>
Thank you! I'll probably address most the issues this evening.