Re: [boost] Formal Review of Proposed Boost.Process library
(First, please, excuse me for my "english".) This variant of the Boost.Process library should NOT (--never--) be accepted. The Boost.Process must be implemented as a DSEL (probably through the Boost.Proto expression templates framework) with a nice (commonly known) syntax like: using boost::process; using process::arg; namespace fs = boost::filesystem; process ls("ls"), grep("grep"); fs::path libs = "/usr/lib"; auto pipe = ls [--arg("reverse") % -arg('l') % libs] | grep ["^d"]; run(pipe); // or: // pipe(); // or: // ls(); This approach provide a declarative, not an imperative programming style. (A lot of work done by expression templates' inner mechanics.) The library must support: * i/o redirection * process piping; [implicit pipes] * named/unnamed pipe objects; [explicit pipes] * runned processes surfing (boost::process::processes_iterator): - CreateToolhelp32Snapshot()/Process32First()/Process32Next() API functions in Windows - /proc filesystem parsing in linux/some unix * loaded shared libraries surfing (boost::process::modules_iterator): - CreateToolhelp32Snapshot()/Module32First()/Module32Next() API functions in Windows - /proc filesystem parsing in linux/some unix * child processes surfing (boost::process::children_iterator) - parsing processes_iterator's range results - /proc filesystem parsing in linux/(perhaps) some unix - empty iterator range by default * runned thread per process surfing (boost::process::threads_iterator) - filtered results of CreateToolhelp32Snapshot()/Thread32First()/Thread32Next() API functions in Windows - /proc filesystem parsing in linux (see /proc/[pid]/task filesystem branch since linux 2.6.0-test6) - empty iterator range by default * runned thread surfing (boost::threads_iterator) - CreateToolhelp32Snapshot()/Thread32First()/Thread32Next() API functions in Windows - FOR-EACH process IN system: back_inserter(boost::process::threads_iterator(process), boost::process::threads_iterator(), threads); * (any) process stats * process creation * daemonization - through Service API on Windows * process security and privacy aspects (probably this is subject for an another separate library, part of which must be integrated with the Boost.Process) _________________________________ More examples: using boost::process; using process::arg; using process::env; namespace fs = boost::filesystem; // echo $PATH # (%PATH% in Windows) process("echo") [env("PATH")] (); // or: run(process("echo") [env("PATH")]); // ps -aux > ps.out process ps("ps"); run(ps [-arg('a', 'u', 'x')] > "ps.out"); process::_this_ > "file.out" < "file.in"; // probably static_assert() on Windows platform process::_this_ >> "file.out" < "file.in"; std::cin >> ...; // read from "file.in" std::cout << ...; // write to "file.out" // grep -i -n searched_word file.in > file.out: process grep("grep"); run( grep [-arg('i') % -arg('n') % "searched_word" % "file.in"] > "file.out"); // cat /etc/rc.d/xinetd | grep -v '^#' | sed '/^$/d' > file.out process cat("cat"), grep("grep"), sed("sed"); fs::path conf("/etc/rc.d/xinetd"); auto pipe = cat [conf] | grep [-arg('v') % "^#"] | sed ["/^$/d"] > "file.out"; pipe(); using namespace process::placeholders; process rm("rm"); rm [arg("filename") % "non_existent"] > "file.out", _2 >& _1; rm(); // or: run(rm); _________________________________ Some mini-review (concerning interface design only) on the proposed variant of the library: * process::find_executable_in_path() is a redundant function. His job must be done by default (when the filename() path's suffix supplied only instead of the full/relative path), if it's supposed on the target platform when the command shell gets the prog name from the user. In rare case the user can manipulates paths through the env(ironment variables) class (or put the full path explicitly), if default behavior is not suitable. By the way, a parameter and an return value must have been the boost::filesystem::basic_path<>, not a std::string. Obviously, the library must be integrated with the Boost.Filesystem. * A "stream behavior" boost::function<> mechanism is not obvious and not straightforward; unwarrantly hard to use. * environment(-variables) and args-list are solid abstractions (with a sharp behavior), not data structures. * child process subtype is a wrong abstraction (his "child" property isn't enough for his existence). Name it process simply. I.e. delete a child subtype from an inheritance tree; In Unix environment (similar in Windows) all processes, except INIT, have a parent: each process is a child almost without exceptions. * ----code excerpt from the User Guide-- int exit_code = child.wait(); #if defined(BOOST_POSIX_API) if (WIFEXITED(exit_code)) exit_code = WEXITSTATUS(exit_code); #endif std::cout << exit_code << std::endl; ---------------------------------------- IFDEFs must be incapsulated in the wait() member function. Signal terminating/stopping status of process can be abstracted into another terms, which are clear enough for a non "signal-conformant" platforms.
"bug" in pseudo-code:
FOR-EACH process IN system: back_inserter(boost::process::threads_iterator(process), boost::process::threads_iterator(), threads);
it must be rewritten as: FOR-EACH process IN system: std::copy(boost::process::threads_iterator(process), boost::process::threads_iterator(), std::back_inserter(threads));
AMDG On 2/19/2011 7:55 AM, Max Sobolev wrote:
(First, please, excuse me for my "english".)
This variant of the Boost.Process library should NOT (--never--) be accepted.
The Boost.Process must be implemented as a DSEL (probably through the Boost.Proto expression templates framework) with a nice (commonly known) syntax like:
I absolutely disagree. Not everything has to be done with expression templates. While I don't have any strong objection to your syntax, I certainly would not consider it essential. You're effectively implementing the shell function in expression templates.
using boost::process; using process::arg; namespace fs = boost::filesystem;
process ls("ls"), grep("grep"); fs::path libs = "/usr/lib";
auto pipe = ls [--arg("reverse") % -arg('l') % libs] | grep ["^d"];
run(pipe); // or: // pipe(); // or: // ls();
This approach provide a declarative, not an imperative programming style. (A lot of work done by expression templates' inner mechanics.)
In Christ, Steven Watanabe
The Boost.Process must be implemented as a DSEL (probably through the Boost.Proto expression templates framework) with a nice (commonly known) syntax like:
using boost::process; using process::arg; namespace fs = boost::filesystem;
process ls("ls"), grep("grep"); fs::path libs = "/usr/lib";
auto pipe = ls [--arg("reverse") % -arg('l') % libs] | grep ["^d"];
run(pipe); // or: // pipe(); // or: // ls();
I would strongly disagree with that. C++ is too "cryptic" enough so having simple, clear and readable interfaces for average programmer is very critical. If it had such interface it would get my vote off.
This approach provide a declarative, not an imperative programming style. (A lot of work done by expression templates' inner mechanics.)
The library must support:
* i/o redirection * process piping; [implicit pipes] * named/unnamed pipe objects; [explicit pipes] * runned processes surfing (boost::process::processes_iterator): - CreateToolhelp32Snapshot()/Process32First()/Process32Next() API functions in Windows - /proc filesystem parsing in linux/some unix * loaded shared libraries surfing (boost::process::modules_iterator): - CreateToolhelp32Snapshot()/Module32First()/Module32Next() API functions in Windows - /proc filesystem parsing in linux/some unix * child processes surfing (boost::process::children_iterator) - parsing processes_iterator's range results - /proc filesystem parsing in linux/(perhaps) some unix - empty iterator range by default * runned thread per process surfing (boost::process::threads_iterator) - filtered results of CreateToolhelp32Snapshot()/Thread32First()/Thread32Next() API functions in Windows - /proc filesystem parsing in linux (see /proc/[pid]/task filesystem branch since linux 2.6.0-test6) - empty iterator range by default * runned thread surfing (boost::threads_iterator) - CreateToolhelp32Snapshot()/Thread32First()/Thread32Next() API functions in Windows - FOR-EACH process IN system: back_inserter(boost::process::threads_iterator(process), boost::process::threads_iterator(), threads); * (any) process stats * process creation * daemonization - through Service API on Windows * process security and privacy aspects (probably this is subject for an another separate library, part of which must be integrated with the Boost.Process)
There are many nice features features that may be useful but: 1. Most of them are not "must-to-have" for process library, there may be other libraries that do it but not boost.process. 2. Many things like service handling are just not possible or feasible to implement in cross platform way. Windows services and Posix deamons are too different. 3. Same for security model on Windows and POSIX OS so making "same" interface would give nothing. For example try to loose process rights under Windows like it is done under Unix platform. Artyom
On 20.02.2011 1:49, Artyom wrote:
I would strongly disagree with that. C++ is too "cryptic" enough so having simple, clear and readable interfaces for average programmer is very critical.
Which code is simpler and more clear? auto pipe = ls [--arg("reverse") % -arg('l') % "/usr/lib"] | grep ["^d"]; pipe(); or: struct redirect_to { explicit redirect_to(boost::process::handle h) : h_(h) {} boost::process::stream_ends operator () (boost::process::stream_type) const { return boost::process::stream_ends(h_, boost::process::handle()); } private: boost::process::handle h_; }; template<typename T> T identity(T value) {return value;} // somewhare in main(): process::stream_ends ends = process::behavior::pipe() (process::output_stream); std::string ls_path = process::find_executable_in_path("ls"); std::vector<std::string> ls_args = boost::assign::list_of("--reverse")("-l")("/usr/lib"); process::context ls_ctx; ls_ctx.streams[process::stdout_id] = boost::bind(identity<process::stream_ends>, ends); process::child ls = process::create_child(ls_path, ls_args, ls_ctx); std::string grep_path = process::find_executable_in_path("grep"); std::vector<std::string> grep_args = boost::assign::list_of("^d"); process::context grep_ctx; grep_ctx.streams[process::stdin_id] = redirect_to(ends.parent); process::child grep = process::create_child(grep_path, grep_args, grep_ctx); (...a rhetorical question)
I actually dig the EDSL way :) It conveys enough semantic information adn is clear to w/e is actually tryign to work with process. I wonder if errors can't be caught by analyzign the whoel expression before hand.
Which code is simpler and more clear?
auto pipe = ls [--arg("reverse") % -arg('l') % "/usr/lib"] | grep ["^d"]; pipe();
or:
Or maybe something like command_line_params ls_params; ls_params.add("--reverse"); ls_params.add("-l"); command_line_params grep_params; grep_params.add("^d"); grep.stdin().connect(ls.stdout()); ls.spawn(ls_params); grep.spawn(grep_params); +- something like that. I'm not telling that Boost.Process's interface is perfect and in fact in the review I noticed that it is not clear and hard to understand. However what you suggest is terribly cryptic. It may be seems to be clear but actually is is very wired construction that hard to understand and would likely lead to very bad error messages. Especially because rm [ "foo.txt" % "bar.txt" ] Would not work... Artyom
Or maybe something like
command_line_params ls_params;
ls_params.add("--reverse"); ls_params.add("-l");
command_line_params grep_params; grep_params.add("^d");
grep.stdin().connect(ls.stdout());
ls.spawn(ls_params); grep.spawn(grep_params);
+- - something like that.
Or even better: pipe::connect(ls.stdout(),grep.stdin()); ls.spawn("--reverse","-l","/usr/bin"); grep.spawn("^d"); ls.wait(); grep.wait(); I think this would be much more readable and what is important maintainable. Artyom
Artyom wrote:
Or maybe something like
command_line_params ls_params;
ls_params.add("--reverse"); ls_params.add("-l");
command_line_params grep_params; grep_params.add("^d");
grep.stdin().connect(ls.stdout());
ls.spawn(ls_params); grep.spawn(grep_params);
+- - something like that.
Or even better:
pipe::connect(ls.stdout(),grep.stdin());
ls.spawn("--reverse","-l","/usr/bin"); grep.spawn("^d");
ls.wait(); grep.wait();
I think this would be much more readable and what is important maintainable.
I've posted a similar approach in the original review thread with 'arg' and 'args' classes with operator() usage inspired by boost spirit's symbols class. The operator is templated and uses boost::lexical_cast to convert to a std::[w]string as appropriate. arg("-d")(some_path_type)("-count")(123)("--do_it=abc") returns an args instance. https://goo.gl/NvRAH has some initial documentation and describes an approach that's similar to what you describe. A zip file is attached to the posting http://goo.gl/8R6cR I was thinking that Stjepan Rajko's previously reviewed Data Flow library's interface was very applicable to the child process domain. It's described at http://dancinghacker.com/code/dataflow/dataflow/introduction/dataflow.html. Jeff
However what you suggest is terribly cryptic.
I beg to differ
It may be seems to be clear but actually is is very wired construction that hard to understand and would likely lead to very bad error messages.
Especially because
rm [ "foo.txt" % "bar.txt" ]
Would not work...
Hmmm, i wonder if turning const char[N] into a custom proto terminal is not enough in fact. just have operator[] waits for a process_expression afterward.
Hi Max - Sounds like you have a competing library? Would you consider merging?
The Boost.Process must be implemented as a DSEL (probably through the Boost.Proto expression templates framework) with a nice (commonly known) syntax like: ... auto pipe = ls [--arg("reverse") % -arg('l') % libs] | grep ["^d"];
I think that's neat but it should be implemented as another layer on top of a functional/OO layer. Cheers, Gordon
On 20.02.2011 1:58, Gordon Woodhull wrote:
Sounds like you have a competing library?
Sounds like you have a competing library?
accidentally, yes :0)
Would you consider merging?
no, it's ineffective: much simpler write a new library from scratch, because the proposed library is not complete and have a "leaky wrapper" design in essential.
participants (6)
-
Artyom -
Gordon Woodhull -
Jeff Flinn -
Joel Falcou -
Max Sobolev -
Steven Watanabe