Re: [boost] Boost.Process 0.5 released

Subject: Re: [boost] Boost.Process 0.5 released
From: Alexander Lamaison (awl03_at_[hidden])
Date: 2012-08-19 18:53:28

"Boris Schaeling" <boris_at_[hidden]> writes:

> On Sun, 19 Aug 2012 10:24:08 +0200, Alexander Lamaison
> <awl03_at_[hidden]> wrote:
>
> Hi Alexander,
>
>> [...]Macros.  Boost Libraries rarely have macros but when they do, it's
>> almost never for platform specific behaviour.  The whole point is that
>> that gets hidden in the implementation.  At first glance it seems to me
>> that all the uses of macros show in the documentation (including the
>> ASIO one) could be 'taken inside'.

[snip]

> For asynchronous I/O Boost.Process relies on Boost.Asio. Boost.Asio
> provides two I/O objects which are unfortunately
> platform-specific. While unfortunate I'm not sure whether this is a
> Boost.Process problem? It's just that because of Boost.Process we
> realize that we have no platform-independent class in Boost.Asio for
> native handles?

Could these not be typedefs in Boost.Process.  After all, you go on to
use them the same way.

[snip]

> But I wonder now whether I shouldn't have mentioned asynchronous I/O
> in the Boost.Process documentation as this wouldn't be a Boost.Process
> problem then. ;)

That would be a shame.  I like that you mentioned it there.  And if you
take it away, user will only ask you questions about it :P

> Asynchronously waiting for a program to exit is a similar problem but
> worse. While boost::asio::posix::stream_descriptor and
> boost::asio::windows::stream_handle are somewhat similar, waiting
> requires to use the very different I/O objects boost::asio::signal_set
> and boost::asio::windows::object_handle. The system APIs are
> unfortunately that different: While you need to catch a signal on
> POSIX you have to use a wait function on Windows. The challenge is to
> rewrite the code in the example at
> http://www.highscore.de/boost/process0.5/boost_process/tutorial.html#boost_process.tutorial.waiting_for_a_program_to_exit
> in a platform-independent way. (Actually I tried this in Boost.Process
> 0.4. We had a class there called status which had an async_wait()
> function which worked on POSIX and Windows - if you were careful. The
> implementation was rather horrible and heavily criticized.

I only vaguely remember the last review.  Can you remind me what the
problem was?

[snip]

>> Non-RAII.  IMHO, modern C++ code should not expect the caller to manage
>> the cleanup themselves.  If `discard` should only be called once the
>> (shared) child process is no longer needed then either don't share the
>> child process between instances (i.e. they become non-copyable) or
>> manage the `child` instances' resources in a shared manner e.g. a
>> shared_ptr that does whatever cleanup `discard` previously did in its
>> destructor.
>
> The challenge is here to rewrite the example from
> http://www.highscore.de/boost/process0.5/boost_process/tutorial.html#boost_process.tutorial.cleaning_up_resources
> in a platform-independent way. In this example I ignore SIGCHLD as
> that's rather easy to do. But if you come up with a
> platform-independent solution you need of course also consider that
> someone might want to clean up by fetching the exit code from the
> child process. The whole issue with signals is complicated anyway as
> it's a global setting in an application. Libraries need to cooperate
> and shouldn't steal signals or overwrite signal handlers. This looks
> like a pretty tough job to create a RAII type which works on Windows
> and POSIX?

Yuk.  At this point my only thought is how have others done it? (It must
have been done before)  What about the Java or Python runtimes?  I
suppose they get to steal the signal all for themselves which makes
their job easier.

>> Environment.  The child inherits the environment on one platform be
>> default but not the other.  This sounds like a big gotcha.  The docs say
>> "on Windows environment variables are inherited by default".  Quite so.
>> But there must be a way to stop them being inherited.  So why not do
>> that by default on Windows and only let the true Windows default
>> behaviour happen when the inherit_env initialiser is passed?  Or vice
>> versa with a suppress_env initialiser?
>
> I agree, we need more initializers here. Right now there is no
> initializer either to easily define a new set of environment
> variables. As of today you can only use inherit_env. I blame my lack
> of time for not having created more useful initializers yet. :)

My criticism wasn't directed at any lack of initialisers, just that the
default behaviour of the two platforms varied when the library could
make it match, either favouring the POSIX way of inheriting nothing by
default or the Windows way of inheriting everything.

Just a couple of other points that occurred to me:

Exceptions.  The error reporting interface of the `execute` function
looks a little unusual.  Have you looked at how Boost.Filesystem does
it?  Their functions are overloaded so that the ones taking an error
code argument don't throw but set the argument and the ones without just
throw.  No need for `set_on_error`, `throw_on_error`.

Unicode.  Again, you might want to look at how Boost.Filesystem (v3
only) handles it.  It always calls the wchar_t versions of the Windows
API and converts the strings internally using a locale.  By default the
behaviour is exactly the same but the advantage is you can pass in a
custom locale to interpret the strings.  This is particularly important
for narrow char strings in Windows which, are *not* treated as Unicode
(UTF-8) but rather the local user's code page.  The result is that you
cannot launch a program with a mix of characters from different code
pages. For example, a Greek user (whose username is presumably in Greek)
could not launch a program in his user directory and pass it a Russian
word as an argument.  With a custom UTF-8 locale object, it all works
fine. Of course, you can use the wchar_t interface to do that but that
makes it difficult to write cross-platform code which is, after all the
purpose of a cross-platform process library.  I know Artyom, for one,
has strong opinions on this. Not a priority at the moment though but I
thought I'd mention is before I forget.

Alex

--
Swish - Easy SFTP for Windows Explorer (http://www.swish-sftp.org)