> On Aug. 6, 2015, 5:48 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 44
> > <https://reviews.apache.org/r/36978/diff/2/?file=1032323#file1032323line44>
> >
> >     s/cmd/command/

Reverted.


> On Aug. 6, 2015, 5:48 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 45
> > <https://reviews.apache.org/r/36978/diff/2/?file=1032323#file1032323line45>
> >
> >     s/cmd/command/
> >     s/empyt/empty/

Reverted.


> On Aug. 6, 2015, 5:48 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, line 
> > 31
> > <https://reviews.apache.org/r/36978/diff/2/?file=1032324#file1032324line31>
> >
> >     s/cmd/command/ (matches the declaration).

reverted.


> On Aug. 6, 2015, 5:48 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, lines 
> > 55-57
> > <https://reviews.apache.org/r/36978/diff/2/?file=1032323#file1032323line55>
> >
> >     A couple of comments:
> >     
> >     (1) This is a good place for a 'std::initializer_list' to clean up the 
> > call sites, i.e.,
> >     
> >     os::shell("echo", {"hello", "world"});
> >     
> >     versus:
> >     
> >     os::shell("echo", vector<string>{"hello", "world"});
> >     
> >     
> >     (2) I'm a little confounded by the 'std::vector' approach (even after 
> > replacing with 'std::initializer_list'). In particular, it seems like the 
> > value it adds is that it keeps a programmer from having to do 
> > 'strings::join(" ", args)' but they still have to stringify their arguments 
> > which makes me wonder why you wouldn't just use 'strings::format' outside 
> > of the call everywhere? Or why not keep the original "printf" version and 
> > call 'strings::format' internally instead of 'strings::join'? With the 
> > 'std::initializer_list' approach I'll imagine we'll see a mix of:
> >     
> >     (A) os::shell("echo", {"hello", stringify(arg)});
> >     
> >     or:
> >     
> >     (B) os::shell("echo hello " + stringify(arg));
> >     
> >     or:
> >     
> >     (C) os::shell(strings::join(" ", "echo", "hello", stringify(arg)));
> >     
> >     or:
> >     
> >     (D) os::shell(strings::format("echo hello %s", arg));
> >     
> >     The existing version (or an improved variadic template version) would 
> > have looked something like:
> >     
> >     (E) os::shell("echo hello %s", arg);
> >     
> >     I acknowledge that the 'std::initializer_list' version let's you add a 
> > third 'ignoreErrors' parameter (which you couldn't do if instead of 'args' 
> > you used variadic parameters). But, do we really need the 'ignoreErrors' 
> > parameter? Why not just ask people to append '|| exit 0' to their command 
> > just like we ask people to append '2>&1'? I like the idea of just giving 
> > people a conduit to the shell, and however they'd do stuff in the shell 
> > world they do here too.
> >     
> >     Now, if we were _not_ using 'popen' under the covers or passing a 
> > string to '/bin/sh' then I would definitely see the value in a 
> > 'std::initializer_list' because then I wouldn't need to quote the command! 
> > But unfortunately we're going to have to force people to quote the command 
> > no matter what.
> >     
> >     Thus, my suggestion is to ask people to append '|| exit 0' and then go 
> > with a variadic template implementation, i.e., (E), or if you want to be 
> > dead simple do (D) and then use 'strings::format' everywhere in the next 
> > review.
> >     
> >     (Note that we use '%s' with our 'strings::format' for all types kind of 
> > like we use '{}' in Python string templates.)

hahaha the `"... || true"` was the first thing I thought (and used to prove the 
failure) then went like "OMG, they'll make fun of me" and used `ignoreErrors` 
instead - totally up for it!

As for why I changed the call signature, I am not quite sure either... the more 
I think about it, it must have been a reflective "Java-ism": varargs (and 
arrays) feel very pre-1.5, so I just get rid of them whenever I see them: this 
clearly doesn't apply to C++, though!

Reverted back to the original signature (minus `&stdout`, obviously) and, you 
are totally right: from a caller's perspective is a way superior user 
experience!


- Marco


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94348
-----------------------------------------------------------


On Aug. 5, 2015, 7:56 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36978/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 7:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3142
>     https://issues.apache.org/jira/browse/MESOS-3142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactoring os::shell.
> See MESOS-3142 for more details.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> ab767ccc4553cc5f61e4fe1b67110a9b5b32f2bc 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
> 53f14e1869ed7a6e1ac7cc8a82c558ed77907dc9 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
> 4b7a7bafe1c64183d021b39cf12523250491f0ee 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 2556bd428cc8990659e30e804b9c96c1659ef4a1 
> 
> Diff: https://reviews.apache.org/r/36978/diff/
> 
> 
> Testing
> -------
> 
> make check
> *Note*: this patch by itself breaks mesos - this only fixes the `stout` part: 
> see also https://reviews.apache.org/r/36979/
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Reply via email to