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

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Lee Clagett (forum_at_[hidden])
Date: 2016-11-09 08:02:32

On Tue, 8 Nov 2016 21:38:19 +0100
Klemens Morgenstern <klemens.morgenstern_at_[hidden]> wrote:

> 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:
> >  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
>  [...]  
> > 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/boost/process/detail/windows/executor.hpp#L235
> 
> 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