Re: [boost] [process] Formal Review starts today, 27 October
On 10/27/2016 08:26 AM, Antony Polukhin wrote:
- Whether you believe the library should be accepted into Boost * Conditions for acceptance
I withhold judgement for now to give the author an opportunity to respond. At the moment I am unsure whether to vote for conditional acceptance or rejection.
- Your knowledge of the problem domain
Advanced on Unix.
You are strongly encouraged to also provide additional information: - What is your evaluation of the library's: * Design
I have a couple of serious concerns. My first concern is about the use of tagged arguments, and especially the redirection operators, in constructors. Instead I propose that the library uses the Named Parameter idiom (see [1], not to be confused with what Niall labelled "tagged name parameter" idiom.) The bp::child object should not execute in the constructor, but rather via operator(). That way we can use the named parameter idiom to build up redirections and similar modifications. For example: auto compile = bp::child("c++").output(bp::null); compile("main.cpp"); // Executes My second concern is about deadlocking. The FAQ shows two ways of deadlocking. I would expect the latter (the ls example) to work without further ado because that is how any newcomer would write their code. My third concern is about the chaining of pipes between processes. The tutorial shows an example where the output of the nm command is forwarded to the c++filt command. However, the forwarding is not done directly from the nm process to the c++filt process, but goes via the parent process. This can incur a performance degradation when nm generates much output. Is it possible to chain them directly? Besides "nm myfile | c++filt" that is. I also have the following minor comments / questions. Is it possible to redirect stderr to stdout? (like 2>&1). The reference documentation only mentions this under asynchronous pipe output. The bp::starting_dir should be renamed to bp::current_path to be consistent with Boost.Filesystem. Consider renaming bp::spawn to avoid confusion with boost::asio::spawn which is how you launch coroutines with Boost.Asio. I am missing a this_process::get_id() function to return the current process identifier.
* Implementation
My major concern with the implementation is that the library is only header-only, so I cannot compile it as a static library if I wish to. There are several Boost header-only libraries that allows you to do that. The reason why I find this important for Boost.Process is because I want to avoid getting platform-specific C header files included into my project when I include a Boost.Process header. The library should throw its own process_error exception which inherits from system_error, similar to Boost.Filesystem. I did not investigate the implementation in greater detail.
* Documentation
The documentation is lacking an entire chapter with an overall description of the library and the concepts used. For instance: * What are processes and pipes? * What are the possible invocation mechanisms? Some of this is mentioned in the Design Rationale, but that is not the place where developers seek their information. * How to set search paths or the current working directory. * How to use environment variables. * this_process Tutorial / Synchronous I/O: What is the default behaviour when I/O redirections are not specified? Tutorial / Asynchronous I/O: Consider including an example with boost::asio::spawn as a way to obtain the yield context. Reference / child: Does not mention how the life-time of the object and the child process are related. For instance, the documentation on the destructor only mentions that it is a destructor. Reference / cmd: bp::cmd will parse the input string as a sequence of space-separated arguments. Is it spaces or whitespaces? Is it possible to have escapes for arguments that do contain spaces?
* Tests
Did not investigate.
* Usefulness
A process library is a very useful addition to Boost.
- Did you attempt to use the library? If so: * Which compiler(s) * What was the experience? Any problems?
Did not try.
- How much effort did you put into your evaluation of the review?
About 4-5 hours (one of which was gratuitously donated by daylight saving time.) [1] https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Named_Parameter
-----Original Message----- From: Boost [mailto:boost-bounces@lists.boost.org] On Behalf Of Bjorn Reese Sent: 30 October 2016 13:22 To: boost@lists.boost.org Subject: Re: [boost] [process] Formal Review starts today, 27 October
My first concern is about the use of tagged arguments, and especially the redirection operators, in constructors.
Instead I propose that the library uses the Named Parameter idiom (see [1], not to be confused with what Niall labelled "tagged name parameter" idiom.)
The bp::child object should not execute in the constructor, but rather via operator(). That way we can use the named parameter idiom to build up redirections and similar modifications. For example:
auto compile = bp::child("c++").output(bp::null); compile("main.cpp"); // Executes
I like it too - a lot. This method was used in a GSoC SVG plotting program where there were hundreds of potential options to set my_plot.color("red"). line_width(3).x_size(100).y_size(200) ... I felt it works very nicely, is easy to write, easy to read, and allows checks during compilation rather than when the string value is read and acted-upon. It also allows later 'calls' to overwrite earlier calls, in the way common with command line arguments. Paul --- Paul A. Bristow Prizet Farmhouse Kendal UK LA8 8AB +44 (0) 1539 561830
Am 30.10.2016 um 14:21 schrieb Bjorn Reese:
> On 10/27/2016 08:26 AM, Antony Polukhin wrote:
>
>> - Whether you believe the library should be accepted into Boost
>> * Conditions for acceptance
>
> I withhold judgement for now to give the author an opportunity to
> respond. At the moment I am unsure whether to vote for conditional
> acceptance or rejection.
Awesome, then let's do this. Thanks for checking the library out.
>
>> - Your knowledge of the problem domain
>
> Advanced on Unix.
>
>> You are strongly encouraged to also provide additional information:
>> - What is your evaluation of the library's:
>> * Design
>
> I have a couple of serious concerns.
>
> My first concern is about the use of tagged arguments, and especially
> the redirection operators, in constructors.
>
> Instead I propose that the library uses the Named Parameter idiom (see
> [1], not to be confused with what Niall labelled "tagged name parameter"
> idiom.)
>
> The bp::child object should not execute in the constructor, but rather
> via operator(). That way we can use the named parameter idiom to build
> up redirections and similar modifications. For example:
>
> auto compile = bp::child("c++").output(bp::null);
> compile("main.cpp"); // Executes
>
I personally don't like that approach though it certainly would work. I
think it's partly a matter of taste, here's my (C) association:
struct X {int std_in; const char* exe;};
X x{.std_in = 2, exe ="stuff"};
I.e. your setting properties.
Now the problem I see is this: what happens here?
auto compile = bp::child("c++");
compile.output(bp::null);
compile("main.cpp");
compile.env({"PATH", "/foo"});
So in order to do that, we'd need a child_builder class, that can be
converted, similar to boost.format. I consider this style pre C++11,
i.e. before variadics. Now granted: it seems not very C++ish, but it's a
very unique problem: most properties are inaccesible after you launched
the process, which is the reason they are not members.
The next problem is: your proposed syntax solves a very special problem,
how would I know if you want to pass the first arg (as in your example)
or the exe or anything else to the operator()? I would constraint what
you can express.
If you want to store settings, you can in fact do that:
template<typename Tuple, int ...Indexes>
child launch_impl(const std::string & file, Tuple && tup,
std::integer_sequence<Indexes...> = )
{
return child(args=file, std::get<Indexes>(tup)...);
};
template<typename Tuple
child launch(const std::string & file, Tuple && tup)
{
return launch_impl(args=file, std::forward<Tuple>(tup),
std::make_index_sequence<std::tuple_size<Tupel>::value>());
};
auto compile = std::tie("c++", std_out>null);
auto c = launch("main.cpp", std::move(compile));
//or much easier:
auto compile = [](const std::string & st){return child("c++", st,
std_out>null);};
I don't know how that would be possible with your approach. The idea of
this library is to be integrated in whatever framework you need; though
to be fair, I could've given an example of that. And you can't copy most
initializers - which also makes sense, considering, that you wouldn't
want several processes to write on the same stream.
To the other point with launching in the ctor-call: I think that's the
right way to implement RAII here. It's similar to std::thread in that
way. You can launch later, since child is movable:
child c;
//do something else
c = child("foo");
> My second concern is about deadlocking. The FAQ shows two ways of
> deadlocking. I would expect the latter (the ls example) to work
> without further ado because that is how any newcomer would write
> their code.
First of all: that is a problem with the nature of pipes. As explained
in the faq, windows doesn't provide the proper facilities like FD_CLOEXEC,
so I did not put that in, so the same happens on windows and posix. The
async api is safe in this regard, which is why I warn in the sync IO
sections.
This is not an issue with the library, but with the OS and the
workaround is asio. However: boost.asio might be an overkill in many
instances, hence it's not enforced.
>
> My third concern is about the chaining of pipes between processes. The
> tutorial shows an example where the output of the nm command is
> forwarded to the c++filt command. However, the forwarding is not done
> directly from the nm process to the c++filt process, but goes via the
> parent process. This can incur a performance degradation when nm
> generates much output.
>
> Is it possible to chain them directly? Besides "nm myfile | c++filt"
> that is.
>
That is what happens; the pipe just opens a file-descriptor/handle and
assigns them to the streams in both processes. This is exactly what the
shell does,
you just can additionally access the pipe from a third process if you
don't destroy it. This is directly chained, the pipe class is a thin
wrapper around the actual pipe.
> I also have the following minor comments / questions.
>
> Is it possible to redirect stderr to stdout? (like 2>&1). The reference
> documentation only mentions this under asynchronous pipe output.
If you mean to redirect the stdout to the stderr of the current process
- yes it is. This is what the FILE* overload is for, you can just write
std_out > stderr. I know it's a bit confusing, but std::cerr cannot be
used, because (1) it doesn not give access to the and secondly you can
change the rdbuf, redirecting std::cerr that way.
>
> The bp::starting_dir should be renamed to bp::current_path to be
> consistent with Boost.Filesystem.
>
Thought about that, but it's not the current_path, since the child
process can change it's execution directory.
> Consider renaming bp::spawn to avoid confusion with boost::asio::spawn
> which is how you launch coroutines with Boost.Asio.
>
Also thought about that, the other variants would be "launch" and
"execute", but they collide with older names in boost.process and spawn
reminds me of posix_spawn. Well it's not entirely correct, but that was
my assciation.
> I am missing a this_process::get_id() function to return the current
> process identifier.
>
Is defined in environment.hpp. I thought about a this_process header,
but that would just include environment.hpp anyway, so that's where it went.
>> * Implementation
>
> My major concern with the implementation is that the library is only
> header-only, so I cannot compile it as a static library if I wish to.
> There are several Boost header-only libraries that allows you to do
> that. The reason why I find this important for Boost.Process is because
> I want to avoid getting platform-specific C header files included into
> my project when I include a Boost.Process header.
Fair point, though it does only include the posix-headers, not
windows.h. If I had to include the latter, I would've done that, but I
consider the posix headers to be tolerable.
If that is an issue, I could may solve that similar to boost/winapi, but
to be honest: the posix stuff doesn't pollute that much, in my opinion.
It's mainly a header-only library, since nearly everything is a tempalte
in one form or another.
>
> The library should throw its own process_error exception which inherits
> from system_error, similar to Boost.Filesystem.
>
Not sure what this would do, the boost.filesystem error gives you more
info about which paths are concerned. This is not true for this library
so I'd need a new exception class for every error, which would be a bit
of an overkill.
> I did not investigate the implementation in greater detail.
>
>> * Documentation
>
> The documentation is lacking an entire chapter with an overall
> description of the library and the concepts used. For instance:
>
Well that's correct, but on purpose. It's a documentation meant to
explain the library, not system concepts. I chose names that correspond
to what the stuff is called in the system, and linked to the systems
documentation occasionaly. I don't think such a documentation is a book,
it should only explain what it does, not what the system does.
> * What are processes and pipes?
>
> * What are the possible invocation mechanisms? Some of this is
> mentioned in the Design Rationale, but that is not the place
> where developers seek their information.
>
You mean child/spawn/system? That's also in the tutorial under the name
"Launch Functions".
> * How to set search paths or the current working directory.
>
> * How to use environment variables.
>
> * this_process
Ok, that's so little stuff, that it's only in the reference.
>
> Tutorial / Synchronous I/O: What is the default behaviour when I/O
> redirections are not specified?
System dependent, that's in the documentation. I think normally it just
redirects to the same streams as the father process. I don't think
there's a good default to enforce.
>
> Tutorial / Asynchronous I/O: Consider including an example with
> boost::asio::spawn as a way to obtain the yield context.
I linked to the boost.asio part of the documentation. Though I
personally like coroutines very much, it's more a feature for people
already familiar with boost.asio, not one for people starting with
boost.process.
>
> Reference / child: Does not mention how the life-time of the object
> and the child process are related. For instance, the documentation on
> the destructor only mentions that it is a destructor.
The destructor has a note on it saying that it will terminate the child.
>
> Reference / cmd: bp::cmd will parse the input string as a sequence of
> space-separated arguments. Is it spaces or whitespaces? Is it possible
> to have escapes for arguments that do contain spaces?
Spaces. I don't know what you mean by the latter, but "foo arg 1 \"arg
2\"" will yield {"foo", "arg", "1", "arg 2"}. It's written in a note in
design.
>
>> * Tests
>
> Did not investigate.
>
>> * Usefulness
>
> A process library is a very useful addition to Boost.
>
>> - Did you attempt to use the library? If so:
>> * Which compiler(s)
>> * What was the experience? Any problems?
>
> Did not try.
>
>> - How much effort did you put into your evaluation of the review?
>
> About 4-5 hours (one of which was gratuitously donated by daylight
> saving time.)
>
> [1] https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Named_Parameter
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
On 31/10/2016 04:39, Klemens Morgenstern wrote:
Am 30.10.2016 um 14:21 schrieb Bjorn Reese:
The bp::child object should not execute in the constructor, but rather via operator(). That way we can use the named parameter idiom to build up redirections and similar modifications. For example:
auto compile = bp::child("c++").output(bp::null); compile("main.cpp"); // Executes
+1 for me for that style too.
I personally don't like that approach though it certainly would work. I think it's partly a matter of taste, here's my (C) association:
struct X {int std_in; const char* exe;}; X x{.std_in = 2, exe ="stuff"};
I.e. your setting properties.
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
The fourth call should fail (and probably throw), since it's impossible to change the environment after the process has been started. Otherwise that should work as you'd expect. One of the great advantages of this style of construction is that it permits conditionals in a fairly natural fashion: auto compile = bp::child("c++"); if (quiet) compile.output(bp::null); compile.arg("-c"); if (debug) compile.arg("-D_DEBUG"); compile.args("-DFOO", filename); for (auto lib : libraries) compile.arg("-l" + lib); auto child = compile(); or something like that. Or you could have a factory function that returns the un-executed builder to the caller to add additional parameters before finally actually executing it, which aids in composability.
So in order to do that, we'd need a child_builder class, that can be converted, similar to boost.format. I consider this style pre C++11, i.e. before variadics. Now granted: it seems not very C++ish, but it's a very unique problem: most properties are inaccesible after you launched the process, which is the reason they are not members.
Variadics are cool, but they contribute to code bloat, since each unique sequence of parameter types requires generating a new overload, and they require templates, which precludes private implementation. For something like this, using them seems like the wrong style to me, and a fluent builder seems like a better approach. But this path might contain bikesheds. On a related note: given a variable number of arguments in a container, how would I pass this to the existing launch functions? I don't see an example of this in the docs, and this seems like the most common case.
On 31/10/2016 04:39, Klemens Morgenstern wrote:
Am 30.10.2016 um 14:21 schrieb Bjorn Reese:
The bp::child object should not execute in the constructor, but rather via operator(). That way we can use the named parameter idiom to build up redirections and similar modifications. For example:
auto compile = bp::child("c++").output(bp::null); compile("main.cpp"); // Executes
+1 for me for that style too.
I personally don't like that approach though it certainly would work. I think it's partly a matter of taste, here's my (C) association:
struct X {int std_in; const char* exe;}; X x{.std_in = 2, exe ="stuff"};
I.e. your setting properties.
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
The fourth call should fail (and probably throw), since it's impossible to change the environment after the process has been started. Otherwise that should work as you'd expect. So you'd be replacing a compile-time error (or to be precise: an interface constraint) with a runtime-error?
One of the great advantages of this style of construction is that it permits conditionals in a fairly natural fashion:
auto compile = bp::child("c++"); if (quiet) compile.output(bp::null); compile.arg("-c"); if (debug) compile.arg("-D_DEBUG"); compile.args("-DFOO", filename); for (auto lib : libraries) compile.arg("-l" + lib); auto child = compile();
or something like that. Or you could have a factory function that returns the un-executed builder to the caller to add additional parameters before finally actually executing it, which aids in composability. Ou, that's even worse, with a chain as proposed by Bjorn, it could've at least be a compile-time construction, but you want that actually all built at runtime. This discussion was held many times before, so please don't take this as a personal attack, but here are my reasons this would be the worst design decisions: It would incurr a runtime-overhead, require you to include and compile
Am 31.10.2016 um 01:18 schrieb Gavin Lambert: literally everything you may use (currently if unused boost.asio is only forward-declared if you don't include async.hpp), require more memory, the usage of variants and is not extensible at all. Just consider all the possible features in the library: every single one of them has to be a member of the child-class at all them. And of course not having a property does not only mean, that you don't have to store a object of some sort (e.g. environment), but in severla cases also will skip one (or several) function calls (e.g. dup2), which in my opinion is best solved at compile-time. And then, after you launched the thing, the average user will be utterly confused, because he assumes he can use the setters after the process has launched. It might look more natural but it just doesn't solve the problem at hand; you have an idea of what you would want to do with the library and you know how that should look. But I cannot assume I know every use-case, that's why there are so many parameters (and they might become more in future versions, i don't know). If you want you solution it will take about 30 lines to implement it using boost.process.
So in order to do that, we'd need a child_builder class, that can be converted, similar to boost.format. I consider this style pre C++11, i.e. before variadics. Now granted: it seems not very C++ish, but it's a very unique problem: most properties are inaccesible after you launched the process, which is the reason they are not members.
Variadics are cool, but they contribute to code bloat, since each unique sequence of parameter types requires generating a new overload, and they require templates, which precludes private implementation. Well I trust my optimizer with the code-bloat; if you have several similar use-cases you can just do that exactly one thing: build a small class, which constructs your process; if you have something that does not change a type (like args), that would not cause a problem; and if you want to optimize that, you can just use "extern template".
For something like this, using them seems like the wrong style to me, and a fluent builder seems like a better approach. But this path might contain bikesheds.
On a related note: given a variable number of arguments in a container, how would I pass this to the existing launch functions? I don't see an example of this in the docs, and this seems like the most common case.
It has an extra overload for std::vector, and if no other overload is known it tries to use it as a range. I.e. this would work: std::list<std::string> args; bp::args+=args; There isn't any example of this, but it's written in the reference: http://klemens-morgenstern.github.io/process/boost/process/args.html#namespa...
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 31/10/2016 14:13, Klemens Morgenstern wrote:
Ou, that's even worse, with a chain as proposed by Bjorn, it could've at least be a compile-time construction, but you want that actually all built at runtime. This discussion was held many times before, so please don't take this as a personal attack, but here are my reasons this would be the worst design decisions: [...] It might look more natural but it just doesn't solve the problem at hand; you have an idea of what you would want to do with the library and you know how that should look. But I cannot assume I know every use-case, that's why there are so many parameters (and they might become more in future versions, i don't know). If you want you solution it will take about 30 lines to implement it using boost.process.
That's a fair point, although with arguments I would expect there to be runtime overhead anyway. I think it would be exceedingly rare to not at least have some parameters passed via variables, and relatively common to have some that are entirely conditional as shown in my example.
Variadics are cool, but they contribute to code bloat, since each unique sequence of parameter types requires generating a new overload, and they require templates, which precludes private implementation.
Well I trust my optimizer with the code-bloat; if you have several similar use-cases you can just do that exactly one thing: build a small class, which constructs your process; if you have something that does not change a type (like args), that would not cause a problem; and if you want to optimize that, you can just use "extern template".
I mean that these: spawn("foo", "bar", "baz", bp::std_out > stdout); spawn("foo", bp::std_out > stdout, "bar", "baz"); spawn("foo", "bar", bp::std_out > stdout, bp::args += {"baz"}); params = {"bar", "baz"}; spawn("foo", bp::args = params, bp::std_out
stdout);
would all generate unique implementations, despite being logically equivalent (and then even more implementations if you make the redirection conditional too, or have differing argument counts). Yes, you can avoid this if you're paying attention and using a consistent style, and the compiler inlining *might* save you even if you don't, but it's a trap for the unwary.
On a related note: given a variable number of arguments in a container, how would I pass this to the existing launch functions? I don't see an example of this in the docs, and this seems like the most common case.
It has an extra overload for std::vector, and if no other overload is known it tries to use it as a range. I.e. this would work:
std::list<std::string> args; bp::args+=args;
There isn't any example of this, but it's written in the reference: http://klemens-morgenstern.github.io/process/boost/process/args.html#namespa...
Then I recommend explicitly mentioning this in the tutorial. As I said, I think this is likely to be a fairly common case in anything that does "real" work.
Am 31.10.2016 um 02:53 schrieb Gavin Lambert:
On 31/10/2016 14:13, Klemens Morgenstern wrote:
Ou, that's even worse, with a chain as proposed by Bjorn, it could've at least be a compile-time construction, but you want that actually all built at runtime. This discussion was held many times before, so please don't take this as a personal attack, but here are my reasons this would be the worst design decisions: [...] It might look more natural but it just doesn't solve the problem at hand; you have an idea of what you would want to do with the library and you know how that should look. But I cannot assume I know every use-case, that's why there are so many parameters (and they might become more in future versions, i don't know). If you want you solution it will take about 30 lines to implement it using boost.process.
That's a fair point, although with arguments I would expect there to be runtime overhead anyway. I think it would be exceedingly rare to not at least have some parameters passed via variables, and relatively common to have some that are entirely conditional as shown in my example.
Sure there's runtime-overhead and starting processes is not the fastest thing your os provides. But: you only pay for what you use, that's the main goal.
Variadics are cool, but they contribute to code bloat, since each unique sequence of parameter types requires generating a new overload, and they require templates, which precludes private implementation.
Well I trust my optimizer with the code-bloat; if you have several similar use-cases you can just do that exactly one thing: build a small class, which constructs your process; if you have something that does not change a type (like args), that would not cause a problem; and if you want to optimize that, you can just use "extern template".
I mean that these:
spawn("foo", "bar", "baz", bp::std_out > stdout); spawn("foo", bp::std_out > stdout, "bar", "baz"); spawn("foo", "bar", bp::std_out > stdout, bp::args += {"baz"}); params = {"bar", "baz"}; spawn("foo", bp::args = params, bp::std_out > stdout);
would all generate unique implementations, despite being logically equivalent (and then even more implementations if you make the redirection conditional too, or have differing argument counts). Yes, you can avoid this if you're paying attention and using a consistent style, and the compiler inlining *might* save you even if you don't, but it's a trap for the unwary.
Sure, that is always possible. That's a trade-off, but the upside is, that only what you use get's compiled in. So the code for this is quite small, if you of course have multiple times using a full asio system, then it might get critical. But then: if you had this not templated, you'd have the asio code always.
On a related note: given a variable number of arguments in a container, how would I pass this to the existing launch functions? I don't see an example of this in the docs, and this seems like the most common case.
It has an extra overload for std::vector, and if no other overload is known it tries to use it as a range. I.e. this would work:
std::list<std::string> args; bp::args+=args;
There isn't any example of this, but it's written in the reference: http://klemens-morgenstern.github.io/process/boost/process/args.html#namespa...
Then I recommend explicitly mentioning this in the tutorial. As I said, I think this is likely to be a fairly common case in anything that does "real" work.
Makes sense. I just expected a user to look into the reference after seing the args initializer. The tutorial is meant to give you an overview. And actually the better style would be: fs::path exe = foo; std::vector<std::string> args = {"bar", "42"}; bp::child c(exe, args); But this only works with std::vector and std::initializer_list.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
On 31 Oct 2016 at 13:18, Gavin Lambert wrote:
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
The fourth call should fail (and probably throw), since it's impossible to change the environment after the process has been started. Otherwise that should work as you'd expect.
The idea in my original proposal of that form (https://godbolt.org/g/38C0KH) was to enable quick and natural child worker factories e.g. auto compile = bp::child("c++"); compile.output(bp::null); bp::child::future child1 = compile("main.cpp"); compile.env({"", "/foo"}); bp::child::future child2 = compile("main.cpp"); ... more config changes and more child launches ... Niall -- ned Productions Limited Consulting http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/
Am 31.10.2016 um 01:18 schrieb Gavin Lambert:
On 31/10/2016 04:39, Klemens Morgenstern wrote:
Am 30.10.2016 um 14:21 schrieb Bjorn Reese:
The bp::child object should not execute in the constructor, but rather via operator(). That way we can use the named parameter idiom to build up redirections and similar modifications. For example:
auto compile = bp::child("c++").output(bp::null); compile("main.cpp"); // Executes
+1 for me for that style too.
I personally don't like that approach though it certainly would work. I think it's partly a matter of taste, here's my (C) association:
struct X {int std_in; const char* exe;}; X x{.std_in = 2, exe ="stuff"};
I.e. your setting properties.
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
The fourth call should fail (and probably throw), since it's impossible to change the environment after the process has been started. Otherwise that should work as you'd expect.
One of the great advantages of this style of construction is that it permits conditionals in a fairly natural fashion:
auto compile = bp::child("c++"); if (quiet) compile.output(bp::null); compile.arg("-c"); if (debug) compile.arg("-D_DEBUG"); compile.args("-DFOO", filename); for (auto lib : libraries) compile.arg("-l" + lib); auto child = compile();
or something like that. Or you could have a factory function that returns the un-executed builder to the caller to add additional parameters before finally actually executing it, which aids in composability.
I want to back up my claims here, so for comparison, here you have the documentation of boost.process 0.3 and you can see how many parameters you can actually set in the way you described: http://www.highscore.de/cpp/process/#creating Five. Five options is what you have. And no asynchronous I/O. If you look over 0.5 (with a variadic interface, http://www.highscore.de/boost/process0.5/boost_process/tutorial.html) you have this set of parameters, some of them mutually exclusive (like set_cmd, set_exe/set_args: - bind_stderr - bind_stdin - bind_stdout - bind_fd - close_fd - close_fds - close_fds_if - close_stderr - close_stdin - close_stdout - hide_console - inherit_env - notify_io_service - on_exec_error - on_exec_setup - on_fork_error - on_fork_setup - on_fork_success - on_CreateProcess_error - on_CreateProcess_setup - on_CreateProcess_success - run_exe - set_args - set_cmd_line - set_env - set_on_error - show_window - start_in_dir - throw_on_error And this is without built-in support of asynchronous I/O or /dev/null. I'm very convinced that it is a bad idea to put all of this into one class by default.
So in order to do that, we'd need a child_builder class, that can be converted, similar to boost.format. I consider this style pre C++11, i.e. before variadics. Now granted: it seems not very C++ish, but it's a very unique problem: most properties are inaccesible after you launched the process, which is the reason they are not members.
Variadics are cool, but they contribute to code bloat, since each unique sequence of parameter types requires generating a new overload, and they require templates, which precludes private implementation.
Alright to back my claims up a bit further, here's how a boost.fusion iteration looks like with O1: https://godbolt.org/g/2q4il0 Now granted, boost.process will have a few moves in there, but that's basically as small as it gets. The operator() mostly calls the syscall directly. Sadly the is not as readable when using boost.process.
On 10/30/2016 04:39 PM, Klemens Morgenstern wrote:
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
So in order to do that, we'd need a child_builder class, that can be converted, similar to boost.format. I consider this style pre C++11, i.e. before variadics. Now granted: it seems not very C++ish, but it's a
That is an odd assessment. Just because something existed before C++11 does not mean that it is outdated. For instance, this idiom is used heavily in the C++11 library sqlpp11: https://github.com/rbock/sqlpp11
very unique problem: most properties are inaccesible after you launched the process, which is the reason they are not members.
You can achieve the same for named parameters using ref-qualifiers. Something along the lines of (notice the && at the end) class child { child& env(std::string key, std::string value) &&; };
The next problem is: your proposed syntax solves a very special problem, how would I know if you want to pass the first arg (as in your example) or the exe or anything else to the operator()? I would constraint what you can express.
Arguments (those that arrive at the child main in argv[1..]) should be passed as parameters, all other attributes as named parameters. That said, a formal review is not the best venue for a design discussion.
The bp::starting_dir should be renamed to bp::current_path to be consistent with Boost.Filesystem.
Thought about that, but it's not the current_path, since the child process can change it's execution directory.
I am not sure I understand that argument. The attribute in question sets the current path of the child process at the time of invocation. Whether the child process changes that is irrelevant. Anyways, at the very least the name should use the _path suffix instead of _dir to be consistent.
The library should throw its own process_error exception which inherits from system_error, similar to Boost.Filesystem.
Not sure what this would do, the boost.filesystem error gives you more info about which paths are concerned. This is not true for this library so I'd need a new exception class for every error, which would be a bit of an overkill.
You only need one for the library, and it is quite trivial to write: class process_error : public system_error { public: process_error(error_code ec) : system_error(ec) {} process_error(error_code ec, const std::string& what) : system_error(ec, what) {} // and the remaining 4 system_error constructors }; It gives the application the ability to distinguish between system exceptions thrown from different libraries.
The documentation is lacking an entire chapter with an overall description of the library and the concepts used. For instance:
Well that's correct, but on purpose. It's a documentation meant to explain the library, not system concepts. I chose names that correspond to what the stuff is called in the system, and linked to the systems documentation occasionaly. I don't think such a documentation is a book, it should only explain what it does, not what the system does.
The documentation should introduce the concepts it is using, and describe how they relate to the library. See the Overview chapter in Boost.Asio for an example of how it can be done. It is not an introduction to network programming, but it does explain the architecture and concepts used in Boost.Asio.
Tutorial / Asynchronous I/O: Consider including an example with boost::asio::spawn as a way to obtain the yield context. I linked to the boost.asio part of the documentation. Though I personally like coroutines very much, it's more a feature for people already familiar with boost.asio, not one for people starting with boost.process.
The example is more or less useless because it pulls a yield context out of the thin air. It should either be removed, or replaced with a working example like this: boost::asio::spawn( io, [] (boost::asio::yield_context yield) { bp::system("my-program", yield); });
Reference / cmd: bp::cmd will parse the input string as a sequence of space-separated arguments. Is it spaces or whitespaces? Is it possible to have escapes for arguments that do contain spaces? Spaces. I don't know what you mean by the latter, but "foo arg 1 \"arg 2\"" will yield {"foo", "arg", "1", "arg 2"}. It's written in a note in design.
That is exactly what I was asking for. However, the note in the Design Rationale section is rather vague, and please be aware that users seldomly read the Design Rationale as it is supposed to answer why the library is designed as it is, not how to use it. Please put that in the reference documentation.
Am 05.11.2016 um 13:20 schrieb Bjorn Reese:
On 10/30/2016 04:39 PM, Klemens Morgenstern wrote:
Now the problem I see is this: what happens here?
auto compile = bp::child("c++"); compile.output(bp::null); compile("main.cpp"); compile.env({"PATH", "/foo"});
So in order to do that, we'd need a child_builder class, that can be converted, similar to boost.format. I consider this style pre C++11, i.e. before variadics. Now granted: it seems not very C++ish, but it's a
That is an odd assessment. Just because something existed before C++11 does not mean that it is outdated. For instance, this idiom is used heavily in the C++11 library sqlpp11:
Well I'm not the author of this library so let me explain why I consider it inferior. You have to possibilities to do that; either you're just chaining up sequential calls as for example in boost::program_options; then I'd be all for it - even with more functions. I.e. in that version you have all the data already in the object your just setting it for convenience. I would be against that since every possible option would need to be put into this option class. This would incurr a massive runtime and compile-time (!) overhead, which I would not feel comfortable with at all. Now the other option is (as in format) that you have an operator but you need a certain amount of calls to get the thing right, and that's what I consider outdated. I.f. "boost::format("%1, %2") % 42" yields an object of boost::format, that has an invalid state unless you use another %. I think the better style there is "boost::format("%1, %2", 42, "foo");" and get an error if you pass the wrong number of arguments. Concerning this library, you have some cross dependencies that come into play (despite the already mentioned overhead), so that one argument might depend on another one. Thus I would have to check that at runtime, which adds even more overhead. That's the reason I think this approach is outdated; I don't mean that this is always the wrong choice, but it is as in boost.format a workaround for the lack of variadic arguments and that is what I meant.
very unique problem: most properties are inaccesible after you launched the process, which is the reason they are not members.
You can achieve the same for named parameters using ref-qualifiers. Something along the lines of (notice the && at the end)
class child { child& env(std::string key, std::string value) &&; };
That's not a solution, because that would disallow something like that: child c("gcc"); c.env("PATH", "/foo"); c("main.cpp"); //now launch the thing. which was the main thing requested in an alternate syntax, and which I can see the benefit of. So you'd be replacing child c("foo", args+={"bar"}, std_out > pipe, std_err>null); with auto c = child("foo").args("bar"). std_out(pipe).std_err(null); I don't really see the benefit in that.
The next problem is: your proposed syntax solves a very special problem, how would I know if you want to pass the first arg (as in your example) or the exe or anything else to the operator()? I would constraint what you can express.
Arguments (those that arrive at the child main in argv[1..]) should be passed as parameters, all other attributes as named parameters.
That said, a formal review is not the best venue for a design discussion. And it does only cover your use-case.. It makes sense for
child compile("gcc"); compile("main.cpp"); but doesn't help here: child antlr("java"); antlr("-jar", "antlr4.jar", "language.g4"); //here I'd much rather have antlr("language.g4");
The bp::starting_dir should be renamed to bp::current_path to be consistent with Boost.Filesystem.
Thought about that, but it's not the current_path, since the child process can change it's execution directory.
I am not sure I understand that argument. The attribute in question sets the current path of the child process at the time of invocation. Whether the child process changes that is irrelevant.
Anyways, at the very least the name should use the _path suffix instead of _dir to be consistent.
It's not an argument in a discussion really, it's just what I though would make the most sense. I though it ain't CWD, i.e. current working directory, but it's the one it's starts in, hence start_dir. It's a directory not a path, because that might a path to a file.
The library should throw its own process_error exception which inherits from system_error, similar to Boost.Filesystem.
Not sure what this would do, the boost.filesystem error gives you more info about which paths are concerned. This is not true for this library so I'd need a new exception class for every error, which would be a bit of an overkill.
You only need one for the library, and it is quite trivial to write:
class process_error : public system_error { public: process_error(error_code ec) : system_error(ec) {} process_error(error_code ec, const std::string& what) : system_error(ec, what) {} // and the remaining 4 system_error constructors };
It gives the application the ability to distinguish between system exceptions thrown from different libraries.
Oh ok, that's the reason, I thought you wanted more information. Yeah, I could add this very quickly.
The documentation is lacking an entire chapter with an overall description of the library and the concepts used. For instance:
Well that's correct, but on purpose. It's a documentation meant to explain the library, not system concepts. I chose names that correspond to what the stuff is called in the system, and linked to the systems documentation occasionaly. I don't think such a documentation is a book, it should only explain what it does, not what the system does.
The documentation should introduce the concepts it is using, and describe how they relate to the library.
See the Overview chapter in Boost.Asio for an example of how it can be done. It is not an introduction to network programming, but it does explain the architecture and concepts used in Boost.Asio.
Well there seems to consensus that this needs to be added.
On 11/05/2016 02:47 PM, Klemens Morgenstern wrote:
auto c = child("foo").args("bar"). std_out(pipe).std_err(null);
That is not the syntax I have proposed. The core of my proposal is to keep attributes separate from arguments (granted, this was not explicitly mentioned before.) You mentioned elsewhere that you modelled bp::child after thread. Boost.Thread handles attributes separate from arguments. The syntax for creating attributes is less important to me, although I would strongly prefer a syntax where invalid attribute parameters results in reasonable compiler error messages (e.g. not SFINAE errors.)
child antlr("java"); antlr("-jar", "antlr4.jar", "language.g4"); //here I'd much rather have antlr("language.g4");
I do not recall having imposed any constraints on how arguments are collected. I would fine with something like this: argument with_jar = {"-jar", "antlr4.jar"}; antlr(with_jar, "language.g4");
It's not an argument in a discussion really, it's just what I though would make the most sense. I though it ain't CWD, i.e. current working directory, but it's the one it's starts in, hence start_dir. It's a directory not a path, because that might a path to a file.
Then initial_path() as used by Boost.Filesystem may be a better name.
On Sat, 5 Nov 2016 17:15:58 +0100 Bjorn Reese <breese@mail1.stofanet.dk> wrote:
On 11/05/2016 02:47 PM, Klemens Morgenstern wrote:
auto c = child("foo").args("bar"). std_out(pipe).std_err(null);
That is not the syntax I have proposed.
The core of my proposal is to keep attributes separate from arguments (granted, this was not explicitly mentioned before.)
I'm glad you brought this up - I think Niall did as well - if you look at the constructor documentation for `process::child` it is very bare. And I am not sure how to easily describe how to use it. I think maintainability is going to be difficult without a more strict contract of expected arguments and behavior.
You mentioned elsewhere that you modelled bp::child after thread. Boost.Thread handles attributes separate from arguments.
The syntax for creating attributes is less important to me, although I would strongly prefer a syntax where invalid attribute parameters results in reasonable compiler error messages (e.g. not SFINAE errors.)
child antlr("java"); antlr("-jar", "antlr4.jar", "language.g4"); //here I'd much rather have antlr("language.g4");
I do not recall having imposed any constraints on how arguments are collected. I would fine with something like this:
argument with_jar = {"-jar", "antlr4.jar"}; antlr(with_jar, "language.g4");
Again, I think Niall proposed something similar (callable). Also, I do not think partial evaluation support is necessary in boost::process - `boost::fit` (which I hope/think will be a boost lib) could partially bind command arguments or even std::bind. The syntax would be different, but once its a callable its much more flexible: auto with_jar = std::bind(child("java"), "-jar", "antlr4.jar", _1); Now `with_jar` requires a single string argument to be invoked, and invoking it executes the process and returns a handle to that process.
It's not an argument in a discussion really, it's just what I though would make the most sense. I though it ain't CWD, i.e. current working directory, but it's the one it's starts in, hence start_dir. It's a directory not a path, because that might a path to a file.
Then initial_path() as used by Boost.Filesystem may be a better name.
Lee
Am 06.11.2016 um 01:39 schrieb Lee Clagett:
> On Sat, 5 Nov 2016 17:15:58 +0100
> Bjorn Reese <breese@mail1.stofanet.dk> wrote:
>
>> On 11/05/2016 02:47 PM, Klemens Morgenstern wrote:
>>
>>> auto c = child("foo").args("bar"). std_out(pipe).std_err(null);
>> That is not the syntax I have proposed.
>>
>> The core of my proposal is to keep attributes separate from arguments
>> (granted, this was not explicitly mentioned before.)
>>
> I'm glad you brought this up - I think Niall did as well - if you look
> at the constructor documentation for `process::child` it is very bare.
> And I am not sure how to easily describe how to use it. I think
> maintainability is going to be difficult without a more strict contract
> of expected arguments and behavior.
That is because it is one of the three launch functions, but their are
actually quite clearly defined. You either pass a known type that will
be intepreted as some property (i.e. std::string->args) or you pass a
property directly, i.e. args+="Value". That is quite clear, but it is
extensible, hence the list of actually allowed types are written in the
reference at the properties, not in a central list.
@Bjorn: That's the syntax you would get if you used rvalue
qualifications, as you proposed.
>> You mentioned elsewhere that you modelled bp::child after thread.
>> Boost.Thread handles attributes separate from arguments.
>>
>> The syntax for creating attributes is less important to me, although I
>> would strongly prefer a syntax where invalid attribute parameters
>> results in reasonable compiler error messages (e.g. not SFINAE
>> errors.)
>>
>>> child antlr("java");
>>> antlr("-jar", "antlr4.jar", "language.g4"); //here I'd much rather
>>> have antlr("language.g4");
>> I do not recall having imposed any constraints on how arguments are
>> collected. I would fine with something like this:
>>
>> argument with_jar = {"-jar", "antlr4.jar"};
>> antlr(with_jar, "language.g4");
> Again, I think Niall proposed something similar (callable). Also, I do
> not think partial evaluation support is necessary in boost::process
> - `boost::fit` (which I hope/think will be a boost lib) could partially
> bind command arguments or even std::bind. The syntax would be
> different, but once its a callable its much more flexible:
>
> auto with_jar = std::bind(child("java"), "-jar", "antlr4.jar", _1);
>
> Now `with_jar` requires a single string argument to be invoked, and
> invoking it executes the process and returns a handle to that process.
>
And and why is that better then the current way of doing things?
auto antlr = [](const std::string & g4) {return child("java", "-jar",
"antlr4.jar", g4);};
//or add an redirection:
auto antlr_p = [](const std::string & g4, pipe &p){return child("java",
"-jar", "antlr4.jar", g4, std_out > p, std_err > null);};
On 6/11/2016 21:30, Klemens Morgenstern wrote:
The core of my proposal is to keep attributes separate from arguments (granted, this was not explicitly mentioned before.) [...] That is because it is one of the three launch functions, but their are actually quite clearly defined. You either pass a known type that will be intepreted as some property (i.e. std::string->args) or you pass a property directly, i.e. args+="Value". That is quite clear, but it is extensible, hence the list of actually allowed types are written in the reference at the properties, not in a central list.
I think perhaps the request is to remove the magic inferring of purpose for external argument types (eg. that passing a std::string represents an argument... sometimes) in favour of making this explicit (arguments can only be passed via "args", either constructed inline or prepared beforehand and passed in. By extension, probably the only types that the launch functions should accept are the explicit "named parameters" helper types defined in Boost.Process (or compatible extensions). This might be slightly annoying in the case of the child process executable path, though. I don't know whether it'd be worthwhile making an exception for that case or not. (I'm personally on the fence on this topic; just trying to clarify.)
Am 07.11.2016 um 01:14 schrieb Gavin Lambert:
On 6/11/2016 21:30, Klemens Morgenstern wrote:
The core of my proposal is to keep attributes separate from arguments (granted, this was not explicitly mentioned before.) [...] That is because it is one of the three launch functions, but their are actually quite clearly defined. You either pass a known type that will be intepreted as some property (i.e. std::string->args) or you pass a property directly, i.e. args+="Value". That is quite clear, but it is extensible, hence the list of actually allowed types are written in the reference at the properties, not in a central list.
I think perhaps the request is to remove the magic inferring of purpose for external argument types (eg. that passing a std::string represents an argument... sometimes) in favour of making this explicit (arguments can only be passed via "args", either constructed inline or prepared beforehand and passed in.
By extension, probably the only types that the launch functions should accept are the explicit "named parameters" helper types defined in Boost.Process (or compatible extensions). That's what boost.process 0.5 did, I extended that (in an also -
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process. Also the "sometimes" is clearly defined: the first string if not after an exe initializer will be interpreted as the argument for exe - unless there's only one, then it's a cmd. theorectically - extensible way), because I really like this syntax: bp::system("gcc","--version"); while this would look sort of overdone in the same context: bp::system(exe="gcc", args={"--version"}); This would also require additional explicit arguments, which are completely unnecessary, since the type can only be used in one way, those would be: group, asio::yield_context and asio::io_service.
This might be slightly annoying in the case of the child process executable path, though. I don't know whether it'd be worthwhile making an exception for that case or not.
I don't understand the problem with allowing both in the first place, but you're mentioning why I added that feature. So why only for the exe?
On Mon, 7 Nov 2016 01:46:42 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 07.11.2016 um 01:14 schrieb Gavin Lambert:
On 6/11/2016 21:30, Klemens Morgenstern wrote: [...] [...] [...]
I think perhaps the request is to remove the magic inferring of purpose for external argument types (eg. that passing a std::string represents an argument... sometimes) in favour of making this explicit (arguments can only be passed via "args", either constructed inline or prepared beforehand and passed in.
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process. Also the "sometimes" is clearly defined: the first string if not after an exe initializer will be interpreted as the argument for exe - unless there's only one, then it's a cmd.
I read through the explanation by Boris and the object as named parameters approach. The documentation never formally specifies a "property", or how a user could create a custom property for interacting with library (or did I miss this?). There is a section labeled "extensions" which list three functions: `on_setup`, `on_error`, and `on_success`. The documentation mentions no arguments for this functions. So how exactly do I extend this library? Did you mean the maintainers could add features easier? Separating different `properties` into unique calls would reduce the maintenance for the internal metaprogramming necessary for the current design.
By extension, probably the only types that the launch functions should accept are the explicit "named parameters" helper types defined in Boost.Process (or compatible extensions). That's what boost.process 0.5 did, I extended that (in an also - theorectically - extensible way), because I really like this syntax:
bp::system("gcc","--version");
while this would look sort of overdone in the same context:
bp::system(exe="gcc", args={"--version"});
Its worth repeating that Bjorn suggested something like: bp::system("gcc").env(...)("--version"); which does not require named parameters for the arguments properties.
This would also require additional explicit arguments, which are completely unnecessary, since the type can only be used in one way, those would be: group, asio::yield_context and asio::io_service.
I do not understand this paragraph. Are you saying that requiring explicit `arguments properties` would affect these other properties?
This might be slightly annoying in the case of the child process executable path, though. I don't know whether it'd be worthwhile making an exception for that case or not.
I don't understand the problem with allowing both in the first place, but you're mentioning why I added that feature. So why only for the exe?
Lee
Am 07.11.2016 um 13:44 schrieb Lee Clagett:
On Mon, 7 Nov 2016 01:46:42 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 07.11.2016 um 01:14 schrieb Gavin Lambert:
On 6/11/2016 21:30, Klemens Morgenstern wrote: [...] [...] [...] I think perhaps the request is to remove the magic inferring of purpose for external argument types (eg. that passing a std::string represents an argument... sometimes) in favour of making this explicit (arguments can only be passed via "args", either constructed inline or prepared beforehand and passed in.
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process. Also the "sometimes" is clearly defined: the first string if not after an exe initializer will be interpreted as the argument for exe - unless there's only one, then it's a cmd. I read through the explanation by Boris and the object as named parameters approach. The documentation never formally specifies a "property", or how a user could create a custom property for interacting with library (or did I miss this?). There is a section labeled "extensions" which list three functions: `on_setup`, `on_error`, and `on_success`. The documentation mentions no arguments for this functions. So how exactly do I extend this library? Did you mean the maintainers could add features easier? Separating different `properties` into unique calls would reduce the maintenance for the internal metaprogramming necessary for the current design. Well there are two things: first I forgot to add the signatures for the extensions and the example is wrong, but with those you can start with that.
Secondly, the I'll add a part about extensions, but that is currently not public. The interface is easily extensible, I didn't add documention for that and the necessary classes are in boost::proces::detail. The reason is, that this part of the library might change, and I don't think give I can give a guarantee that I won't break your code at some point if you use that. I.e. I don't want to, but it may be unavoidable. I'll add some implementation notes for that soon. Here's how you'd do it: struct my_handler : boost::process::detail::handler { //overload the event handlers template<typename Exec> void on_success(Executor & exec) const {cout << "it works" << endl;} }; my_handler mh; bp::child c("foo", mh); And with two template specializations & one more class you can add a marked type, which is not directly an initializer. That's a bit mor complicated, but possible.
By extension, probably the only types that the launch functions should accept are the explicit "named parameters" helper types defined in Boost.Process (or compatible extensions). That's what boost.process 0.5 did, I extended that (in an also - theorectically - extensible way), because I really like this syntax:
bp::system("gcc","--version");
while this would look sort of overdone in the same context:
bp::system(exe="gcc", args={"--version"}); Its worth repeating that Bjorn suggested something like:
bp::system("gcc").env(...)("--version");
which does not require named parameters for the arguments properties.
Do you really want me to repeat for the 42th time why this is a bad idea?
This would also require additional explicit arguments, which are completely unnecessary, since the type can only be used in one way, those would be: group, asio::yield_context and asio::io_service. I do not understand this paragraph. Are you saying that requiring explicit `arguments properties` would affect these other properties? well instead of writing
bp::group g; bp::child c("foo", g); you'd need to write bp::group g; bp::child c(cmd="foo", some_artificial_group_ref_name=g);
On Mon, 7 Nov 2016 14:26:11 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 07.11.2016 um 13:44 schrieb Lee Clagett:
On Mon, 7 Nov 2016 01:46:42 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
I read through the explanation by Boris and the object as named parameters approach. The documentation never formally specifies a "property", or how a user could create a custom property for interacting with library (or did I miss this?). There is a section labeled "extensions" which list three functions: `on_setup`, `on_error`, and `on_success`. The documentation mentions no arguments for this functions. So how exactly do I extend this library? Did you mean the maintainers could add features easier? Separating different `properties` into unique calls would reduce the maintenance for the internal metaprogramming necessary for the current design. Well there are two things: first I forgot to add the signatures for
[...] [...] [...] [...] [...] [...] [...] the extensions and the example is wrong, but with those you can start with that.
Secondly, the I'll add a part about extensions, but that is currently not public. The interface is easily extensible, I didn't add documention for that and the necessary classes are in boost::proces::detail. The reason is, that this part of the library might change, and I don't think give I can give a guarantee that I won't break your code at some point if you use that. I.e. I don't want to, but it may be unavoidable. I'll add some implementation notes for that soon.
I think defining the concept of `properties` and how it will be extended will help in the documentation and refining the implementation.
Here's how you'd do it:
struct my_handler : boost::process::detail::handler { //overload the event handlers template<typename Exec> void on_success(Executor & exec) const {cout << "it works" << endl;} };
my_handler mh; bp::child c("foo", mh);
And with two template specializations & one more class you can add a marked type, which is not directly an initializer. That's a bit mor complicated, but possible.
So I cannot modify how the process is actually launched? Or does `exec` provided to `on_setup` have a modifiable dynamically dispatch function (I didn't see this in the implementation)? I apologize for not being explicit earlier, what I noticed was that `argument properties` are different from other type of properties. They manipulate the launching of the process. Some of the other properties manipulate before/after state (`on_setup`, `on_error`, `on_success`). Does this mean that there are actually two different concepts here? A `launcher` (execv, shell, etc) concept, and a `property` concept? There is certainly some special magic for `shell`. Which leads to ... why does the `child` constructor launch the process? Is `child launch(...)` and `child shell(...)` more appropriate? Or even `child launch("foo", "args")(...properties...)`, where `properties` can only change state via `on_setup`, `on_error`, `on_success`?
[...] [...]
Its worth repeating that Bjorn suggested something like:
bp::system("gcc").env(...)("--version");
which does not require named parameters for the arguments properties. Do you really want me to repeat for the 42th time why this is a bad idea? [...]
I _think_ your issue with named parameters is that they inherently require lazy process launching, and some of the parameters cannot/should not be "bound" to an object that is later used to launch a process. Is that accurate? The remaining arguments I recall seeing: (1) not extensible, (2) inefficient, (3) access to values that should be hidden - I do not agree with. Conceptually, the only difference is the lazy nature of the named parameter approach which requires "binding" of some values to an object before launching the process. So yes, this could be undesirable if resources are cleaned up before invocation. I suspect the named parameters will be easier to implement since it does not require massive filtering and dispatching, although I could be incorrect. Named parameters also means the removal of a accept-anything constructor with potentially confusing internal errors. And it should be possible to disable named parameters by returning a new callable type, so that a parameter can only be invoked once.
I do not understand this paragraph. Are you saying that requiring explicit `arguments properties` would affect these other properties? well instead of writing
bp::group g; bp::child c("foo", g);
you'd need to write
bp::group g; bp::child c(cmd="foo", some_artificial_group_ref_name=g);
Why would the group have to be passed in the same call with the arguments? The arguments are already being copied to a std::string, so what harm is there in "binding" these arguments to an object before invoking the process? bp::group g; bp::launch("foo", "args")(g); Also, is it possibly invalid to "bind" `g` here (because it could have a dead handle by invocation time)? Lee
Am 08.11.2016 um 19:57 schrieb Lee Clagett:
On Mon, 7 Nov 2016 14:26:11 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 07.11.2016 um 13:44 schrieb Lee Clagett:
On Mon, 7 Nov 2016 01:46:42 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
I read through the explanation by Boris and the object as named parameters approach. The documentation never formally specifies a "property", or how a user could create a custom property for interacting with library (or did I miss this?). There is a section labeled "extensions" which list three functions: `on_setup`, `on_error`, and `on_success`. The documentation mentions no arguments for this functions. So how exactly do I extend this library? Did you mean the maintainers could add features easier? Separating different `properties` into unique calls would reduce the maintenance for the internal metaprogramming necessary for the current design. Well there are two things: first I forgot to add the signatures for
[...] [...] [...] [...] [...] [...] [...] the extensions and the example is wrong, but with those you can start with that.
Secondly, the I'll add a part about extensions, but that is currently not public. The interface is easily extensible, I didn't add documention for that and the necessary classes are in boost::proces::detail. The reason is, that this part of the library might change, and I don't think give I can give a guarantee that I won't break your code at some point if you use that. I.e. I don't want to, but it may be unavoidable. I'll add some implementation notes for that soon. I think defining the concept of `properties` and how it will be extended will help in the documentation and refining the implementation. Uhm ok, well it's a property of a process, I don't what's so ambiguous about that. But they're all basically private. Here's how you'd do it:
struct my_handler : boost::process::detail::handler { //overload the event handlers template<typename Exec> void on_success(Executor & exec) const {cout << "it works" << endl;} };
my_handler mh; bp::child c("foo", mh);
And with two template specializations & one more class you can add a marked type, which is not directly an initializer. That's a bit mor complicated, but possible. So I cannot modify how the process is actually launched? Or does `exec` provided to `on_setup` have a modifiable dynamically dispatch function (I didn't see this in the implementation)? Here you go. https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...
The handler modifies the executor, which then launches the function.
I apologize for not being explicit earlier, what I noticed was that `argument properties` are different from other type of properties. They manipulate the launching of the process. Some of the other properties manipulate before/after state (`on_setup`, `on_error`, `on_success`). Does this mean that there are actually two different concepts here? A `launcher` (execv, shell, etc) concept, and a `property` concept? There is certainly some special magic for `shell`.
most of the properties modify the process at launch - I think actually all on windows. You have a set of parameters you set up in the executor which then get invoked in "CreateProcess". Thus the distinction you make here is just wrong. But to be fair, the executor is not documented (yet).
Which leads to ... why does the `child` constructor launch the process? Is `child launch(...)` and `child shell(...)` more appropriate? Or even `child launch("foo", "args")(...properties...)`, where `properties` can only change state via `on_setup`, `on_error`, `on_success`?
Its worth repeating that Bjorn suggested something like:
bp::system("gcc").env(...)("--version");
which does not require named parameters for the arguments properties. Do you really want me to repeat for the 42th time why this is a bad idea? [...] I _think_ your issue with named parameters is that they inherently require lazy process launching, and some of the parameters cannot/should not be "bound" to an object that is later used to launch a process. Is that accurate? The remaining arguments I recall seeing: (1) not extensible, (2) inefficient, (3) access to values that should be hidden - I do not agree with. Conceptually, the only difference is
[...] [...] the lazy nature of the named parameter approach which requires "binding" of some values to an object before launching the process. So yes, this could be undesirable if resources are cleaned up before invocation.
Yes that is another reason, since some properties (as io_service, group) can only be taken by reference. Why you disagree with the other points is beyond me.
I suspect the named parameters will be easier to implement since it does not require massive filtering and dispatching, although I could be incorrect. Named parameters also means the removal of a accept-anything constructor with potentially confusing internal errors. And it should be possible to disable named parameters by returning a new callable type, so that a parameter can only be invoked once. Well massive filtering is an over-statement, but elsewise your correct. But: passing a wrong type gives you an error of the type "undefined refernce to boost::process::detail::initializer_tag<foo>", while the actual ugly error comes from forgetting your io_service in the sequence, but you need it. Than you get the horrible error, and I don't know a way around this - except for SFINAE, but then you get an even less explanatory error.
I do not understand this paragraph. Are you saying that requiring explicit `arguments properties` would affect these other properties? well instead of writing
bp::group g; bp::child c("foo", g);
you'd need to write
bp::group g; bp::child c(cmd="foo", some_artificial_group_ref_name=g);
Why would the group have to be passed in the same call with the arguments? The arguments are already being copied to a std::string, so what harm is there in "binding" these arguments to an object before invoking the process?
It has to be present when CreateProcess or fork is called, that's the reason. Sure you can copy it, that's why there the syntax bp::system(exe="foo", args="bar"); And you can also bind them to an object beforehand: std::vector<std::string> args = {"bar"}; bp::system(exe="foo", args=args);
bp::group g; bp::launch("foo", "args")(g);
Also, is it possibly invalid to "bind" `g` here (because it could have a dead handle by invocation time)?
How would launching work with in this code? Launching in the destructor? What would bp::launch("foo", "args") return?
On Tue, 8 Nov 2016 21:38:19 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 08.11.2016 um 19:57 schrieb Lee Clagett:
On Mon, 7 Nov 2016 14:26:11 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
[...] [...] [...] [...] [...]
I think defining the concept of `properties` and how it will be extended will help in the documentation and refining the implementation. Uhm ok, well it's a property of a process, I don't what's so ambiguous about that. But they're all basically private.
[...]
So I cannot modify how the process is actually launched? Or does `exec` provided to `on_setup` have a modifiable dynamically dispatch function (I didn't see this in the implementation)? Here you go. https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...
The handler modifies the executor, which then launches the function.
Some properties do things before and after the process is launched. Others try to manipulate the implementation defined `executor` object. Then another (`shell`) changes what process is launched via argument manipulation. Does it make sense for all of these to be taken in the same variadic argument (especially the last case)? I know there is some difficulty because the Posix fork + exec design does not work the same as way as the Windows API. But I think there is some value in specifying some basic concept of an `executor` and its relationship with a `property`. For instance, the executor will have some function (call operator) that starts a new process using internally stored string arguments and returns a handle to the new process. The string arguments should arguably be non-modifiable once the executor is created, etc.
I apologize for not being explicit earlier, what I noticed was that `argument properties` are different from other type of properties. They manipulate the launching of the process. Some of the other properties manipulate before/after state (`on_setup`, `on_error`, `on_success`). Does this mean that there are actually two different concepts here? A `launcher` (execv, shell, etc) concept, and a `property` concept? There is certainly some special magic for `shell`.
most of the properties modify the process at launch - I think actually all on windows. You have a set of parameters you set up in the executor which then get invoked in "CreateProcess". Thus the distinction you make here is just wrong.
But to be fair, the executor is not documented (yet).
Which leads to ... why does the `child` constructor launch the process? Is `child launch(...)` and `child shell(...)` more appropriate? Or even `child launch("foo", "args")(...properties...)`, where `properties` can only change state via `on_setup`, `on_error`, `on_success`?
[...] [...] [...] [...]
I _think_ your issue with named parameters is that they inherently require lazy process launching, and some of the parameters cannot/should not be "bound" to an object that is later used to launch a process. Is that accurate? The remaining arguments I recall seeing: (1) not extensible, (2) inefficient, (3) access to values that should be hidden - I do not agree with. Conceptually, the only difference is the lazy nature of the named parameter approach which requires "binding" of some values to an object before launching the process. So yes, this could be undesirable if resources are cleaned up before invocation. Yes that is another reason, since some properties (as io_service, group) can only be taken by reference. Why you disagree with the other points is beyond me.
I suspect the named parameters will be easier to implement since it does not require massive filtering and dispatching, although I could be incorrect. Named parameters also means the removal of a accept-anything constructor with potentially confusing internal errors. And it should be possible to disable named parameters by returning a new callable type, so that a parameter can only be invoked once. Well massive filtering is an over-statement, but elsewise your correct. But: passing a wrong type gives you an error of the type "undefined refernce to boost::process::detail::initializer_tag<foo>", while the actual ugly error comes from forgetting your io_service in the sequence, but you need it. Than you get the horrible error, and I don't know a way around this - except for SFINAE, but then you get an even less explanatory error.
[...] [...]
Why would the group have to be passed in the same call with the arguments? The arguments are already being copied to a std::string, so what harm is there in "binding" these arguments to an object before invoking the process?
It has to be present when CreateProcess or fork is called, that's the reason. Sure you can copy it, that's why there the syntax bp::system(exe="foo", args="bar");
I was asking whether you could store a handle to the `group` in some object whose call operator starts the process (like Niall originally mentioned). So the handle would be present, but potentially invalid depending on how it was implemented.
And you can also bind them to an object beforehand:
std::vector<std::string> args = {"bar"}; bp::system(exe="foo", args=args);
bp::group g; bp::launch("foo", "args")(g);
Also, is it possibly invalid to "bind" `g` here (because it could have a dead handle by invocation time)?
How would launching work with in this code? Launching in the destructor? What would bp::launch("foo", "args") return?
`launch` returns a callable that accepts 0 or more `properties` to launch the process. Example: class launcher { std::string exe_; std::vector<std::string> args_; public: launcher(std::string exe, std::vector<std::string> args) : exe_(std::move(exe)), args_(std::move(args)) { } template<typename... Props> child operator()(Props&&... props) const { return detail::exec(exe_, args_, std::forward<Props>(props)...); } }; struct launch_ { template<typename... Args> launcher operator()(std::string exe, Args&&... args) const { return {std::move(exe), {std::forward<Args>(args)...}}; } }; constexpr launch_ launch{}; // syntax: // launch("ls", "-la", "/")(stdout(my_pipe)); I don't see a good reason for the `child` class to actually launch the process. A global or static function that returns a `child` object should do it instead. This would remove the need for the `shell` property, which would be a distinct function. Also, is there an advantage to allowing string arguments anywhere in the function call? A single `std::vector<std::string>` argument can be initialized implicitly with a `{...}`. This would easily separate the arguments from the rest of the properties without the lazy binding provided above: shell(std::string cmd, Props&&...) launch(std::string exe, std::vector<std::string> args, Props&&...) Which can be invoked like: bp::shell("ls -la /", bp::stdout(my_pipe)); bp::launch("ls", {"-la", "/"}, bp::stdout(my_pipe)); bp::launch("foo", {}, bp::stdout(my_pipe)); Is requiring a `{}` pair less desirable than the variadic syntax? Lee
So I cannot modify how the process is actually launched? Or does `exec` provided to `on_setup` have a modifiable dynamically dispatch function (I didn't see this in the implementation)? Here you go. https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...
The handler modifies the executor, which then launches the function.
Some properties do things before and after the process is launched. Others try to manipulate the implementation defined `executor` object. Then another (`shell`) changes what process is launched via argument manipulation. Does it make sense for all of these to be taken in the same variadic argument (especially the last case)? Yes I know there is some difficulty because the Posix fork + exec design does not work the same as way as the Windows API. But I think there is some value in specifying some basic concept of an `executor` and its relationship with a `property`. For instance, the executor will have some function (call operator) that starts a new process using internally stored string arguments and returns a handle to the new process. The string arguments should arguably be non-modifiable once the executor is created, etc. The string are only set after the executor is created, I don't know how
Why would the group have to be passed in the same call with the arguments? The arguments are already being copied to a std::string, so what harm is there in "binding" these arguments to an object before invoking the process? It has to be present when CreateProcess or fork is called, that's the reason. Sure you can copy it, that's why there the syntax bp::system(exe="foo", args="bar"); I was asking whether you could store a handle to the `group` in some object whose call operator starts the process (like Niall originally mentioned). So the handle would be present, but potentially invalid depending on how it was implemented. Yes it could, but I stated enought times why this would be a horrible idea.
And you can also bind them to an object beforehand:
std::vector<std::string> args = {"bar"}; bp::system(exe="foo", args=args);
bp::group g; bp::launch("foo", "args")(g);
Also, is it possibly invalid to "bind" `g` here (because it could have a dead handle by invocation time)?
How would launching work with in this code? Launching in the destructor? What would bp::launch("foo", "args") return? `launch` returns a callable that accepts 0 or more `properties` to launch the process. Example:
class launcher { std::string exe_; std::vector<std::string> args_; public: launcher(std::string exe, std::vector<std::string> args) : exe_(std::move(exe)), args_(std::move(args)) { }
template<typename... Props> child operator()(Props&&... props) const { return detail::exec(exe_, args_, std::forward<Props>(props)...); } }; struct launch_ { template<typename... Args> launcher operator()(std::string exe, Args&&... args) const { return {std::move(exe), {std::forward<Args>(args)...}}; } }; constexpr launch_ launch{};
// syntax: // launch("ls", "-la", "/")(stdout(my_pipe)); Yeah, that still doesn't work. When do you launcht process? Please think
that would work. Also the flags need to be accumulated through disjunction, I don't see how it would be better to this on construction time, and not in on_setup. It's internal, so why bother with a strange workaround there? those Ideas through, if you want me to take them serious. If you launch at the call of "launch("ls", "-la", "/") than you couldn't bind stdout. If you launch after the binding of stdout, how would you append another paremeter? It's broken.
I don't see a good reason for the `child` class to actually launch the process. A global or static function that returns a `child` object should do it instead. This would remove the need for the `shell` property, which would be a distinct function. Well we had a global function for that in previous versions, but that wasn't like that much; especially since people got reminded of std::async. I have to agree, it's not the common C++ idiom to use functions to construct object, you also don't write
std::thread t = std::launch_thread([]{}); Also this would be bad, and probably a common mistake. It would launch the process and immediatly terminate it. bp::launch("foo"); Secondly, I'm not sure why shell would be a distrinct function - that doesn make sense, you can use it in all three launch-functions. A shell ist just another process, why would I handly it any different?
Also, is there an advantage to allowing string arguments anywhere in the function call? A single `std::vector<std::string>` argument can be initialized implicitly with a `{...}`. This would easily separate the arguments from the rest of the properties without the lazy binding provided above:
shell(std::string cmd, Props&&...) launch(std::string exe, std::vector<std::string> args, Props&&...)
Which can be invoked like:
bp::shell("ls -la /", bp::stdout(my_pipe)); bp::launch("ls", {"-la", "/"}, bp::stdout(my_pipe)); bp::launch("foo", {}, bp::stdout(my_pipe));
Is requiring a `{}` pair less desirable than the variadic syntax? Yes there is, bp::system("gcc", "main.cpp", "-o", "main.o"); looks awesome. Also keep in mind, that you have the cmd-syntax, which is a single string and that you can path a boost::filesystem path. What you are basically proposing are positional arguments, which might make sense for exe/args or cmd, but not for the test of that. There is no logical order of the other arguments and secondly it is variadic to allow the user to skip some arguments.
Also I don't think arguments are any different from the other properties, or at least I don't consider them to be. It's a value that determines how a process behaves that I pass at launch, so why would that be any different from the environment? And yes, args args are optional. Since you're coming up with so many ideas, I would encourage you to start your own library, which would allow us to talk about some tests way of doing things. Currently you're only Feel free to fork mine or the older version from Boris. That might be a good proof-of-concept.
On Wed, 9 Nov 2016 14:50:19 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
[...] [...]
Some properties do things before and after the process is launched. Others try to manipulate the implementation defined `executor` object. Then another (`shell`) changes what process is launched via argument manipulation. Does it make sense for all of these to be taken in the same variadic argument (especially the last case)? Yes I know there is some difficulty because the Posix fork + exec design does not work the same as way as the Windows API. But I think there is some value in specifying some basic concept of an `executor` and its relationship with a `property`. For instance, the executor will have some function (call operator) that starts a new process using internally stored string arguments and returns a handle to the new process. The string arguments should arguably be non-modifiable once the executor is created, etc. The string are only set after the executor is created, I don't know how that would work. Also the flags need to be accumulated through disjunction, I don't see how it would be better to this on construction time, and not in on_setup. It's internal, so why bother with a strange workaround there?
What I noticed in the source is that some of the properties require special logic because they are manipulating other properties instead of the "executor". For example, the `shell` property is manipulating the path and arg properties but nothing else. Earlier I mentioned on working out a loose way of specifying a `property`. I think such a specification would not consider properties that only manipulate other properties to be a property (since it could do that separately from the `child` constructor).
[...] [...]
I was asking whether you could store a handle to the `group` in some object whose call operator starts the process (like Niall originally mentioned). So the handle would be present, but potentially invalid depending on how it was implemented. Yes it could, but I stated enought times why this would be a horrible idea.
Yes, and I disagreed with your analysis. I recall the primary objection was poor performance - specifically that space for each type of possible property was needed. This is not true, the binding approach (at least how I saw it), would return a new callable type after each binding. In other words, the binding was conceptually calling `push_back` on a fusion like tuple. Moves + ref-qualifiers meant that dynamic memory would not copied in many cases. So `launcher("foo").group(...).stdout(...)` would return the equivalent of `tuple<launcher, group, stdout>` while moving `std::string` memory as needed. After thinking about it, all that I (and I think Bjorn and Niall) were trying to accomplish is separating the path and arguments from the remainder of the properties. If the path and arguments were in a single property, `shell` and `cmd` could return that property instead of requiring custom logic internally.
`launch` returns a callable that accepts 0 or more `properties` to launch the process. Example:
class launcher { std::string exe_; std::vector<std::string> args_; public: launcher(std::string exe, std::vector<std::string> args) : exe_(std::move(exe)), args_(std::move(args)) { }
template<typename... Props> child operator()(Props&&... props) const { return detail::exec(exe_, args_, std::forward<Props>(props)...); } }; struct launch_ { template<typename... Args> launcher operator()(std::string exe, Args&&... args) const { return {std::move(exe), {std::forward<Args>(args)...}}; } }; constexpr launch_ launch{};
// syntax: // launch("ls", "-la", "/")(stdout(my_pipe)); Yeah, that still doesn't work. When do you launcht process? Please
[...] [...] [...] think those Ideas through, if you want me to take them serious. If you launch at the call of "launch("ls", "-la", "/") than you couldn't bind stdout. If you launch after the binding of stdout, how would you append another paremeter? It's broken.
How is it broken? Look at the code snippet, the second function call takes a variadic argument list like the `child` constructor. The string arguments are the only thing "bound" to an object. So appending another property would be done like: launch("ls, "-la", "/")(stdout(my_pipe1), stdin(my_pipe2)); That style is simply separating string arguments into a separate variadic function call. Or another way of looking at it - there is a single `property` for specifying the path and args that has a fixed positional location.
I don't see a good reason for the `child` class to actually launch the process. A global or static function that returns a `child` object should do it instead. This would remove the need for the `shell` property, which would be a distinct function. Well we had a global function for that in previous versions, but that wasn't like that much; especially since people got reminded of std::async. I have to agree, it's not the common C++ idiom to use functions to construct object, you also don't write
std::thread t = std::launch_thread([]{});
Also this would be bad, and probably a common mistake. It would launch the process and immediatly terminate it.
Why would it immediately terminate the process? Or do you mean a non-joined `child` should terminate the process like `std::thread`? Admittedly, I did forget about that case - its probably best to follow `std::thread` closely and terminate the current process if not `join`ed or `detach`ed. I don't there is anything wrong with a function returning a `child`, provided the behavior of ignoring the `child` is well documented and defined. However, there is little difference to the code snippet above and: bp::child c1(bp::exec("ls", "-la", "/"), bp::stdout(my_pipe1)); bp::child c2(bp::shell("ls", "-la", "/"), bp::stdout(my_pipe2)); bp::child c3("ls", {"-la, "/"}, bp::stdout(my_pipe3)); Where the constructor in `c3` forwards to the constructor in `c1`; `c2` uses the `bp::exec` object returned by `bp::shell`; `c1` uses the `bp::exec` property to set the path and args of the executor.
bp::launch("foo");
Secondly, I'm not sure why shell would be a distrinct function - that doesn make sense, you can use it in all three launch-functions. A shell ist just another process, why would I handly it any different?
This should already be covered, but the shell function would just return an object storing the path and arguments.
Also, is there an advantage to allowing string arguments anywhere in the function call? A single `std::vector<std::string>` argument can be initialized implicitly with a `{...}`. This would easily separate the arguments from the rest of the properties without the lazy binding provided above:
shell(std::string cmd, Props&&...) launch(std::string exe, std::vector<std::string> args, Props&&...)
Which can be invoked like:
bp::shell("ls -la /", bp::stdout(my_pipe)); bp::launch("ls", {"-la", "/"}, bp::stdout(my_pipe)); bp::launch("foo", {}, bp::stdout(my_pipe));
Is requiring a `{}` pair less desirable than the variadic syntax? Yes there is, bp::system("gcc", "main.cpp", "-o", "main.o"); looks awesome. Also keep in mind, that you have the cmd-syntax, which is a single string and that you can path a boost::filesystem path. What you are basically proposing are positional arguments, which might make sense for exe/args or cmd, but not for the test of that. There is no logical order of the other arguments and secondly it is variadic to allow the user to skip some arguments.
Requiring a `{}` hardly seems detrimental to me, but I guess people have different tastes. I hope you at least consider positional arguments for the path and args. For the remainder - I agree the variadic constructor is probably preferable since its behavior can be closer to std::thread.
Also I don't think arguments are any different from the other properties, or at least I don't consider them to be. It's a value that determines how a process behaves that I pass at launch, so why would that be any different from the environment? And yes, args args are optional.
I agree, its just that I do not think `cmd` and `shell` are like the other properties. I also think there is benefit to having a single property for path and args that is taken positionally instead of anywhere in the argument list.
Since you're coming up with so many ideas, I would encourage you to start your own library, which would allow us to talk about some tests way of doing things. Currently you're only Feel free to fork mine or the older version from Boris. That might be a good proof-of-concept.
Lee
I know there is some difficulty because the Posix fork + exec design does not work the same as way as the Windows API. But I think there is some value in specifying some basic concept of an `executor` and its relationship with a `property`. For instance, the executor will have some function (call operator) that starts a new process using internally stored string arguments and returns a handle to the new process. The string arguments should arguably be non-modifiable once the executor is created, etc. The string are only set after the executor is created, I don't know how that would work. Also the flags need to be accumulated through disjunction, I don't see how it would be better to this on construction time, and not in on_setup. It's internal, so why bother with a strange workaround there? What I noticed in the source is that some of the properties require special logic because they are manipulating other properties instead of
Yes the "executor". For example, the `shell` property is manipulating the path and arg properties but nothing else.
Earlier I mentioned on working out a loose way of specifying a `property`. I think such a specification would not consider properties that only manipulate other properties to be a property (since it could do that separately from the `child` constructor). Well that depends on how you model it, I consider the question if a
process is launched through the shell or not to be a property of this; it must be set prior to launching and it affects how it runs. There is no need to model this exactly after the excutor; that's is actually not possible. Also it does not modify a property, but rather modifies an initializer, which is build from several properties. So do I understand you correctly, that you would want this syntax? bp::system(cmd="foo"); bp::system(shell="foo"); If so - why? Why should'n you be able to launch the same process on it's own or through the shell? It just makes more sense to me to enable the shell as an additional feature.
I was asking whether you could store a handle to the `group` in some object whose call operator starts the process (like Niall originally mentioned). So the handle would be present, but potentially invalid depending on how it was implemented. Yes it could, but I stated enought times why this would be a horrible idea. Yes, and I disagreed with your analysis. I recall the primary objection was poor performance - specifically that space for each type of possible
[...] [...] property was needed. This is not true, the binding approach (at least how I saw it), would return a new callable type after each binding. In other words, the binding was conceptually calling `push_back` on a fusion like tuple. Moves + ref-qualifiers meant that dynamic memory would not copied in many cases. So `launcher("foo").group(...).stdout(...)` would return the equivalent of `tuple<launcher, group, stdout>` while moving `std::string` memory as needed.
After thinking about it, all that I (and I think Bjorn and Niall) were trying to accomplish is separating the path and arguments from the remainder of the properties. If the path and arguments were in a single property, `shell` and `cmd` could return that property instead of requiring custom logic internally.
Well that is one reason, though I also assumed, that you wouldn't construct a tuple like that. The primary reason I really dislike the idea, is that every conceivable property would need to be in a member defninition of this tuple, and that is horrible.
`launch` returns a callable that accepts 0 or more `properties` to launch the process. Example:
class launcher { std::string exe_; std::vector<std::string> args_; public: launcher(std::string exe, std::vector<std::string> args) : exe_(std::move(exe)), args_(std::move(args)) { }
template<typename... Props> child operator()(Props&&... props) const { return detail::exec(exe_, args_, std::forward<Props>(props)...); } }; struct launch_ { template<typename... Args> launcher operator()(std::string exe, Args&&... args) const { return {std::move(exe), {std::forward<Args>(args)...}}; } }; constexpr launch_ launch{};
// syntax: // launch("ls", "-la", "/")(stdout(my_pipe)); Yeah, that still doesn't work. When do you launcht process? Please
[...] [...] [...] think those Ideas through, if you want me to take them serious. If you launch at the call of "launch("ls", "-la", "/") than you couldn't bind stdout. If you launch after the binding of stdout, how would you append another paremeter? It's broken. How is it broken? Look at the code snippet, the second function call takes a variadic argument list like the `child` constructor. The string arguments are the only thing "bound" to an object. So appending another property would be done like:
launch("ls, "-la", "/")(stdout(my_pipe1), stdin(my_pipe2));
That style is simply separating string arguments into a separate variadic function call. Or another way of looking at it - there is a single `property` for specifying the path and args that has a fixed positional location. What about the cmd style, i.e. bp::system("gcc --version"); ?
I don't see a good reason for the `child` class to actually launch the process. A global or static function that returns a `child` object should do it instead. This would remove the need for the `shell` property, which would be a distinct function. Well we had a global function for that in previous versions, but that wasn't like that much; especially since people got reminded of std::async. I have to agree, it's not the common C++ idiom to use functions to construct object, you also don't write
std::thread t = std::launch_thread([]{});
Also this would be bad, and probably a common mistake. It would launch the process and immediatly terminate it.
Why would it immediately terminate the process? Or do you mean a non-joined `child` should terminate the process like `std::thread`? Exactly. Also this would produce a deadlock if waiting in ~child and foo
Oh ok, sry, misunderstood that one. But then why is that any better? Just fyi: since you can pass environment variable to the property-list, this doesn't even have to be the same executable. throws. bopstream os; bp::child c("foo", std_in < os); os << foo() << endl; //boom, deadlock.
Admittedly, I did forget about that case - its probably best to follow `std::thread` closely and terminate the current process if not `join`ed or `detach`ed. I don't there is anything wrong with a function returning a `child`, provided the behavior of ignoring the `child` is well documented and defined. However, there is little difference to the code snippet above and:
bp::child c1(bp::exec("ls", "-la", "/"), bp::stdout(my_pipe1)); bp::child c2(bp::shell("ls", "-la", "/"), bp::stdout(my_pipe2)); bp::child c3("ls", {"-la, "/"}, bp::stdout(my_pipe3));
Where the constructor in `c3` forwards to the constructor in `c1`; `c2` uses the `bp::exec` object returned by `bp::shell`; `c1` uses the `bp::exec` property to set the path and args of the executor.
Ok, so I consider exe/args, cmd and shell different properties, but in the end they all get put into one initializer. (https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...) So if anything this could be made public, though at the time the values get put into the exe-args initializrs they are already parsed and formatted. I really don't see the benefit.
Also, is there an advantage to allowing string arguments anywhere in the function call? A single `std::vector<std::string>` argument can be initialized implicitly with a `{...}`. This would easily separate the arguments from the rest of the properties without the lazy binding provided above:
shell(std::string cmd, Props&&...) launch(std::string exe, std::vector<std::string> args, Props&&...)
Which can be invoked like:
bp::shell("ls -la /", bp::stdout(my_pipe)); bp::launch("ls", {"-la", "/"}, bp::stdout(my_pipe)); bp::launch("foo", {}, bp::stdout(my_pipe));
Is requiring a `{}` pair less desirable than the variadic syntax? Yes there is, bp::system("gcc", "main.cpp", "-o", "main.o"); looks awesome. Also keep in mind, that you have the cmd-syntax, which is a single string and that you can path a boost::filesystem path. What you are basically proposing are positional arguments, which might make sense for exe/args or cmd, but not for the test of that. There is no logical order of the other arguments and secondly it is variadic to allow the user to skip some arguments. Requiring a `{}` hardly seems detrimental to me, but I guess people have different tastes. I hope you at least consider positional arguments for the path and args. For the remainder - I agree the variadic constructor is probably preferable since its behavior can be closer to std::thread. The only reason you can't use that is that the parameter list is forwarded internally, hence the type-deduction fails. That syntax is already valid:
bp::system("foo", std::vector<std::string>{"--arg", "bar"}); or auto args = {"--arg", "bar"}; bp::system("foo", args); Though I just checked my code, and that would work for `std::initializer_list<char*>` but not for `std::initializer_list<const char*>`, so I'll add a bugfix and test for the latter.
On Thu, 10 Nov 2016 10:05:00 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
What I noticed in the source is that some of the properties require special logic because they are manipulating other properties instead of the "executor". For example, the `shell` property is manipulating the path and arg properties but nothing else.
Earlier I mentioned on working out a loose way of specifying a `property`. I think such a specification would not consider properties that only manipulate other properties to be a property (since it could do that separately from the `child` constructor). Well that depends on how you model it, I consider the question if a
[...] [...] [...] process is launched through the shell or not to be a property of this; it must be set prior to launching and it affects how it runs. There is no need to model this exactly after the excutor; that's is actually not possible.
Yes, whether a process is being launched in a shell must be set before starting the process, but this is irrelevant. Is `shell` a property of the `child` or a property of the path / arguments pair? In other words, does the internal logic of the `child` constructor actually need to know whether its launching a shell or just some other executable? The implementation would suggest that `child` does not need to know.
Also it does not modify a property, but rather modifies an initializer, which is build from several properties.
So do I understand you correctly, that you would want this syntax?
bp::system(cmd="foo"); bp::system(shell="foo");
If so - why? Why should'n you be able to launch the same process on it's own or through the shell? It just makes more sense to me to enable the shell as an additional feature.
You can launch the process on its own or through the shell with that syntax. Its just that `shell` is modifying the path + args properties given to the `child`.
[...] [...] [...]
Yes, and I disagreed with your analysis. I recall the primary objection was poor performance - specifically that space for each type of possible property was needed. This is not true, the binding approach (at least how I saw it), would return a new callable type after each binding. In other words, the binding was conceptually calling `push_back` on a fusion like tuple. Moves + ref-qualifiers meant that dynamic memory would not copied in many cases. So `launcher("foo").group(...).stdout(...)` would return the equivalent of `tuple<launcher, group, stdout>` while moving `std::string` memory as needed.
After thinking about it, all that I (and I think Bjorn and Niall) were trying to accomplish is separating the path and arguments from the remainder of the properties. If the path and arguments were in a single property, `shell` and `cmd` could return that property instead of requiring custom logic internally. Well that is one reason, though I also assumed, that you wouldn't construct a tuple like that.
The primary reason I really dislike the idea, is that every conceivable property would need to be in a member defninition of this tuple, and that is horrible.
The current implementation isn't without boilerplate - traits and processing hooks for fusion::for_each are needed for the properties too. Although I do agree that launching the process inside of the `child` constructor is preferable since the destructor should terminate if not detached or joined. However, I think some of the properties need some work.
How is it broken? Look at the code snippet, the second function call takes a variadic argument list like the `child` constructor. The string arguments are the only thing "bound" to an object. So appending another property would be done like:
launch("ls, "-la", "/")(stdout(my_pipe1), stdin(my_pipe2)); Oh ok, sry, misunderstood that one. But then why is that any better? Just fyi: since you can pass environment variable to the
[...] [...] [...] [...] property-list, this doesn't even have to be the same executable.
That style is simply separating string arguments into a separate variadic function call. Or another way of looking at it - there is a single `property` for specifying the path and args that has a fixed positional location. What about the cmd style, i.e. bp::system("gcc --version"); ?
[...] [...]
Why would it immediately terminate the process? Or do you mean a non-joined `child` should terminate the process like `std::thread`? Exactly. Also this would produce a deadlock if waiting in ~child and foo throws.
bopstream os; bp::child c("foo", std_in < os);
os << foo() << endl; //boom, deadlock.
Admittedly, I did forget about that case - its probably best to follow `std::thread` closely and terminate the current process if not `join`ed or `detach`ed. I don't there is anything wrong with a function returning a `child`, provided the behavior of ignoring the `child` is well documented and defined. However, there is little difference to the code snippet above and:
bp::child c1(bp::exec("ls", "-la", "/"), bp::stdout(my_pipe1)); bp::child c2(bp::shell("ls", "-la", "/"), bp::stdout(my_pipe2)); bp::child c3("ls", {"-la, "/"}, bp::stdout(my_pipe3));
Where the constructor in `c3` forwards to the constructor in `c1`; `c2` uses the `bp::exec` object returned by `bp::shell`; `c1` uses the `bp::exec` property to set the path and args of the executor.
Ok, so I consider exe/args, cmd and shell different properties, but in the end they all get put into one initializer. (https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...) So if anything this could be made public, though at the time the values get put into the exe-args initializrs they are already parsed and formatted. I really don't see the benefit.
The benefit is it removes the special internal logic for a few specific properties and makes what the code is _actually_ doing more clear. I've also been pushing for a more formal specification of `properties` because some of the IO variants are doing too much in `child`. The constructor for `std::thread` does not throw once the thread is created, but what about some of the IO properties for `process::child`? Several of the `on_exec`/`on_success` hooks are doing various calls that can throw - does `child` block internally in the constructor until the other process completes? Does it detach? Terminate? And why is so much being done inside of the `child` constructor when the `child` should only need to know the pipe handles for communication. I'm mainly thinking of the code for piping to future or mutable buffer.
[...] [...]
Requiring a `{}` hardly seems detrimental to me, but I guess people have different tastes. I hope you at least consider positional arguments for the path and args. For the remainder - I agree the variadic constructor is probably preferable since its behavior can be closer to std::thread. The only reason you can't use that is that the parameter list is forwarded internally, hence the type-deduction fails. That syntax is already valid:
Yes I know that the initializer list deduction would fail. Which is why I suggested the process arguments be positional.
bp::system("foo", std::vector<std::string>{"--arg", "bar"});
or
auto args = {"--arg", "bar"}; bp::system("foo", args);
Though I just checked my code, and that would work for `std::initializer_list<char*>` but not for `std::initializer_list<const char*>`, so I'll add a bugfix and test for the latter.
Lee
Am 12.11.2016 um 13:35 schrieb Lee Clagett:
On Thu, 10 Nov 2016 10:05:00 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
What I noticed in the source is that some of the properties require special logic because they are manipulating other properties instead of the "executor". For example, the `shell` property is manipulating the path and arg properties but nothing else.
Earlier I mentioned on working out a loose way of specifying a `property`. I think such a specification would not consider properties that only manipulate other properties to be a property (since it could do that separately from the `child` constructor). Well that depends on how you model it, I consider the question if a
[...] [...] [...] process is launched through the shell or not to be a property of this; it must be set prior to launching and it affects how it runs. There is no need to model this exactly after the excutor; that's is actually not possible. Yes, whether a process is being launched in a shell must be set before starting the process, but this is irrelevant. Is `shell` a property of the `child` or a property of the path / arguments pair? In other words, does the internal logic of the `child` constructor actually need to know whether its launching a shell or just some other executable? The implementation would suggest that `child` does not need to know. Well the child doesn't know of any of the properties you pass it...
Also it does not modify a property, but rather modifies an initializer, which is build from several properties.
So do I understand you correctly, that you would want this syntax?
bp::system(cmd="foo"); bp::system(shell="foo");
If so - why? Why should'n you be able to launch the same process on it's own or through the shell? It just makes more sense to me to enable the shell as an additional feature. You can launch the process on its own or through the shell with that syntax. Its just that `shell` is modifying the path + args properties given to the `child`. I know, that's why there's shell, like this:
bp::system(cmd="foo", bp::shell); bp::system(exe="../foo", "--bar", bp::shell); So obivously the question isn't why is this useful, but why is your syntax better?
Admittedly, I did forget about that case - its probably best to follow `std::thread` closely and terminate the current process if not `join`ed or `detach`ed. I don't there is anything wrong with a function returning a `child`, provided the behavior of ignoring the `child` is well documented and defined. However, there is little difference to the code snippet above and:
bp::child c1(bp::exec("ls", "-la", "/"), bp::stdout(my_pipe1)); bp::child c2(bp::shell("ls", "-la", "/"), bp::stdout(my_pipe2)); bp::child c3("ls", {"-la, "/"}, bp::stdout(my_pipe3));
Where the constructor in `c3` forwards to the constructor in `c1`; `c2` uses the `bp::exec` object returned by `bp::shell`; `c1` uses the `bp::exec` property to set the path and args of the executor.
Ok, so I consider exe/args, cmd and shell different properties, but in the end they all get put into one initializer. (https://github.com/klemens-morgenstern/boost-process/blob/develop/include/bo...) So if anything this could be made public, though at the time the values get put into the exe-args initializrs they are already parsed and formatted. I really don't see the benefit. The benefit is it removes the special internal logic for a few specific properties and makes what the code is _actually_ doing more clear. I've also been pushing for a more formal specification of `properties` because some of the IO variants are doing too much in `child`. The constructor for `std::thread` does not throw once the thread is created, but what about some of the IO properties for `process::child`? Several of the `on_exec`/`on_success` hooks are doing various calls that can throw - does `child` block internally in the constructor until the other process completes? Does it detach? Terminate? And why is so much being done inside of the `child` constructor when the `child` should only need to know the pipe handles for communication. I'm mainly thinking of the code for piping to future or mutable buffer. The child-class does not know of anything there, the child(...) ctor actually just forwards to values to a launch function. It's just a form of RAII, since I think that makes sense. It's equivalent to boost::thread([](int), 42);
If there's an error before the launch it will not launch, if afterwards it will terminate the child immediatly.
On Sat, 12 Nov 2016 14:22:24 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 12.11.2016 um 13:35 schrieb Lee Clagett:
On Thu, 10 Nov 2016 10:05:00 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
[...] [...] [...]
Yes, whether a process is being launched in a shell must be set before starting the process, but this is irrelevant. Is `shell` a property of the `child` or a property of the path / arguments pair? In other words, does the internal logic of the `child` constructor actually need to know whether its launching a shell or just some other executable? The implementation would suggest that `child` does not need to know. Well the child doesn't know of any of the properties you pass it...
[...]
You can launch the process on its own or through the shell with that syntax. Its just that `shell` is modifying the path + args properties given to the `child`. I know, that's why there's shell, like this:
bp::system(cmd="foo", bp::shell); bp::system(exe="../foo", "--bar", bp::shell);
This same usage/syntax in `process::system` is irrelevant.
So obivously the question isn't why is this useful, but why is your syntax better?
I think the syntax I proposed is a better model of what is actually happening. I do not think there are any aesthetical gains to my proposal for `process::shell`, I think its a "push" in that regard.
The benefit is it removes the special internal logic for a few specific properties and makes what the code is _actually_ doing more clear. I've also been pushing for a more formal specification of `properties` because some of the IO variants are doing too much in `child`. The constructor for `std::thread` does not throw once the thread is created, but what about some of the IO properties for `process::child`? Several of the `on_exec`/`on_success` hooks are doing various calls that can throw - does `child` block internally in the constructor until the other process completes? Does it detach? Terminate? And why is so much being done inside of the `child` constructor when the `child` should only need to know the pipe handles for communication. I'm mainly thinking of the code for piping to future or mutable buffer. The child-class does not know of anything there, the child(...) ctor actually just forwards to values to a launch function. It's just a
[...] [...] form of RAII, since I think that makes sense. It's equivalent to boost::thread([](int), 42);
I do not think this is equivalent to `std::thread`. The constructor for `std::thread` takes the bare minimum to start a new processing thread, while the constructor for `process::child` accepts parameters that are unnecessary - `std::future` for instance. I think its worth determining the minimum set of values needed before process launching, and restricting the constructor to accept those values. If `stdout` of a process needs to go to `std::future<std::string>`, this does not need to be indicated before process launching, only a designated pipe needs to be specified, etc.
If there's an error before the launch it will not launch, if afterwards it will terminate the child immediatly.
Lee
`launch` returns a callable that accepts 0 or more `properties` to launch the process. Example:
class launcher { std::string exe_; std::vector<std::string> args_; public: launcher(std::string exe, std::vector<std::string> args) : exe_(std::move(exe)), args_(std::move(args)) { }
template<typename... Props> child operator()(Props&&... props) const { return detail::exec(exe_, args_, std::forward<Props>(props)...); } }; struct launch_ { template<typename... Args> launcher operator()(std::string exe, Args&&... args) const { return {std::move(exe), {std::forward<Args>(args)...}}; } }; constexpr launch_ launch{};
// syntax: // launch("ls", "-la", "/")(stdout(my_pipe)); Yeah, that still doesn't work. When do you launcht process? Please
[...] [...] [...] think those Ideas through, if you want me to take them serious. If you launch at the call of "launch("ls", "-la", "/") than you couldn't bind stdout. If you launch after the binding of stdout, how would you append another paremeter? It's broken.
I forgot: you can write all of this very easily: struct launch { std::string exe; std::vector<std::string> args; launch(const std::string & exe, const std::vector<std::string> &args) : exe(exe), args(args) {} template<typename ... Args> bp::child operator()(Args && ... args) { return bp::child(bp::exe=exe, bp::args=args, std::forward<Args>(args)...); } };
bp::shell("ls -la /", bp::stdout(my_pipe)); bp::launch("ls", {"-la", "/"}, bp::stdout(my_pipe)); bp::launch("foo", {}, bp::stdout(my_pipe));
Same here: template<typename ... Args> void shell(const std::string & cmd, Args &&...args) { bp::spawn(bp::cmd=cmd, bp::shell, std::forward<Args>(args)...); } template<typename ... Args> void launch(const std::string & exe, const std::vector<std::string> & args, Args && .. args) { bp::spawn(bp::exe=exe, bp::ars=args, std::forward<Args>(args)...); }
On 11/07/2016 01:46 AM, Klemens Morgenstern wrote:
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process.
"Horrific mess" is in the eye of the beholder. With the current API it is unclear which parameters are mutually exclusive (e.g. can I pass both a yield context and an error_code?) or what happens if I specify the same attribute multiple times like this: bp::child c(command, bp::std_out > p, bp::std_out > "output.txt"); and of course the argument overwrite issue mentioned by Lee, all of which demonstrates that the current API is error-prone. The discussion about attributes is a bit fuzzy because the library does not define what they are. After a closer look it appears that there are several categories of attributes crammed into the initializer list. One such category is the status-reporting (e.g. error_code and yield context.) Here is another idea for an API: have separate functions for synchronous and asynchronous invocation. Let the synchronous invocation accept one command parameter, one optional attributes parameter, zero or more arguments, and one optional error_code. The latter determine whether the call throws or not. I am deliberately omitting the attributes parameter below because it is irrelevant to the sync/async split. bp::system(command, arguments...); // throws bp::system(command, arguments..., error_code&) nothrow; Let the asynchronous invocation accept one io_service parameter, one command parameter, one optional attributes parameter, zero or more arguments, and one handler/extension parameter (i.e. no error_code parameter.) bp::async_system(io, command, arguments..., [](error_code){}); bp::async_system(io, command, arguments..., yield); This solves the error_code versus yield context issue, and it further more enables more capable extensions (see my next mail) such as for Boost.Fiber.
Am 13.11.2016 um 13:23 schrieb Bjorn Reese:
On 11/07/2016 01:46 AM, Klemens Morgenstern wrote:
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process.
"Horrific mess" is in the eye of the beholder. I'm talking about the implementation, not of the usage. Please prove me wrong, but with an implementation not an usage-example. If you find a solution which can be integrated into boost.process, I'll add it to the library.
With the current API it is unclear which parameters are mutually exclusive (e.g. can I pass both a yield context and an error_code?) or what happens if I specify the same attribute multiple times like this:
bp::child c(command, bp::std_out > p, bp::std_out > "output.txt");
Well this will use the second pipe, though I grant you, that's not the best solution here - an error would be better. I could add an assertion, so there are no duplicates of that. On the other hand it is similar to that: child_builder cb; cb.std_out(p); cb.std_out("output.txt"); So it's not that strange.
and of course the argument overwrite issue mentioned by Lee, all of which demonstrates that the current API is error-prone.
The discussion about attributes is a bit fuzzy because the library does not define what they are. After a closer look it appears that there are several categories of attributes crammed into the initializer list.
Well yes, they are not called attributes, but properties. The reason is, that I don't see a different between the arguments passed and all the other stuff that's passed. After all, at least of those properties, you'd call attributes, can in some cases even select a different executable (i.e. the environment variables).
One such category is the status-reporting (e.g. error_code and yield context.) Here is another idea for an API: have separate functions for synchronous and asynchronous invocation. That's not the same. std::error_code is used to avoid exceptions and is only used when the process is launched. On exit you don't get an error_code, but only the exit-code of the process; and another std::error_code in case something went wrong with waiting. So there's no std::error_code vs. yield_context, they do different things and it actually makes sense to use them together. Now in case you use your error_code with system, like that
bp::system("foo", ec, yield); ec being set will indicate that the process was not launched successfully and hence the coroutine wasn't suspended. Also, you do not need to pass an io_service when passing a yield_context, since the system function can get the io_service from the yield_context. The io_service is needed bp::on_exit and the convenience stuff for asynchronous read, e.g. std_out > buffer(thingy). Additionally you do not need to pass an io_service for async-operations to bp::system; if none is passed but a property requires that, system will internally create one and use that.
Let the synchronous invocation accept one command parameter, one optional attributes parameter, zero or more arguments, and one optional error_code. The latter determine whether the call throws or not. I am deliberately omitting the attributes parameter below because it is irrelevant to the sync/async split.
bp::system(command, arguments...); // throws bp::system(command, arguments..., error_code&) nothrow;
Let the asynchronous invocation accept one io_service parameter, one command parameter, one optional attributes parameter, zero or more arguments, and one handler/extension parameter (i.e. no error_code parameter.)
bp::async_system(io, command, arguments..., [](error_code){}); bp::async_system(io, command, arguments..., yield); As stated above, both do not need to get an io-service passed, so I don't see the point of the second one. The first can already be done, if you want that, in the followin way:
bp::child(ios, exe, args..., on_exit=[](int code, const std::error_code &ec){}).detach(); I wouldn't recomment that, but that's possible. And no, bp::spawn doesn't work.
On 11/13/2016 8:54 AM, Klemens Morgenstern wrote:
Am 13.11.2016 um 13:23 schrieb Bjorn Reese:
On 11/07/2016 01:46 AM, Klemens Morgenstern wrote:
I didn't understand it that way, especially since he Bjorn mentioned boost::thread. I'm a bit tired of this discussion, and Boris explained very well, why the initializers-approach was chosen. As far as I understand it he considers everything but args as attributes and wants them all crammed into one type (or something like that) which would be a horrific mess. I should know, I tried to build somethinke like that (based on boost.process), just two ours ago - it doesn't work for a proper process library, there are just too many properties to a process.
"Horrific mess" is in the eye of the beholder. I'm talking about the implementation, not of the usage. Please prove me wrong, but with an implementation not an usage-example. If you find a solution which can be integrated into boost.process, I'll add it to the library.
With the current API it is unclear which parameters are mutually exclusive (e.g. can I pass both a yield context and an error_code?) or what happens if I specify the same attribute multiple times like this:
bp::child c(command, bp::std_out > p, bp::std_out > "output.txt");
Well this will use the second pipe, though I grant you, that's not the best solution here - an error would be better.
Whatever you decide it is important to document.
I could add an assertion, so there are no duplicates of that. On the other hand it is similar to that:
child_builder cb; cb.std_out(p); cb.std_out("output.txt");
So it's not that strange. snipped...
On 11/13/2016 02:54 PM, Klemens Morgenstern wrote:
Well this will use the second pipe, though I grant you, that's not the best solution here - an error would be better. I could add an assertion, so there are no duplicates of that. On the other hand it is similar to that:
child_builder cb; cb.std_out(p); cb.std_out("output.txt");
This can be turned into a compiler error. See the sqlpp11 for inspiration.
That's not the same. std::error_code is used to avoid exceptions and is only used when the process is launched. On exit you don't get an error_code, but only the exit-code of the process; and another
The exit code is the "return value" (quoted because it is actually "returned" as an input parameter to the handler) and thus irrelevant to the discussion about error_code versus yield context.
std::error_code in case something went wrong with waiting. So there's no std::error_code vs. yield_context, they do different things and it actually makes sense to use them together. Now in case you use your error_code with system, like that
bp::system("foo", ec, yield);
ec being set will indicate that the process was not launched successfully and hence the coroutine wasn't suspended.
That is not how (asio) asynchronous operations work. Errors must be reported via the handler, which in this case is the yield context. Another important requirement is that the handler is not called directly from the initiating function. See: http://www.boost.org/doc/html/boost_asio/reference/asynchronous_operations.h... The error_code is reported via the yield context as "yield[ec]". For yield context it may not seem like much of a difference (besides the fact that with your approach we do not yield in the error case, and therefore may starve other competing yield context) but for other async_result constructs the difference is significant.
Am 20.11.2016 um 12:48 schrieb Bjorn Reese:
On 11/13/2016 02:54 PM, Klemens Morgenstern wrote:
Well this will use the second pipe, though I grant you, that's not the best solution here - an error would be better. I could add an assertion, so there are no duplicates of that. On the other hand it is similar to that:
child_builder cb; cb.std_out(p); cb.std_out("output.txt");
This can be turned into a compiler error. See the sqlpp11 for inspiration.
How? How can I turn a sequence of calls valid when the object stays the same and each call is valid?
std::error_code in case something went wrong with waiting. So there's no std::error_code vs. yield_context, they do different things and it actually makes sense to use them together. Now in case you use your error_code with system, like that
bp::system("foo", ec, yield);
ec being set will indicate that the process was not launched successfully and hence the coroutine wasn't suspended.
That is not how (asio) asynchronous operations work. Errors must be reported via the handler, which in this case is the yield context. Another important requirement is that the handler is not called directly from the initiating function. See:
http://www.boost.org/doc/html/boost_asio/reference/asynchronous_operations.h...
The error_code is reported via the yield context as "yield[ec]". For yield context it may not seem like much of a difference (besides the fact that with your approach we do not yield in the error case, and therefore may starve other competing yield context) but for other async_result constructs the difference is significant.
I'm sorry, but I think you still do not understand how that works. Two things can hapen: 1. The process launches successfully --> No error in ec, the coroutine is suspended the wait-handle might fail, which is not captured (but could be with yield[ec_yield]) 2. The process does not launch --> ec is set, the couroutine is NOT suspended and nor error can occur since the code does not wait for the handle (obviously, since there is none) so yield does nothing. system(yield) does not launch, but execute asynchronously, i.e. it suspends after successful launch.
On 11/20/2016 01:38 PM, Klemens Morgenstern wrote:
How? How can I turn a sequence of calls valid when the object stays the same and each call is valid?
Look at the sqlpp11 codebase.
I'm sorry, but I think you still do not understand how that works.
Oh I understand it perfectly fine. I am pointing out that your yield integration does not fulfill the requirements for asynchronous operations. Look at the Asio references implementation and tell me if you can find any async_ functions that take an error_code output parameter.
Two things can hapen:
1. The process launches successfully --> No error in ec, the coroutine is suspended the wait-handle might fail, which is not captured (but could be with yield[ec_yield]) 2. The process does not launch --> ec is set, the couroutine is NOT suspended and nor error can occur since the code does not wait for the handle (obviously, since there is none) so yield does nothing.
Case 2 is where you deviate from the Asio model. Normally we post a function on the io_service that calls the handler with the error to fullfil the requirement that all errors are reported via the handler.
Am 20.11.2016 um 15:02 schrieb Bjorn Reese:
On 11/20/2016 01:38 PM, Klemens Morgenstern wrote:
How? How can I turn a sequence of calls valid when the object stays the same and each call is valid?
Look at the sqlpp11 codebase.
Look, I can see (as in sqlpp11) that this won't work, and yield a compiler error foo f; f.bar().bar(); but not the one that was in the prior E-Mail: foo f; f.bar(); f.bar(); Could you please explain how that works, since you claimed it does? We were talking about a specifc example, so please explain it to me.
I'm sorry, but I think you still do not understand how that works.
Oh I understand it perfectly fine. I am pointing out that your yield integration does not fulfill the requirements for asynchronous operations. Look at the Asio references implementation and tell me if you can find any async_ functions that take an error_code output parameter.
No, but who - except you - cares?
Two things can hapen:
1. The process launches successfully --> No error in ec, the coroutine is suspended the wait-handle might fail, which is not captured (but could be with yield[ec_yield]) 2. The process does not launch --> ec is set, the couroutine is NOT suspended and nor error can occur since the code does not wait for the handle (obviously, since there is none) so yield does nothing.
Case 2 is where you deviate from the Asio model.
Normally we post a function on the io_service that calls the handler with the error to fullfil the requirement that all errors are reported via the handler.
It is not a regular async-function and there is only one async-operation - so why would I be forced to do that? It's a feature, that allows you to use exactly one thing, the yield_context, with boost.asio. So it's not a general anync-operation. If I implement the error reporting with yield[ec] I'll break my interface style. And that would destroy part of my error-handling.
On 11/20/2016 05:04 PM, Klemens Morgenstern wrote:
No, but who - except you - cares?
I am trying to help you with a more generic API for asynchronous operations. If you support asio::async_result, then you get all sorts of integration, including with coroutines and fibers, for free. When Nat asked about fibers, you responded that you were open to suggestions, so I would have thought that you cared too.
On 11/20/2016 05:04 PM, Klemens Morgenstern wrote:
No, but who - except you - cares?
I am trying to help you with a more generic API for asynchronous operations. If you support asio::async_result, then you get all sorts of integration, including with coroutines and fibers, for free. When Nat asked about fibers, you responded that you were open to suggestions, so I would have thought that you cared too. I am, but the problem is a bit more complex than that. I see three
Am 20.11.2016 um 17:52 schrieb Bjorn Reese: possibilities for handling the exit code: future<int> handler(int, error_code) yield_context. The first two are possible with child() and the latter with system. The preferred way for me is to use child, so you have a handle. So the exception here is yield_context because using that with system seems intuitive to me. I don't see how I could use this with child, because you'd have to suspend the coroutine in on_success, which will lead to possible errors if you put something behind it in the argument-list that needs the on_success call. Therefore, it's restrictet to bp::system. That's the main reason, so it's an exception, it does not try to be a correct asio-interface.
On 20-11-16 17:52, Bjorn Reese wrote:
If you support asio::async_result, then you get all sorts of integration, including with coroutines and fibers, for free.
I'm very curious about this, because I'm currently designing a good async framework for our backend software. Me too I've been trying to get the most flexible implementation with the least amount of code. Is there a pointer to a bit of documentation that shows how `async_result` gets coroutine support "for free" - or perhaps an existing example if the docs don't exist? Cheers, Seth
On 21/11/2016 07:14, Seth wrote:
On 20-11-16 17:52, Bjorn Reese wrote:
If you support asio::async_result, then you get all sorts of integration, including with coroutines and fibers, for free.
I'm very curious about this, because I'm currently designing a good async framework for our backend software. Me too I've been trying to get the most flexible implementation with the least amount of code.
Is there a pointer to a bit of documentation that shows how `async_result` gets coroutine support "for free" - or perhaps an existing example if the docs don't exist?
+1. I tried this once in the past (with an earlier version of Boost) but documentation on how to create from-scratch async operations was very poor. It also required way too much boilerplate, from what I could see. (And relies on using detail classes, which is usually a no-no for external code.) I haven't really been following the standards track but I'm hopeful that the network/async/whatever TS ends up better documented. :)
Am 20.11.2016 um 17:52 schrieb Bjorn Reese:
On 11/20/2016 05:04 PM, Klemens Morgenstern wrote:
No, but who - except you - cares?
I am trying to help you with a more generic API for asynchronous operations. If you support asio::async_result, then you get all sorts of integration, including with coroutines and fibers, for free. When Nat asked about fibers, you responded that you were open to suggestions, so I would have thought that you cared too.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
So, after further consideration, i.e. writing an example, I decided to add `async_system` and remove system(yield_context) in favor of it. Now integrating async_result into system, would've been pure horror, which is why I rejected that, but that actually made my yield_context implementation look unecessary complicated. So you will have a function `async_system(io_service& ios, Exithandler && handler, Args...args);` where args are the usual properties and the return value will be obtained from async_result. That seems to be the better approach in general. I'll have it documented sometime next week.
On Sun, 6 Nov 2016 09:30:18 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 06.11.2016 um 01:39 schrieb Lee Clagett:
On Sat, 5 Nov 2016 17:15:58 +0100 Bjorn Reese <breese@mail1.stofanet.dk> wrote:
I'm glad you brought this up - I think Niall did as well - if you look at the constructor documentation for `process::child` it is very bare. And I am not sure how to easily describe how to use it. I think maintainability is going to be difficult without a more strict contract of expected arguments and behavior. That is because it is one of the three launch functions, but their are actually quite clearly defined. You either pass a known type that will be intepreted as some property (i.e. std::string->args) or you
[...] [...] [...] pass a property directly, i.e. args+="Value". That is quite clear, but it is extensible, hence the list of actually allowed types are written in the reference at the properties, not in a central list.
Is the following clear: bp::child("foo", "arg1" bp::stdout < bp::null, bp::args="arg2"); Does the last constructor argument clear all `argument properties` used by the child? Would this try to invoke process "arg2"?
@Bjorn: That's the syntax you would get if you used rvalue qualifications, as you proposed. [...] [...] [...]
Again, I think Niall proposed something similar (callable). Also, I do not think partial evaluation support is necessary in boost::process - `boost::fit` (which I hope/think will be a boost lib) could partially bind command arguments or even std::bind. The syntax would be different, but once its a callable its much more flexible:
auto with_jar = std::bind(child("java"), "-jar", "antlr4.jar", _1);
Now `with_jar` requires a single string argument to be invoked, and invoking it executes the process and returns a handle to that process. And and why is that better then the current way of doing things?
auto antlr = [](const std::string & g4) {return child("java", "-jar", "antlr4.jar", g4);}; //or add an redirection: auto antlr_p = [](const std::string & g4, pipe &p){return child("java", "-jar", "antlr4.jar", g4, std_out > p, std_err > null);};
I was responding to Bjorn who suggested some kind of argument builder/binder. I didn't think it was necessary based on the rest of his proposed syntax. Lee
Am 07.11.2016 um 13:43 schrieb Lee Clagett:
On Sun, 6 Nov 2016 09:30:18 +0100 Klemens Morgenstern <klemens.morgenstern@gmx.net> wrote:
Am 06.11.2016 um 01:39 schrieb Lee Clagett:
On Sat, 5 Nov 2016 17:15:58 +0100 Bjorn Reese <breese@mail1.stofanet.dk> wrote:
I'm glad you brought this up - I think Niall did as well - if you look at the constructor documentation for `process::child` it is very bare. And I am not sure how to easily describe how to use it. I think maintainability is going to be difficult without a more strict contract of expected arguments and behavior. That is because it is one of the three launch functions, but their are actually quite clearly defined. You either pass a known type that will be intepreted as some property (i.e. std::string->args) or you
[...] [...] [...] pass a property directly, i.e. args+="Value". That is quite clear, but it is extensible, hence the list of actually allowed types are written in the reference at the properties, not in a central list. Is the following clear:
bp::child("foo", "arg1" bp::stdout < bp::null, bp::args="arg2");
Does the last constructor argument clear all `argument properties` used by the child? Would this try to invoke process "arg2"?
This would not compile for other reasons. But if you put a bp::args=... somewhere it overridesprior arg-setters; that's why you'd use bp::args+=.... But that only overrides args, not exe, so if you write "bp::child("foo", "arg1", bp::args="arg2") it would call "foo arg2".
On 10/30/2016 04:39 PM, Klemens Morgenstern wrote: >> Is it possible to redirect stderr to stdout? (like 2>&1). The reference >> documentation only mentions this under asynchronous pipe output. > If you mean to redirect the stdout to the stderr of the current process > - yes it is. This is what the FILE* overload is for, you can just write > std_out > stderr. I know it's a bit confusing, but std::cerr cannot be > used, because (1) it doesn not give access to the and secondly you can > change the rdbuf, redirecting std::cerr that way. I am asking about something like this: child(command, std_out > pipe, std_err > std_out); which corresponds to the shell redirection 2>&1.
Am 05.11.2016 um 13:32 schrieb Bjorn Reese:
> On 10/30/2016 04:39 PM, Klemens Morgenstern wrote:
>
>>> Is it possible to redirect stderr to stdout? (like 2>&1). The reference
>>> documentation only mentions this under asynchronous pipe output.
>> If you mean to redirect the stdout to the stderr of the current process
>> - yes it is. This is what the FILE* overload is for, you can just write
>> std_out > stderr. I know it's a bit confusing, but std::cerr cannot be
>> used, because (1) it doesn not give access to the and secondly you can
>> change the rdbuf, redirecting std::cerr that way.
>
> I am asking about something like this:
>
> child(command, std_out > pipe, std_err > std_out);
>
> which corresponds to the shell redirection 2>&1.
>
What you can do is similar to the console syntax:
child c("foo", (std_out & std_err) > pipe);
which is the same as 2>&1.
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
I would like to merge a bunch of serialization library fixes to the master which is going to be 1.63. I hadn't done as I wasn't ready and it seemed that the release of 1.63 was imminent. But I haven't heard anything. So I'm wondering if things have gotten hung up and maybe I could merge. Assuming I can't do this, does anyone have any idea when the master would be open for merges? Robert Ramey
On November 9, 2016 1:53:36 AM Robert Ramey <ramey@rrsd.com> wrote:
I would like to merge a bunch of serialization library fixes to the master which is going to be 1.63.
I hadn't done as I wasn't ready and it seemed that the release of 1.63 was imminent. But I haven't heard anything. So I'm wondering if things have gotten hung up and maybe I could merge.
Assuming I can't do this, does anyone have any idea when the master would be open for merges?
AFAIK, the release schedule on the website is actual.
On Tue, Nov 8, 2016 at 6:28 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On November 9, 2016 1:53:36 AM Robert Ramey <ramey@rrsd.com> wrote:
I would like to merge a bunch of serialization library fixes to the
master which is going to be 1.63.
I hadn't done as I wasn't ready and it seemed that the release of 1.63 was imminent. But I haven't heard anything. So I'm wondering if things have gotten hung up and maybe I could merge.
Assuming I can't do this, does anyone have any idea when the master would be open for merges?
AFAIK, the release schedule on the website is actual.
Yes. The calendar on the website is currently accurate. -- -- Rene Rivera -- Grafik - Don't Assume Anything -- Robot Dreams - http://robot-dreams.net -- rrivera/acm.org (msn) - grafikrobot/aim,yahoo,skype,efnet,gmail
On 11/8/16 3:57 PM, Rene Rivera wrote:
On Tue, Nov 8, 2016 at 6:28 PM, Andrey Semashev <andrey.semashev@gmail.com> wrote:
On November 9, 2016 1:53:36 AM Robert Ramey <ramey@rrsd.com> wrote:
I would like to merge a bunch of serialization library fixes to the
master which is going to be 1.63.
I hadn't done as I wasn't ready and it seemed that the release of 1.63 was imminent. But I haven't heard anything. So I'm wondering if things have gotten hung up and maybe I could merge.
Assuming I can't do this, does anyone have any idea when the master would be open for merges?
AFAIK, the release schedule on the website is actual.
Yes. The calendar on the website is currently accurate.
OK - for some reason I thought that things were behind. I notice that things are not closed. I would like to merge develop in to master for the serialization library. The test matrix looks as good as it's going to look in the near future - I don't know what else to fix. So I'm asking for permission here. Robert Ramey
On Tue, Nov 8, 2016 at 10:49 PM, Robert Ramey <ramey@rrsd.com> wrote:
On 11/8/16 3:57 PM, Rene Rivera wrote:
On Tue, Nov 8, 2016 at 6:28 PM, Andrey Semashev < andrey.semashev@gmail.com> wrote:
On November 9, 2016 1:53:36 AM Robert Ramey <ramey@rrsd.com> wrote:
I would like to merge a bunch of serialization library fixes to the
master which is going to be 1.63.
I hadn't done as I wasn't ready and it seemed that the release of 1.63 was imminent. But I haven't heard anything. So I'm wondering if things have gotten hung up and maybe I could merge.
Assuming I can't do this, does anyone have any idea when the master would be open for merges?
AFAIK, the release schedule on the website is actual.
Yes. The calendar on the website is currently accurate.
OK - for some reason I thought that things were behind.
I notice that things are not closed. I would like to merge develop in to master for the serialization library. The test matrix looks as good as it's going to look in the near future - I don't know what else to fix. So I'm asking for permission here.
You don't need permission for "not major changes". It's up to you to decide if your changes are "not major". -- -- Rene Rivera -- Grafik - Don't Assume Anything -- Robot Dreams - http://robot-dreams.net -- rrivera/acm.org (msn) - grafikrobot/aim,yahoo,skype,efnet,gmail
participants (11)
-
Andrey Semashev -
Bjorn Reese -
Edward Diener -
Gavin Lambert -
Klemens Morgenstern -
Lee Clagett -
Niall Douglas -
Paul A. Bristow -
Rene Rivera -
Robert Ramey -
Seth