Boost.Process: Formal review request
I'd like to ask for a formal review of the library which is known as Boost.Process 0.5: - Docs: http://www.highscore.de/boost/process0.5/ - Code: http://www.highscore.de/boost/process0.5/process.zip Since 2006 a lot of people spent a tremendous amount of work on a process management library for Boost. Since then Boost.Process went through two GSoC projects, had a formal review in 2011 where it got rejected, was discussed at BoostCon, went through a complete overhaul afterwards, spun off boost::asio::windows::object_handle and got a sponsorship. In fact the library is around for so long and has attracted so much interest that it could be called a de facto Boost library. People call it Boost.Process, and there are some who use the library in their projects. The main reason why Boost.Process is under construction for six years: Differences between the supported platforms Windows and POSIX. There are other libraries like Boost.Asio or Boost.Interprocess which have to deal with platform differences, too. But in their case no core functionality is affected. Classes like boost::asio::posix::stream_descriptor or boost::interprocess::windows_shared_memory are nice to have, and no one is worried that they only work on one platform. Boost.Process has to deal with platform differences in core functions. For example, resource leaking is unacceptable (zombie processes on POSIX and open HANDLEs on Windows when child processes exit). Given that platforms are very different two extreme solutions are possible: Abstract away differences at all costs. Or tell the library user to call platform-specific system functions. As none is satisfactory, a balance between these extremes has to be found. But where this balance should be exactly can be discussed with no end. Someone has to make a decision at some point, and that will be another important role for the review manager. :) Looking back at what has been accomplished since the first review, I think Boost.Process 0.5 is a huge step forward: * The library is now fully customizable. While previous versions had a function called launch() which was immutable, the current version is based on things called executors and initializers. An executor is basically a struct with member variables which will be passed as arguments to a system function to spawn a child. Initializers set those member variables. Library users pick initializers provided by Boost.Process or define their own to setup executors. * Boost.Process 0.5 is cleaner and more focused as it makes better use of other Boost libraries. For example, previous versions had their own stream classes. They were removed in the current version - Boost.Iostreams is used instead. Now Boost.Process doesn't reinvent concepts or reimplement functions anymore which belong in other libraries. All of that makes me believe it's time for a new review. Boris
Could you add an example to boost.process' documentation which demonstrates how boost.process is used together with boost.asio in order to wait asynchronously on a child process? I think it is a common pattern on POSIX systems to establish an signal handler waiting on SIGCHLD and fetching the exit code. Oliver
On Sun, 09 Dec 2012 12:37:15 +0100, Oliver Kowalke <oliver.kowalke@gmail.com> wrote:
Could you add an example to boost.process' documentation which demonstrates how boost.process is used together with boost.asio in order to wait asynchronously on a child process? I think it is a common pattern on POSIX systems to establish an signal handler waiting on SIGCHLD and fetching the exit code.
I agree. That's why I really wanted to get support for asynchronous I/O into Boost.Process. That said the example at <http://www.highscore.de/boost/process0.5/boost_process/tutorial.html#boost_process.tutorial.waiting_for_a_program_to_exit> is I think what you are looking for. :) Boris
On 2012-12-09 02:42, Boris Schaeling wrote:
Given that platforms are very different two extreme solutions are possible: Abstract away differences at all costs. Or tell the library user to call platform-specific system functions. I say: Do both.
* Abstract away all differences for those who do not care about platform specific ways of doing this or that. * Allow those who /do/ care to use platform specific methods in addition (although I would prefer wrappers for system functions). Just my 2ct. I'd fall under the first category. Regards, Roland
On Sun, Dec 9, 2012 at 2:06 PM, Roland Bock <rbock@eudoxos.de> wrote:
I say: Do both.
* Abstract away all differences for those who do not care about platform specific ways of doing this or that. * Allow those who /do/ care to use platform specific methods in addition (although I would prefer wrappers for system functions).
Just my 2ct. I'd fall under the first category.
Exactly the same for me. Joel Lamotte
On Sun, 09 Dec 2012 14:13:27 +0100, Klaim - Joël Lamotte <mjklaim@gmail.com> wrote:
On Sun, Dec 9, 2012 at 2:06 PM, Roland Bock <rbock@eudoxos.de> wrote:
I say: Do both.
* Abstract away all differences for those who do not care about platform specific ways of doing this or that. * Allow those who /do/ care to use platform specific methods in addition (although I would prefer wrappers for system functions).
Just my 2ct. I'd fall under the first category.
Exactly the same for me.
Sounds fair to me! And having added the header file boost/process/mitigate.hpp recently users have now a choice. Some might still not be happy with the options to choose from. But there is now a choice at least. :) And as #ifdefs have been mentioned in other threads in the previous months: The only use case I'm aware of where #ifdefs are ultimately required is waiting asynchronously for child processes to exit. For anything else it is possible to write cross-platform code (unless someone uses intentionally features which are only available on one platform, like file descriptors > 2 on POSIX). If anyone runs into a problem here, please let me know and I see what I can do about it. Boris
On 2012-12-12 23:47, Boris Schaeling wrote:
On Sun, 09 Dec 2012 14:13:27 +0100, Klaim - Joël Lamotte <mjklaim@gmail.com> wrote:
On Sun, Dec 9, 2012 at 2:06 PM, Roland Bock <rbock@eudoxos.de> wrote:
I say: Do both.
* Abstract away all differences for those who do not care about platform specific ways of doing this or that. * Allow those who /do/ care to use platform specific methods in addition (although I would prefer wrappers for system functions).
Just my 2ct. I'd fall under the first category.
Exactly the same for me.
Sounds fair to me! And having added the header file boost/process/mitigate.hpp recently users have now a choice. Some might still not be happy with the options to choose from. But there is now a choice at least. :) And as #ifdefs have been mentioned in other threads in the previous months: The only use case I'm aware of where #ifdefs are ultimately required is waiting asynchronously for child processes to exit. For anything else it is possible to write cross-platform code (unless someone uses intentionally features which are only available on one platform, like file descriptors > 2 on POSIX). If anyone runs into a problem here, please let me know and I see what I can do about it.
Boris The main difference in our views is that I'd use the "abstract-away-all-differences"-variant as default and the platform specific things as add-on. The current implementation is the opposite. The mitigate header is not even included by defaul, and hints towards it sound like a warning, not an encouragement to use it.
IMHO, the main reason for a Process-library in boost would be to have something that just works with a wide range of compilers and operating systems. Or, to cite boost file system: *Portability between operating systems.* * At the C++ syntax level, it is convenient to learn and use one interface regardless of the operating system. * At the semantic level, behavior of code is reasonably portable across operating systems. * Dual generic or native path format support encourages program portability, yet still allows communication with users in system specific formats. Or Scott Meyers: "Make Interfaces Easy to Use Correctly and Hard to Use Incorrectly" To me that implies: Platform specific stuff only if I really need it. Regards, Roland
On Thu, 13 Dec 2012 12:23:45 +0100, Roland Bock <rbock@eudoxos.de> wrote:
[...]The mitigate header is not even included by defaul, and hints towards it sound like a warning, not an encouragement to use it.
That's correct - exactly what I intended. :) What I try to get across is that there is a different quality of service. On the one hand there are functions like execute(), wait_for_exit() or the various initializers. They are intrinsic to Boost.Process, and I as the library maintainer guarantee that they work as documented or I'll fix it. On the other hand there are helpers in the mitigate header which maybe do what you need - without an exact definition what this should be nor a guarantee from my side that this will always be the case. There is for example the typedef boost::process::pipe_end. Depending on the platform it's a different Boost.Asio class. These Boost.Asio classes have a couple of member functions which have the same signature on all platforms. That's what makes it possible to write cross-platform code to initiate asynchronous operations. But boost::process::pipe_end doesn't guarantee that you will never need to use #ifdefs. For example, it is possible that you want to call a member function which exists only on one platform. From Boost.Process' point of view the ideal solution would be if Boost.Asio provided a cross-platform type for file descriptors/Windows handles. As this type doesn't exist Boost.Process makes a best effort - but no more.
[...]Or Scott Meyers: "Make Interfaces Easy to Use Correctly and Hard to Use Incorrectly"
I believe that's what I did. These lines of code make it clear for every C++ developer that it depends on the platform-specific type what you can do with pipe_end: #if defined(BOOST_WINDOWS_API) typedef boost::asio::windows::stream_handle pipe_end; #elif defined(BOOST_POSIX_API) typedef boost::asio::posix::stream_descriptor pipe_end; #endif pipe_end pend(io_service, file_descriptor); This code however could make you think you have a perfectly fine cross-platform type as after all it's provided by Boost.Process: boost::process::pipe_end pend(io_service, file_descriptor); But your assumption would be wrong, and I would have made it easy for you to use pipe_end incorrectly. So my hope is that by making you explicitly include the mitigate header (which defines boost::process::pipe_end) I help you not to shoot into your foot. And for those who really just want to use a seemingly cross-platform type including the header is easy enough. So much about the background information where the mitigate header comes from. :) Boris
On 2012-12-14 00:26, Boris Schaeling wrote:
On Thu, 13 Dec 2012 12:23:45 +0100, Roland Bock <rbock@eudoxos.de> wrote:
[...]The mitigate header is not even included by defaul, and hints towards it sound like a warning, not an encouragement to use it.
That's correct - exactly what I intended. :)
What I try to get across is that there is a different quality of service. On the one hand there are functions like execute(), wait_for_exit() or the various initializers. They are intrinsic to Boost.Process, and I as the library maintainer guarantee that they work as documented or I'll fix it. On the other hand there are helpers in the mitigate header which maybe do what you need - without an exact definition what this should be nor a guarantee from my side that this will always be the case.
There is for example the typedef boost::process::pipe_end. Depending on the platform it's a different Boost.Asio class. These Boost.Asio classes have a couple of member functions which have the same signature on all platforms. That's what makes it possible to write cross-platform code to initiate asynchronous operations. But boost::process::pipe_end doesn't guarantee that you will never need to use #ifdefs. For example, it is possible that you want to call a member function which exists only on one platform.
From Boost.Process' point of view the ideal solution would be if Boost.Asio provided a cross-platform type for file descriptors/Windows handles. As this type doesn't exist Boost.Process makes a best effort - but no more. Got it, here's what I'd suggest (not being very knowledgeable about Asio's file descriptor / windows handles):
a) File an asio bug / feature request (guess you already did that :-) ) b) Write a wrapper that offers only the common methods. This wrapper would become a typedef (or is eliminated completely) once the platform independent asio type is available With b) you would be able to offer a platform independent pipe_end. And if someone really needed a special feature that isn't available in the platform independent thingy, he could then use the current version #if defined(BOOST_WINDOWS_API) typedef boost::asio::windows::stream_handle pipe_end; #elif defined(BOOST_POSIX_API) typedef boost::asio::posix::stream_descriptor pipe_end; #endif This would have no impact on current users of the library, but it would make it much easier for new users. Regards, Roland
Hello Boris, I have added Process to the review schedule. Best, Ron On Dec 8, 2012, at 5:42 PM, Boris Schaeling wrote:
I'd like to ask for a formal review of the library which is known as Boost.Process 0.5:
- Docs: http://www.highscore.de/boost/process0.5/ - Code: http://www.highscore.de/boost/process0.5/process.zip
Since 2006 a lot of people spent a tremendous amount of work on a process management library for Boost. Since then Boost.Process went through two GSoC projects, had a formal review in 2011 where it got rejected, was discussed at BoostCon, went through a complete overhaul afterwards, spun off boost::asio::windows::object_handle and got a sponsorship. In fact the library is around for so long and has attracted so much interest that it could be called a de facto Boost library. People call it Boost.Process, and there are some who use the library in their projects.
The main reason why Boost.Process is under construction for six years: Differences between the supported platforms Windows and POSIX. There are other libraries like Boost.Asio or Boost.Interprocess which have to deal with platform differences, too. But in their case no core functionality is affected. Classes like boost::asio::posix::stream_descriptor or boost::interprocess::windows_shared_memory are nice to have, and no one is worried that they only work on one platform.
Boost.Process has to deal with platform differences in core functions. For example, resource leaking is unacceptable (zombie processes on POSIX and open HANDLEs on Windows when child processes exit). Given that platforms are very different two extreme solutions are possible: Abstract away differences at all costs. Or tell the library user to call platform-specific system functions. As none is satisfactory, a balance between these extremes has to be found. But where this balance should be exactly can be discussed with no end. Someone has to make a decision at some point, and that will be another important role for the review manager. :)
Looking back at what has been accomplished since the first review, I think Boost.Process 0.5 is a huge step forward:
* The library is now fully customizable. While previous versions had a function called launch() which was immutable, the current version is based on things called executors and initializers. An executor is basically a struct with member variables which will be passed as arguments to a system function to spawn a child. Initializers set those member variables. Library users pick initializers provided by Boost.Process or define their own to setup executors.
* Boost.Process 0.5 is cleaner and more focused as it makes better use of other Boost libraries. For example, previous versions had their own stream classes. They were removed in the current version - Boost.Iostreams is used instead. Now Boost.Process doesn't reinvent concepts or reimplement functions anymore which belong in other libraries.
All of that makes me believe it's time for a new review.
Boris
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
participants (5)
-
Boris Schaeling -
Klaim - Joël Lamotte -
Oliver Kowalke -
Roland Bock -
Ronald Garcia