Re: [boost] [process] Formal Review starts today, 27 October

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-11-08 15:38:19

Am 08.11.2016 um 19:57 schrieb Lee Clagett:
> On Mon, 7 Nov 2016 14:26:11 +0100
> Klemens Morgenstern <klemens.morgenstern_at_[hidden]> wrote:
>
>> Am 07.11.2016 um 13:44 schrieb Lee Clagett:
>>> On Mon, 7 Nov 2016 01:46:42 +0100
>>> Klemens Morgenstern <klemens.morgenstern_at_[hidden]> 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/boost/process/detail/windows/executor.hpp#L235

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?