On 12 September 2016 at 14:41, Giel van Schijndel < Giel.vanSchijndel@tomtom.com> wrote:

* You assume the presence of $PATH and a current working directory. Unfortunately not all operating systems have these concepts: notably Windows CE has neither environment variables nor a current working directory. * Additionally current working directory duplicates functionality from Boost.Filesystem * You seem to always take paths (on Windows) by 'char' based strings. This will be problematic in that the Windows ("ANSI") API interprets these dependent on the current code page. Providing the ability to use 'wchar_t' based strings (or just boost::filesystem::path) should alleviate that problem because it's always UTF-16. Additionally, Windows CE, in the incarnations that I've seen, doesn't even provide the "ANSI" API, meaning the Unicode API is the only option.

I share these concerns. Also here are comments I wrote while reading the current documentation in details: Boost.Process Documentation Review ============================ In reading order: 1. Arguments/Command Style: Could you clarify if there are different semantics for cmd and exe/args styles? I assume that it is purely syntactic sugar for C++. 2. Synchronous I/O: "This comes with the danger of dead-locks, so it should be used with care. See the the faq for details. " Suggestion: replace "this" by "Communicating with child through pipes" to be clear. Also, move the caution before the example. 3. I see no warning about async_pipe allowing dead-locks. I think that you should clarify if or not using async_pipe 4. Starting a process: It is not clear to me what character types are supported by system(). Also, in general, what are the advantages of using boost::process::system over std::system? 5. I had a hard time finding reference docs to the different names in the tutorial, because there is no link in the functions and classes names. 6. Does Boost.Process process starting functions allow passing boost::filesystem::paths or anything that looks like it? 7. I/O: "The default depends on the system, but usually this will just write it to the output of the launching process." I don't understand what "this" is. 8. Why not name the null device null_device instead of null? "null" looks potentially misleading. 9. First example in I/O: maybe I'm wrong but it looks like the last line can never be read. Or maybe I do not understand when exactly does c.running() returns false. Or maybe I'm thinking with windows console that can, if I remember correctly, continue printing output while the command's process is already finished. 10. In the same example, isn't there a race between the c.running() and the other conditions? What if it becomes false after being called, but before calling getline? Same question for the second example. 11. Asynchronous I/O: Boost.Asio name occurences in the text should have a link to it's documentation. 12. In the example, am I correct that cout << fut.get() << endl; should be cout << data.get() << endl; 13. Child, Waiting: "This class" -> "Child" ? "If the process exits, the exit_code will be stored inside the class." Which process? 14. Termination: "Another way to stop the process is to terminate it." Add an example line, like in the previous section, so that it's clear that "terminate" is an actual function. "It is implemented via TerminateProcess and kill -9 on posix." -> "It is implemented via `TerminateProcess` on Windows and `kill -9` on Posix." Also please provide links to their documentations in case the reader wants to know their behaviour. 15. I would prefer you to clarify in this section that this implementation of terminate() will not notify the child process (through C signals on posix or other api on windows) before ending it, and if the user want such notification, point to other some other doc about this subject. 16. Exit Code: "The latter can be checked by calling is_running." -> "Checking if a child runs or exited can be done using is_running()" 17. RAII: "The child will automatically terminate the child process on destruction..." -> "A child object will automatically terminate the child process it represents on destruction..." 18. The pipe section makes me wonder: if each time you use a synchrounous pipe you enable a potential deadlock which is basically unavoidable, why even allow it? Why not allow only the async_pipe? 19. Async Pipes: Please clarify the type of the exception that could be thrown on windows, or add a link to a documentation about the details of this warning. 20. The Group section explains how a Group is implemented but not what a Group is. Is it a group of child processes? Does it work the same on all platforms? 21. What is an environnement? I have an idea of it of course but it took me some time to even learn about what it is when I first started manipulating processes. So I think it's a good section to explain what we are talking about when we talk about the environnement, what kind of data from where. Maybe a link to another page clarifying it would be nice. 22. " It essentially works like a shared_ptr, though several copies may not represent the changes done by one." So...it does not work at all like a shared_ptr? 23. this_process: "Get the process id of the current process." Is it an arbitrary value provided by the OS or something with values specific to boost::process? 24. Please add a short explaination of in which case the native_handle can be useful, maybe mentionning Windows process APIs as a good exemple. 25. Is there a reason why pwd() and path() doesn't return boost::filesystem::path objects? 26. What is a property? When/where can they be used? It is not explained at all. I can guess from the rest of the examples and my experience with other boost libraries but it's still not super clear to the newcomer. 27. Are there incompatibilities between the different properties? Or are they all combinable? 28. "This will disable the exception." - What exception? Maybe clarify "any exception that might be thrown by the process launcher function"? 29. "The throw_on_error property will enable the exception when launching a process. It is unnecessary by default, but may be used, when an additional error_code is provided." I do not know at all what this is about. Maybe develop? 30. io_service and throw_on_error sections lacks a simple code example. 31. The I/O syntax needs to be explained. I'm talking about (something & somethingelse) > text Instead of relying on just an example, clarify - where this syntax can be used; - what should be where (in the parenthesis? are the parenthesis necessary? what is right/left of the arrow?) - if it imply some special semantic; 32. Why is there 2 different Child sections? Why are they separate? 33. What is the equivalent process launcher function that the child constructor's behaviour matches? 34. Consider moving the coroutine informations in a separate page. 35. Extensions: "The are called extensions because they are not part of the core library (i.e. the portable part)." -> "The are called extensions because they are not in the portable part of the library." 36. Handlers: Do you mean that on_setup, on_error and on_succes are actually available on all the supported platforms? And they only differ in their behaviour because of the platform-specific ways they are implemented? This section is not very clear as later you talk about additional platform-specific handlers. 37. Posix Extensions: Where is the doc of the additional handlers? -------- Note that I didn't try yet to use the library, but intend to before the review week. Joël Lamotte

On 14 September 2016 at 18:08, Klemens Morgenstern < klemens.morgenstern@gmx.net> wrote:

Am 14.09.2016 um 16:29 schrieb Klaim - Joël Lamotte:

...

I share these concerns.

It's not easy to solve, it would probably look like this:

child c("äöü", windows::unicode);

Looks ugly but I'm not specialist in solving this problem Would it be a problem to just allow wstring too, and like boost::filesystem, assume it's UTF-16's for window's wide API on Windows?

I do however think, that such cases are rather rare for processes, while they might be regular for filesystems.

Unfortunately, as soon as you launch processes on windows, you must assume that the name of the process might have unicode characters, so in general it is better to use the wide API (UTF-16 like) anyway. As allow providing bfs::paths I guess that it's taken care of, but then the internals of boost::process which use the windows API must use the internal representation of bfs::path (through the native() function) which do store pathsin UTF-16.

Also here are comments I wrote while reading the current documentation in

...

details:

Thank you very much, that really helps. I commented a few points; but you're right I should link to more, that will help. Other than that, I'll work your comments into the documentation.

Good!

...

Boost.Process Documentation Review ============================

In reading order:

1. Arguments/Command Style: Could you clarify if there are different semantics for cmd and exe/args styles? I assume that it is purely syntactic sugar for C++.

That's actually written there:

"The cmd style will interpret the string as a sequence of the exe and arguments and parse them as such, while the exe-/args-style will interpret each string as an argument."

It's actually not syntactic sugar, because the call of CreateProcess is different on windows. But yeah, I should add this.

4. Starting a process:

...

It is not clear to me what character types are supported by system(). Also, in general, what are the advantages of using boost::process::system over std::system?

Uhm, you can path any of the properties. I thought that was obvious by the examples. It's basically an extended std::system.

Well.. 5. I had a hard time finding reference docs to the different names in the

...

tutorial, because there is no link in the functions and classes names.

6. Does Boost.Process process starting functions allow passing boost::filesystem::paths or anything that looks like it?

You can use this as a parameter for "exe", and for I/O. I.e. you can to this:

fs::path log="log.txt", out="out.txt", in="in.txt", _exe="stuff.exe";

child c(exe=_exe, std_out>out, std_inlog); child c2(_exe, std_out>out, std_inlog);

Those usages are actually mentioned in the doc.

...I guess a lot of my remarks are pointing at some information being explained latter in the doc, that might have been explained when first showed to help with understanding through reading the tutorial.

...

8. Why not name the null device null_device instead of null? "null" looks potentially misleading.

Possibly, but it's shorter. Also, since nullptr is a keyword, what would it lead you to think? Btw: I orginally wanted to use nullptr here, but that would lead to confusion between closing & redirecting to null.

I was suggesting null_device because you describe boost::process::null as being a null device.

9. First example in I/O: maybe I'm wrong but it looks like the last line

...

can never be read. Or maybe I do not understand when exactly does c.running() returns false. Or maybe I'm thinking with windows console that can, if I remember correctly, continue printing output while the command's process is already finished.

10. In the same example, isn't there a race between the c.running() and the other conditions? What if it becomes false after being called, but before calling getline? Same question for the second example.

I think you're right. I know why I don't use Sync I/O.

Maybe rewriting it using handlers instead of relying on a racy check would be better?

12. In the example, am I correct that

...

cout << fut.get() << endl;

should be

cout << data.get() << endl;

Yes you are.

15. I would prefer you to clarify in this section that this implementation

...

of terminate() will not notify the child process (through C signals on posix or other api on windows) before ending it, and if the user want such notification, point to other some other doc about this subject.

Oh, well that will be frustrating, because I can only point to a section where I explain why that's not possible. Would make sense for the review though.

Yes, I think it's an important think to clarify whatever the way.

...

18. The pipe section makes me wonder: if each time you use a synchrounous pipe you enable a potential deadlock which is basically unavoidable, why even allow it? Why not allow only the async_pipe?

That's due to the way the OS are working. The deadlock is not specific to the library.

It makes sense because you might have an application where boost.asio is an overkill and you know how the output looks.

I see.

22. " It essentially works like a shared_ptr, though several copies may not

...

represent the changes done by one." So...it does not work at all like a shared_ptr?

Uhm, yeah, that's bad. It points to the same shared data, but you need to renew it manually. At least on windows.

Maybe explaining without mentionning shared_ptr would be better, to avoid confusion.

25. Is there a reason why pwd() and path() doesn't return

...

boost::filesystem::path objects?

I already removed them because they're also in boost.filesystem. It did return std::string because it was a const char* from the system.

26. What is a property? When/where can they be used? It is not explained at

...

all. I can guess from the rest of the examples and my experience with other boost libraries but it's still not super clear to the newcomer.

Well a property is something like "exe", a property of the process.

Since you brought that up: I didn't want to call it a parameter, because that could get confusing since parameters build properties, i.e. "system("gcc", args="thingy.c", args+="-o", args+="thingy.o");", which would define two properties "exe" and "args". I couldn't call them members though, because they are not visible class-members.

Yes "property" is a beter name, though it would be clearer if you say that it's a type of object that holds a value for one of the property of the process.

...

27. Are there incompatibilities between the different properties? Or are they all combinable?

Execept for exe-args vs. cmd completely free.

...

29. "The throw_on_error property will enable the exception when launching a process. It is unnecessary by default, but may be used, when an additional error_code is provided."

I do not know at all what this is about. Maybe develop?

Yeah, inherited from process 0.5. It's basically there if you want dual error handling. This can be useful for further development, but maybe should not be documented.

...

30. io_service and throw_on_error sections lacks a simple code example.

throw_on_error should be removed and io_service does nothing on it's own. It's in the asio examples though.

...

31. The I/O syntax needs to be explained. I'm talking about (something & somethingelse) > text Instead of relying on just an example, clarify - where this syntax can be used; - what should be where (in the parenthesis? are the parenthesis necessary? what is right/left of the arrow?) - if it imply some special semantic;

Ok, thanks. This is what you do in the shell "stuff &2> log.txt", so you combine both out streams to one sink. It's only available for "(stderr & stdout) > sink".

32. Why is there 2 different Child sections?

...

Why are they separate?

Because one is the class description, the other is the launch mode.

Note that it was not completely clear to me because the class description is not a reference page and both class description and launch mode are in the same page but not all the documentation is on this page. It looks like you want both a big one page doc and separate some parts, which is a bit conflicting when reading the doc in order.

33. What is the equivalent process launcher function that the child

...

constructor's behaviour matches?

There is none, that's why there's a custom section for this. The child ctor itself does all the work, so I don't see the need to have a launcher function.

So... if I understood correctly, what you mean is that the other process launching functions could be explained in term of the child's constructor. Am I correct?

--------

...

Note that I didn't try yet to use the library, but intend to before the review week.

Awesome!

Joël Lamotte

...

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman /listinfo.cgi/boost

Am 16.09.2016 um 14:24 schrieb Klaim - Joël Lamotte:

On 14 September 2016 at 18:08, Klemens Morgenstern < klemens.morgenstern@gmx.net> wrote:

...

Am 14.09.2016 um 16:29 schrieb Klaim - Joël Lamotte:

...

I share these concerns.

It's not easy to solve, it would probably look like this:

child c("äöü", windows::unicode);

Looks ugly but I'm not specialist in solving this problem Would it be a problem to just allow wstring too, and like boost::filesystem, assume it's UTF-16's for window's wide API on Windows?

...

I do however think, that such cases are rather rare for processes, while they might be regular for filesystems.

Unfortunately, as soon as you launch processes on windows, you must assume that the name of the process might have unicode characters, so in general it is better to use the wide API (UTF-16 like) anyway.

As allow providing bfs::paths I guess that it's taken care of, but then the internals of boost::process which use the windows API must use the internal representation of bfs::path (through the native() function) which do store pathsin UTF-16.

I implemented a alpha for a wchar_t (which is UCS-2 on windows) version, which works similar to boost.filesystem. You can change the locale by calling boost::process::imbue. It's not yet tested or even implemented on posix, but it seems to work thus far. You can check it out here: https://github.com/klemens-morgenstern/boost-process/tree/codecvt If any element that requires wchar_t is passed, everything gets converted to wchar_t on windows, but on linux all wchar_t elements will be converted to char. On windows boost::filesystem::path requires wchar_t, so that case may occur rather often. Once I've implemented that on posix, I'll add more tests and documentation.

On 18 October 2016 at 13:46, Klemens Morgenstern < klemens.morgenstern@gmx.net> wrote:

Am 16.09.2016 um 14:24 schrieb Klaim - Joël Lamotte:

...

On 14 September 2016 at 18:08, Klemens Morgenstern < klemens.morgenstern@gmx.net> wrote:

Am 14.09.2016 um 16:29 schrieb Klaim - Joël Lamotte:

...

...

I share these concerns.

It's not easy to solve, it would probably look like this:

child c("äöü", windows::unicode);

Looks ugly but I'm not specialist in solving this problem

Would it be a problem to just allow wstring too, and like boost::filesystem, assume it's UTF-16's for window's wide API on Windows?

So, I implemented support for wchar_t, which is now in the library and reworked the entire documentation. Now the most information is in the reference and I added a simpler tutorial which touches on the most features. Not yet perfect, but I hope improved.

It's now in develop and you can find the doc at the usual location ( http://klemens-morgenstern.github.io/process/).

As soon as I find the time I'll look through your list of questions and check if they are all adressed.

Very nice, I will take a look as soon as I find the time. Joël Lamotte

Am 22.10.2016 um 09:17 schrieb Remi Chateauneu:

In boost.process, is there a portable way to: - Kill another process based on the pid ?

- Get the parent process id ? There is not. I know there's "getppid" in linux, but there is no similar

Yes, if you have the access rights of course. You can construct a child-object from a pid and see which methods work. thing on windows

- Get the list of subprocesses ? Maybe the whole subprocesses tree ? There is not. - Be notified when a subprocess crashes ? Well, when it exits, which includes crashes. You can do that asynchronously with boost.asio. - Get the current process user id or user name ?

It should not be very difficult to implement because several of these features are already available in some portable Python modules (psutil for example).

I tried to keep to the actual APIs and implement what both platforms provide. There are more things that can be added, especially if one is open to use the "undocumented" api functions on windows. I'd be happy to talk about additions to the library, but that will happen after the review is done. The API will not change that much anymore, so if you have ideas for additions feel free to work on them and let me know. But keep in mind: they must be on both platforms. The only thing where I accept extensions are the properties - elsewise you can just use the platform-dependent stuff directly: If the extensions are not needed in the launch-functions, you can just use the stuff directly. I.e. child c(...); auto parent_id = ::getppid(c.id()); child parent(parent_id); parent.terminate(); //boom.

Am 24.10.2016 um 23:58 schrieb Remi Chateauneu:

"Get the parent process id ?"

I found and successfully tested this on Windows 7: http://stackoverflow.com/questions/185254/how-can-a-win32-process-get-the-pi...

ULONG_PTR GetParentProcessId() // By Napalm @ NetCore2K{ ULONG_PTR pbi[6]; ULONG ulSize = 0; LONG (WINAPI *NtQueryInformationProcess)(HANDLE ProcessHandle, ULONG ProcessInformationClass, PVOID ProcessInformation, ULONG ProcessInformationLength, PULONG ReturnLength); *(FARPROC *)&NtQueryInformationProcess = GetProcAddress(LoadLibraryA("NTDLL.DLL"), "NtQueryInformationProcess"); if(NtQueryInformationProcess){ if(NtQueryInformationProcess(GetCurrentProcess(), 0, &pbi, sizeof(pbi), &ulSize) >= 0 && ulSize == sizeof(pbi)) return pbi[5]; } return (ULONG_PTR)-1;}

Neat. I hope you don't have a problem, if I do in fact consider that not straight-forward. Though to be fair, I had code like that in my library too, before I realized that the particular problem was easier to solve. Not sure however, why the function returns a pointer. It's also a bit ugly, since the Nt* functions have errorcodes different from the regular API functions - and you can run into a number of access right errors here. Well the review starts on thursday, so considering, that I might still get an urgent bugfix and have a dayjob, I'd think that's to little time to get a solid version into the library. We'll do it afterwards, but this shouldn't be a problem. So you'd have two functions: child::get_parent_id(); this_process::get_parent_id(); If you want to help me out here, the pattern is this: Implementation in boost::process::detail::windows/posix, two times, on that throws, one that uses std::error_code, and then using them in the higher layer though the namespace alias api, i.e. boost::process::api. Also the implementation must use boost.winapi, so windows.h does not get included.