> 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 > >
