On Thu, Oct 27, 2016 at 1:26 AM, Antony Polukhin
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. * 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. * 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).
- 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). Environment is OK, is there a reason this_process isn't in the process namespace?
* 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.
* Tests
I didn't actually look at the tests, but was disappointed to see that there was only 44% coverage.
* Usefulness
Yes! This is something that definitely needs to be in boost.
- Did you attempt to use the library? If so: * Which compiler(s)
* 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.
- 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