On 8/17/06, Johan Nilsson <r.johan.nilsson@gmail.com> wrote:

Hi,

I've been wishing for a portable process management library, and this looks promising.

...

From quickly browsing the documentation, I miss (at least) two major pieces of functionality:

1) The ability to force/request the termination of a child process. 2) The possibility to specify a timeout when waiting for child processes to terminate.

Good points; both seem fairly easy to add so will do.

I might just have missed these in the docs, though. Some minor stuff (not as important for me personally):

3) The ability to set up callbacks for child termination (which I guess is related to your comments on Boost.Asio integration).

Yes, that's related to Boost.Asio.

4) Parameterization by character type (char/wchar etc) for e.g. the command_line class.

Should all strings be parametrizable? For example, the launcher class includes a string to denote the work directory; does it make sense to have that as a wstring? (I'm afraid it does since Win32's CreateProcess receives a TCHAR string as the work directory parameter...) Thanks for the suggestions! -- Julio M. Merino Vidal <jmmv84@gmail.com> The Julipedia - http://julipedia.blogspot.com/

On 8/17/06, Alex Ott <ott@jet.msk.su> wrote:

Hello

It's looks very interesting. How can i take sources in one bundle?

Thanks. I've uploaded a couple of archives here: http://www.NetBSD.org/~jmmv/process.tar.gz http://www.NetBSD.org/~jmmv/process.zip These also include pregenerated HTML documentation (only as sources in the repository) in libs/process/doc/html. Enjoy!

Julio M. Merino Vidal wrote:

...

Hello,

During the past two-three months Jeff Garland (as mentor) and I have been working on the Boost Summer of Code project whose goal was to implement a library to spawn and manage child processes. A little introduction and a link to the source code can be found here:

https://www.boost-consulting.com:8443/trac/soc/wiki/process

But you'll probably want to go straight to the documentation which includes, among many other information, a couple of tutorials and multiple examples. I'm keeping an up-to-date snapshot at the following URL:

http://www.NetBSD.org/~jmmv/process/

If it wasn't for the imminent SoC deadline (next Monday), I wouldn't present it just yet because there are still a couple of important issues that I would like to see resolved as soon as possible:

First is the lack of a "design decisions" chapter in the documentation. This is not an extremely big issue because the Wiki page linked above already contains many discussions on the library's current design. In other words, that chapter will end up summarizing the information in there and adding few new things.

Second, and very important, is the "need" to integrate the library with Boost.Asio to allow asynchronous management of communication streams and process termination. Unfortunately it does not support such asynchronous events yet, and integrating with Boost.Asio sounds like a reasonable approach to cover this area.

Anyway, I wouldn't like this to discourage you from taking a look at the documentation and code. The library is already big enough to let you get a general idea on its focus and design. You can start criticising now! ;-) Sincerely, any constructive comments will be certainly appreciated.

Thank you.

PS: I'd be great if you could attempt to run the test suite on any platform not listed in the "Platforms and compilers" chapter.

PSS: I hope to continue working on it after SoC is officially over.

-- With best wishes, Alex Ott Jet Infosystems, Head of software development group, MBA +7 (495) 411 76 01 http://www.jetsoft.ru/

-- Julio M. Merino Vidal <jmmv84@gmail.com> The Julipedia - http://julipedia.blogspot.com/

On 8/17/06, Oleg Abrosimov <beholder@gorodok.net> wrote:

Hello Julio,

First of all, I want to thank you and your mentor for the new great library. I've started to read it's docs from top to bottom and I have a few comments till now:

Hi Oleg,

1) The motivation part is too long. Process handling is a hot topic for most of us and there is no need to prove that your library is great and very useful. It was important when you made your proposal for SoC, but now your goal is to provide a very short start for your reader and potential user of your library. (Sorry for so long paragraph, hope it would be useful)

Noted.

2) why you don't abstract out things like EXIT_FAILURE/EXIT_SUCCESS? they are artifacts from C and looks ugly in C++ code. simple enum would be just enough.

I was going to do it but then reconsidered the decision because those constants are standard, aren't they? Same for STD{IN,OUT,ERR}_FILENO. I'm not really sure; maybe it is indeed a good idea to abstract all of them.

3) seems that you need to fix you quickbook code: // quickbook:begin(command-line) bp::command_line cl("svn"); cl.argument("update"); // quickbook:end(command-line)

What do you mean? I can't see the problem there.

4) command_line constructor is defined as command_line(const std::string & executable, const std::string & firstarg = "", const std::string & path = "");

docs says that executable The full path to the executable program. firstarg The program's name. If empty, the constructor sets it to be the executable's base name (i.e., skips the directory). path Set of directories where to look for the executable, if it does not include any path component.

the difference of first and second arguments are not clear and purpose of the second argument too. command_line class provides a member function called argument, so the name firstarg suggests that it is the first argument passed to the executable under question. Resolve this confusion, please.

OK.

5) the argument member function is non-intuitive. it is a noun but should be a verb like add_argument. The name proposed is even more verbose than current one, it leads to the next suggestion: use 'operator<<' to append arguments to command line object.

Agreed. I'd rather call it 'add'. And maybe also provide operator<<. Could the operator make things clearer? cl.add("foo").add("bar").add(4); cl << "foo" << "bar" << 4;

6) const arguments_vector & get_arguments(void ) const; is it really needed? in what scenario other then dump for debugging purposes?

No... I don't think it is needed.

what an invariant it tries to maintain? if there is no one, then it can be simply public: arguments_vector arguments;

But what if there is a need to maintain an invariant in the future? You couldn't do it with a public member without breaking backwards compatibility.

the one argument for member function against simple member is to ensure

I guess you mean "the one member function for argument"?

immutability (the immutable object pattern) but it is not true for arguments. they can be added. (but not removed oops!)

Yes they can be added, but then you want to make the attribute private, right? Thus breaking compatibility as mentioned above. (I'm not too fond on breaking the API but if it is common practice in Boost I don't have a problem.)

It seems that command_line tries to be both, path to executable and arguments to it. first can be made immutable and it can be proved to be a good thing (or can it?) but arguments are different. one needs to pass different arguments for the same executable in a case when it fails for previous attempt for example. May be it is worth to factor it out of command line and make command_line immutable, but arguments - mutable?

bp::command_line cl("svn"); cl.argument("update");

becomes

fs::path exec("svn"); bp::command_line_arguments args("update"); // supports up to N parameters to be passed in constructor + modifier/composition functions for reuse bp::command_line cl(exec, args);

Hmm... separating the two concepts sounds good. Maybe the executable can be completely separated from the command_line, without adding a command_line_arguments class. After all, the command line is an entity on its own that has nothing to do with the binary. I.e.: std::string exec("/my/program"); bp::command_line cl("a", "b", "c"); bp::launcher l; l.start(exec, cl);

7) const std::string & get_executable(void ) const; same questions are applied here + one more - did you considered to return boost::filesystem::path here? if yes, why it was rejected?

Yes, it was considered. It was rejected to avoid a dependency on the Boost Filesystem (binary) library because Boost.Process is completely header-based at the moment. If it goes binary, the decision shall be reconsidered. (One of the things that needs to go in the design decisions chapter...)

+ I believe that get_executable_path() would better reflect its purpose.

Not if it is separated from the class ;)

(Note, I would prefer executable_path public member)

8) static command_line shell(const std::string & command) ; from docs to this function I've realized that this function can not be used in portable way. The question is - why it is in a portable part of the library? it can be replaced by a pair of free factory functions in corresponding platform specific namespaces, like: command_line posix_shell_command(const std::string & command, const std::string & shell_name)

The function is portable in that there is no difference when using it from Windows or Unix. What is (generally) not portable is the string passed to it. E.g.: command_line c = command_line::shell("svn update"); could work equally well under Windows and Unix.

shell_name allows to pass command not only for sh, but any other platform specific shell, like python, for example.

No. The shell() constructor is supposed to provide an easy way to execute external commands without having to construct their command line argument by argument. It is very similar to the standard system(3) function which is restricted to the default system shell. Running, e.g., python is no different than executing any other application. E.g. if you want to execute "svn update *.cpp" you do not care about the interpreter used to parse this. (If you do, you must construct the command line on your own with the appropriate interpreter and its flags.) Summarizing: the basic purpose behind shell() is to ease command line construction. We thought it was a good idea because calling the 'argument' method can be cumbersome.

9) the launcher. It seems to be just a container for different process' attributes. (It's documentation shows only one function start. fix your quickbook please)

Don't know how yet. launcher inherits from the detail::launcher_base class, which is documented in the source code but not in the generate documents. Ideally, the launcher documentation in the manual could simply inherit that from its parent... Somewhat related to this, maybe it'd also be worth to make launcher_base a public class. It can be useful to people implementing a custom launcher. Not that matters any more if launcher becomes 'context' as you suggest below.

Same invariant issues applies here too. it has no invariant at all and can be a simple struct with all members are public and simply assignable without setter methods.

The current getters check for several sanity conditions (currently using assertions, but exceptions should be considered). Moving to plain attributes means that these cannot take place any more :(

bp::launcher l; l.set_stdout_behavior(bp::redirect_stream); l.set_merge_out_err(true); l.set_work_directory(dir);

would be

bp::launcher l; l.stdout_behavior = bp::redirect_stream; l.merge_out_err = true; l.work_directory = dir; //should be a fs::path object ?

Same as above. If Filesystem was used, this could certainly be a path object. And how could you convert this, which includes containers in the launcher and not just "plain" objects? bp::posix_launcher l; l.set_input_behavior(1, bp::silent_stream); l.set_input_behavior(2, bp::silent_stream); l.set_input_behavior(3, bp::silent_stream); Something like: bp::posix_launcher l; l.input_set.insert(bp::input_stream(1, bp::silent_stream)); l.input_set.insert(bp::input_stream(2, bp::silent_stream)); l.input_set.insert(bp::input_stream(3, bp::silent_stream)); or similar?

Here again two concepts are coupled: a collection of attributes or context and the action of launching the child process. if we decouple it we get the following:

bp::context cntx; cntx.stdout_behavior = bp::redirect_stream; cntx.merge_out_err = true; cntx.work_directory = dir; //should be a fs::path object ?

bp::child c = bp::launch(cntx, cl);

// was: bp::child c = l.start(cl); - the start member function looks like a strange artifact.

I like this idea. Very much in fact.

10) all relevant pieces should be parameterized against char_type and traits_type

"Relevant pieces" refers to any class including/dealing with strings?

11) docs says that child' constructor is protected, but in synopsis it is under public section...

Grr, blame BoostBook... I've seen that it has several problems formatting Doxygen, not only this one...

12) the following methods adds nothing over exposing underlying streams directly (overincapsulated again): postream & get_stdin(void) const; pistream & get_stdout(void) const; pistream & get_stderr(void) const;

Again, these do sanity checks like e.g. not allowing you to get a reference to stdin if it was not redirected by the launcher. (Yes, assertions could become exceptions here.) (With "sanity checks" I mean ensuring that the preconditions are met.)

+ they are strange because they are const, but returns non-const reference.

13) description for status class says: "This class represents the status returned by a child process after it has terminated. It contains many methods that may make no sense in some platforms, but this is done for simplicity. These methods are supported everywhere, although they may simply return fake values"

but it smells not very good. immediate question arises - do there alternatives exists? enumerate them and prove that the current solution is better.

I don't like it either, but have not payed much attention to it until recently (have been busy with other issues). I wrote a couple of days ago about this issue to my mentor with a possible solution: templatize the wait method on the return parameter so: bp::status s = c.wait<bp::status>(); bp::posix_status s = c.wait<bp::posix_status>();

I propose to make two platform-specific status classes say, posix_status and win32_status and their (set) intersection - status

then if one doesn't care about platform specific issues he writes: bp::status s = c.wait();

and if one is care about it, he writes: bp::posix_status s = c.wait(); //posix_status' constructor accepts status object. it is a simplified form of polimorphism.

I'm assuming that you'd store all the status on the 'status' class, and its children could only provide getters for additional information, right? If so, sounds better than the template approach because it's easier to use. The only "strange" situation that can arise is this: bp::status s = c.wait(); if (s.exited()) // OK, deal with exit status. else // Uh... something happened but we cannot know exactly what. Why? (I'm only afraid of people trying to use exit_status() when exited() returns false in this specific situation because they'd see that exited() never returns true under Windows...)

14) constructor status(int flags); is public. Why? Am I (as a user) allowed to construct it directly?

Oops, no. It should be private.

15) if (s.exited() && s.exit_status() == EXIT_SUCCESS) looks not so good ;-) see the issue (2). I would prefer if (s.exited() && s == status::exit_success)

or

if (s.exited() && s == bp::exit_success)

where exit_success is a const status object or object of any other special type (to avoid unsafe comparisons to int in case of using enum)

Indeed. See reply to 2 ;-)

16) one final comment: status object is already immutable. it can be enforced if child::wait() would returns const status object. after this you can eliminate most of member functions and replace em with plain members. it would simplify its usage.

See status' code. Again, getters do some sanity checks on the preconditions... About making the resulting object const, yes, sounds appropriate.

It is already too much for one letter. I'm stop here. Hope that my comments would help you to improve the library.

Of course they are. Thank you very much for your time and all the suggestions! -- Julio M. Merino Vidal <jmmv84@gmail.com> The Julipedia - http://julipedia.blogspot.com/

On 8/23/06, Oleg Abrosimov <beholder@gorodok.net> wrote:

Julio M. Merino Vidal wrote:

...

...

3) seems that you need to fix you quickbook code: // quickbook:begin(command-line) bp::command_line cl("svn"); cl.argument("update"); // quickbook:end(command-line)

What do you mean? I can't see the problem there.

this code snippet is from the library docs. such a comment adds nothing here (it can not improve understanding of code), furthermore, it clutters code and makes it less readable. That's why I've concluded that it is a quickbook misconfiguration.

Ah, I see... that's a misfeature in Quickbook. That block of code is nested in another quickbook:begin/end section; when including the outer one in the document, the inner markers are not removed. I guess I could modify my little patch to Quickbook to bypass them; it's easy :-)

Now I've realized that this stuff is not needed at all. It is used to construct the string that you pass to function shell() (I've simplified things a bit). Alternatively, you could remove arguments stuff and provide only the shell() function and leave a user with constructing the arguments string by itself. The reason is that if you are going to provide a way to simplify the process - you are trying to do too much. It is not an easy task to create a sub-library to manipulate program arguments that would satisfy the majority of us. For example, what about not only adding but also removing arguments and maybe replacing? To conclude - it is a separate task to create an arguments manipulating sub-library. You should better don't do it now. May be later...

Well... there is no way I remove the possibility to construct a command line argument by argument. As mentioned in the documentation, this is the safest way to guarantee that the arguments passed are what will really end up in the program's argv[]. But I understand your point. A possibility could be to simply bypass the command_line class and turn it into a regular collection -- e.g. a vector. We'd even have: typedef std::vector< std::string > command_line; or something like that. shell() could then be a free function that constructs such a vector. And the user could be free to construct the vector on his own if he wanted to with whichever technique he wanted.

...

The current getters check for several sanity conditions (currently using assertions, but exceptions should be considered). Moving to plain attributes means that these cannot take place any more :(

it suggests that you have an invariant here and your checks ensure it. It can be a significant improvement if this hidden invariant becomes visible and explicit in user code. in this case you'll have no need to do any sanity checks (it is not always possible, though). For example, if object obj can be used only after function foo was called you can enforce it by returning the obj from foo. and prohibit any other way to obtain an obj.

Aha.

...

I'm assuming that you'd store all the status on the 'status' class, and its children could only provide getters for additional information, right? If so, sounds better than the template approach because it's easier to use.

The only "strange" situation that can arise is this:

bp::status s = c.wait();

if (s.exited()) // OK, deal with exit status. else // Uh... something happened but we cannot know exactly what. Why?

(I'm only afraid of people trying to use exit_status() when exited() returns false in this specific situation because they'd see that exited() never returns true under Windows...)

The idea was that bp::status has all the information, but exposes publicly only the portable part. in order to obtain platform-specific info one needs to construct the platform specific status object and access it's member functions. For example, because status::exited() is not useful under win32 bp::status wouldn't contain this method.

exited() is useful to ensure that exit_status() is valid. Directly calling exit_status() (without checking exited() first) could not be portable because it could return bogus values under POSIX. See below:

...

...

where exit_success is a const status object or object of any other special type (to avoid unsafe comparisons to int in case of using enum)

Indeed. See reply to 2 ;-)

boost::tribool can be used to eliminate exited() function and all ugly comparisons with EXIT_SUCCESS/EXIT_FAILURE

But the problem is that EXIT_SUCCESS and EXIT_FAILURE are *not* the only possible values for the exit code. A program returns an integer; in fact, there are many programs that return several integers (not only 1 = EXIT_FAILURE) to denote different errors. Or you mean to turn exited() into a tribool while still keeping exit_status()? The "problem" is that signaled() and stopped() could behave differently... By the way, I've changed the code as somebody else suggested: there is now a posix_status class and the old status now only provides exited() and exit_status(). Thank you! -- Julio M. Merino Vidal <jmmv84@gmail.com> The Julipedia - http://julipedia.blogspot.com/