> On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > >
Thanks for super-quick review! > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 40 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line40> > > > > Was not it discussed at some point that it would be good to have a > > funtion that returns a {status, stderr, stdout} tuple? This was relative to upgrading `process::Subprocess` - the idea here was the opposite: to greatly simplify the interface (and usage pattern) for `os::shell` See https://reviews.apache.org/r/36424/ > 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`? 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` > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, lines > > 97-99 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line97> > > > > alignment seems to be off here. uh? how did *that* happen? thanks! > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 66 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line66> > > > > Why not do `std::string cmd = _cmd + " " + strings::join(" ", args)?` > > and get rid of cmdLine? why not, indeed! > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 98 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026032#file1026032line98> > > > > Is there are reason for specifically calling out error 127? I would > > suggest a generic error message + stringify(WEXITSTATUS(status)). Because I'm trying to be a nice guy? :) Error 127 is really usually a "command not found" code (at least in *nix systems - no idea about Win32) > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 881 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line881> > > > > Maybe it makes sense to factor out os::shell tests into a separate > > function. I have created a new test fixture for os::shell specifically; adding a new file would seem a bit overkill? (note the other review has a lot of tests for os::shell updated too, so refactoring would be a bit of a major effort for not much gain). > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 885 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line885> > > > > From what I've seen the variables with _ are used when some > > transformation needs to be applied on the original value: > > > > ``` > > _var = transform1(__var); > > var = transform2(_var); > > ``` I'm just reusing what was already there? The rationale, I guess, was to name it in an obvious way, but leaving room for the actual `user` further down? I don't know, really. Do we really care? usage/intent seems abundantly clear here. > On Aug. 5, 2015, 2:54 a.m., Artem Harutyunyan wrote: > > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, line 882 > > <https://reviews.apache.org/r/36978/diff/1/?file=1026033#file1026033line882> > > > > Could you please add a test that makes sure that redirecting stderr to > > stdout with 2>&1 works? Awesome catch! turns out that even adding `2>&1` was going to be ignored, as the non-zero exit code forces an `Error` to be returned. I've added an `ignoreErrors` flag, that could be used for this purpose: ``` result = os::shell("ls /tmp/foobar889076 2>&1", vector<string>{}, true); ``` see the tests for an example. - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36978/#review94162 ----------------------------------------------------------- On Aug. 5, 2015, 12:54 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, 12:54 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/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 > >
