On Mon, 20 Aug 2012 00:53:28 +0200, Alexander Lamaison <awl03@doc.ic.ac.uk> wrote:
[...]
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.
Yes, this could work. posix::stream_descriptor has a few more member functions than windows::stream_handle. But there is no difference in regard to the asynchronous read/write operations. I wonder though whether the typedef shouldn't be provided by Boost.Asio? Or instead of a typedef Boost.Asio could provide a new class boost::asio::file_descriptor_stream which could be initialized with a boost::iostreams::file_descriptor_sink or boost::iostreams::file_descriptor_source?
[...]I only vaguely remember the last review. Can you remind me what the problem was?
The main problem was to create a class which basically does what boost::asio::signal_set and boost::asio::windows::object_handle do. These classes are very different, and if you put all their code into one class it's an interesting mixture of signal handling code and Windows wait functions. ;) That signals are handled globally in an application doesn't make it easier. If you look at the two classes in the Boost.Asio documentation I guess no one would ever think that it's a good idea to merge them (it makes much more sense for posix::stream_descriptor and windows::stream_handle).
[...]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.
I think they don't need to care much about the system level. They can invent a new concept Foo, and the rest is a matter of implementation details. But I'm not sure whether this is the way to go for a Boost C++ library? What I've been trying to do in Boost.Process 0.5 is making existing cross-platform concepts platform-independent in code. I didn't try to invent new concepts. So where I didn't find cross-platform concepts I accepted the fact that platforms are different and used the appropriate C++ tool to express the difference: #ifdef. Maybe someone has a great idea how to make SIGCHLD and Windows wait functions platform-independent. I gave it a shot once, and it was rather messy. Maybe it's not a SIGCHLD vs. Windows wait function problem either but must be solved on a very different level. I don't know. I think that's a task and maybe even a library of its own. It took me already half a year to create windows::object_handle after the status class in Boost.Process 0.4 was rejected. I don't know how long it will take to solve the SIGCHLD/Windows wait function problem. And this is not the first time that it is discussed on the mailing list (only six years since the very first Boost.Process draft ;).
[...]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.
Ah, I see. Well, I think I prefer the Law of Economy: If I don't need something, I don't add it. Here it means undefined behavior if you don't use an initializer for environment variables. But if you don't care about environment variables, that's fine and there is no need to do something extra in the library.
[...]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`.
Yes, I modeled wait_for_exit() and terminate() after the functions in Boost.Filesystem. But execute() has such a different interface that I felt it makes more sense if everything is an initializer. Besides there is this issue on POSIX that you need another initializer return_on_error if you want to know somehow whether execve() failed in the child process.
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.
And I was already happy to support Unicode API functions on Windows at all. :) Thanks for this feedback and the example! I agree it would be useful if the library supported a UTF-8 locale. Sounds like a nice idea to work on once all major issues have been resolved. ;) Boris