Re: [boost] [boost.process] Review

Subject: Re: [boost] [boost.process] Review
From: Klaim - Joël Lamotte (mjklaim_at_[hidden])
Date: 2016-09-14 10:29:36

On 12 September 2016 at 14:41, Giel van Schijndel <
Giel.vanSchijndel_at_[hidden]> 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