Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 11, 2015, 7:36 a.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Changes --- Replaced the test check; added a LOG(ERROR) stdout for when the command errors out. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Refactoring os::shell. See MESOS-3142 for more details. Diffs (updated) - 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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/#review94881 --- Ship it! 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 53) https://reviews.apache.org/r/36978/#comment149555 And the next iteration would be to use variadic templates and then call strings::format versus strings::internal::format which would make it so that we wouldn't have to do `.c_str()` at all the existing and future call sites! 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (lines 963 - 964) https://reviews.apache.org/r/36978/#comment149556 EXPECT_TRUE(strings::contains(result.get(), No such file or directory)); - Benjamin Hindman On Aug. 6, 2015, 6:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 6, 2015, 6:20 p.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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 50 https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line50 should the variable be called `_cmd`? Marco Massenzio wrote: note this is neither a private member, not a constructor arg - it's a temp var: AFAIK there are no guidelines (apart from the obvious naming it sensibly). Renamed `command` Artem Harutyunyan wrote: It does not really matter since you renamed, but there is a guideline for function arguments: ``` We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing: ``` ah! It *does* matter: I know something that I didn't, but should have :) Thanks! - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/#review94162 --- On Aug. 6, 2015, 6:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 6, 2015, 6:20 p.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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
On Aug. 4, 2015, 7:54 p.m., Artem Harutyunyan wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 50 https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line50 should the variable be called `_cmd`? Marco Massenzio wrote: note this is neither a private member, not a constructor arg - it's a temp var: AFAIK there are no guidelines (apart from the obvious naming it sensibly). Renamed `command` It does not really matter since you renamed, but there is a guideline for function arguments: ``` We prepend constructor and function arguments with a leading underscore to avoid ambiguity and / or shadowing: ``` - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/#review94162 --- On Aug. 6, 2015, 11:20 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 6, 2015, 11:20 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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/ --- (Updated Aug. 6, 2015, 6:20 p.m.) Review request for mesos, Benjamin Hindman and Artem Harutyunyan. Changes --- Following Ben's comments, reverted the call signature to accept varargs. Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Refactoring os::shell. See MESOS-3142 for more details. Diffs (updated) - 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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
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, vectorstring{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 '21'? 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
-
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
--- 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. Changes --- Artem's comments Bugs: MESOS-3142 https://issues.apache.org/jira/browse/MESOS-3142 Repository: mesos Description --- Refactoring os::shell. See MESOS-3142 for more details. Diffs (updated) - 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
Re: Review Request 36978: MESOS-3142 Refactoring os::shell - patch 1/2
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36978/#review94348
---
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 44)
https://reviews.apache.org/r/36978/#comment148934
s/cmd/command/
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (line 45)
https://reviews.apache.org/r/36978/#comment148935
s/cmd/command/
s/empyt/empty/
3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp (lines 55 -
57)
https://reviews.apache.org/r/36978/#comment148932
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, vectorstring{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 '21'? 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.)
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp (line 31)
https://reviews.apache.org/r/36978/#comment148920
s/cmd/command/ (matches the declaration).
- Benjamin Hindman
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
