You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2016/01/22 18:38:06 UTC
Review Request 42662: Added common command utils file.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.
Diffs
-----
src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
src/common/command_utils.hpp PRE-CREATION
src/common/command_utils.cpp PRE-CREATION
src/tests/common/command_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/42662/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
>
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
>
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same underlying mecheniam (os::execvpe) to exec the process. Why one works and the other one does not?
>
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", command} ` as arguments to the command. Thats what I understood from L275 in `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am missing something.
Reading more code, I believe the difference is because of the PATH of some commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
>
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
>
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same underlying mecheniam (os::execvpe) to exec the process. Why one works and the other one does not?
>
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", command} ` as arguments to the command. Thats what I understood from L275 in `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am missing something.
>
> Jojy Varghese wrote:
> Reading more code, I believe the difference is because of the PATH of some commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.
>
> Jie Yu wrote:
> execvpe will search PATH as well. I don't understand why sh is able to find the executable while execvpe cannot.
Reading further, i think you are right. There shouldnt be any difference. I will update the patch to use `argv`. I think when i tried earlier, I was not passing all the args to the exec.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
Why can't you use the argv version for sha512 as well?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
>
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
I don't understand. Both the argv and non-argv version use the same underlying mecheniam (os::execvpe) to exec the process. Why one works and the other one does not?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
>
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
>
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same underlying mecheniam (os::execvpe) to exec the process. Why one works and the other one does not?
>
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", command} ` as arguments to the command. Thats what I understood from L275 in `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am missing something.
>
> Jojy Varghese wrote:
> Reading more code, I believe the difference is because of the PATH of some commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.
execvpe will search PATH as well. I don't understand why sh is able to find the executable while execvpe cannot.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line46>
> >
> > We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
> >
> > For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
> >
> > ```
> > static Future<string> launch(
> > const string& path,
> > const vector<string>& argv)
> > {
> > ...
> > }
> > ```
>
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use the `argv` version and command like `sha512` would use the other one. I have the `Option<vector...>` here to differentiate between the two.
>
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
>
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it as name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. In fact i have tried using `argv` version of subprocess for `sha512` and it doesnt work.
>
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same underlying mecheniam (os::execvpe) to exec the process. Why one works and the other one does not?
the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", command} ` as arguments to the command. Thats what I understood from L275 in `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am missing something.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, lines 153-162
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line153>
> >
> > I don't think this check is necessary. We are basically checking what 'tar' will check later. Can you remove it?
The only reason I have these validations before we launch the process is to avoid launching the process if basic validations fail.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, lines 153-162
> > <https://reviews.apache.org/r/42662/diff/1/?file=1205717#file1205717line153>
> >
> > I don't think this check is necessary. We are basically checking what 'tar' will check later. Can you remove it?
>
> Jojy Varghese wrote:
> The only reason I have these validations before we launch the process is to avoid launching the process if basic validations fail.
That being said, let's try to avoid optimizing (usually means complexity) before there's data showing that it's a problem.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116116
-----------------------------------------------------------
Haven't looked at the tests yet. Will do another pass once the existing issues are addressed. Thanks Jojy for doing this. This is really useful!
src/common/command_utils.hpp (line 32)
<https://reviews.apache.org/r/42662/#comment177117>
Can we remove these namespaces? I found the following less verbose and readable:
```
command::tar
command::untar
command::sha512
```
than
```
command::archive::tar
command::archive::untar
command::digest::sha512
```
src/common/command_utils.hpp (line 34)
<https://reviews.apache.org/r/42662/#comment177113>
'{' in the next line.
src/common/command_utils.hpp (line 50)
<https://reviews.apache.org/r/42662/#comment177118>
inputFilePath is a little confusing to readers because it can be a directory as well.
How about renaming it to 'input' since the Path part if pretty much redundent as the type is Path.
src/common/command_utils.hpp (line 51)
<https://reviews.apache.org/r/42662/#comment177119>
Ditto.
s/outputFilePath/output/
src/common/command_utils.hpp (line 52)
<https://reviews.apache.org/r/42662/#comment177121>
Hum, you use 'directory' in the cpp file while using a different name here. Can you use 'directory' through out?
src/common/command_utils.hpp (line 63)
<https://reviews.apache.org/r/42662/#comment177123>
Ditto. s/inputFilePath/input/
src/common/command_utils.hpp (line 64)
<https://reviews.apache.org/r/42662/#comment177124>
Ditto. s/changeToDirectory/directory/
src/common/command_utils.hpp (lines 70 - 74)
<https://reviews.apache.org/r/42662/#comment177114>
Can you separate the patches for sha512? We try to avoid big patches.
src/common/command_utils.cpp (line 44)
<https://reviews.apache.org/r/42662/#comment177175>
I would s/launchCommand/launch/ since you've already had 'command' in namespace.
command::launch reads much more nicely than command::launchCommand
src/common/command_utils.cpp (line 46)
<https://reviews.apache.org/r/42662/#comment177176>
We try to avoid using Option<vector<...>> since the semantics is not clear between None() and an empty vector.
For this case, I would suggest we always use the argv version of subprocess to avoid escaping spaces.
```
static Future<string> launch(
const string& path,
const vector<string>& argv)
{
...
}
```
src/common/command_utils.cpp (line 70)
<https://reviews.apache.org/r/42662/#comment177188>
Any way to print the agv as well?
src/common/command_utils.cpp (lines 117 - 124)
<https://reviews.apache.org/r/42662/#comment177173>
See my comments below, no need for this.
src/common/command_utils.cpp (lines 127 - 144)
<https://reviews.apache.org/r/42662/#comment177174>
See my comments below. No need for this.
src/common/command_utils.cpp (lines 153 - 162)
<https://reviews.apache.org/r/42662/#comment177127>
I don't think this check is necessary. We are basically checking what 'tar' will check later. Can you remove it?
src/common/command_utils.cpp (lines 172 - 176)
<https://reviews.apache.org/r/42662/#comment177128>
Ditto my comment above. I don't think the check performed in `addChangeDirectoryFlag` is necessary. 'tar' will validate that as well. Let's keep the function in this file simple (just shelling out).
No need to the additional helper function here. We also avoid using non const references if possible.
src/common/command_utils.cpp (lines 191 - 193)
<https://reviews.apache.org/r/42662/#comment177153>
What will this print? the type which will an int? Can you introduce a helper method to stringify the enum?
src/common/command_utils.cpp (line 228)
<https://reviews.apache.org/r/42662/#comment177170>
Let's not introduce this helper method. You can just do:
```
return launch(path, argv)
.then([]() { return Nothing(); });
```
- Jie Yu
On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 5:38 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116600
-----------------------------------------------------------
Fix it, then Ship it!
This is looking good! Some nits and we're good to go.
src/common/command_utils.hpp (line 48)
<https://reviews.apache.org/r/42662/#comment177650>
Why untar has a default for 'directory' while tar does not?
src/common/command_utils.hpp (line 56)
<https://reviews.apache.org/r/42662/#comment177644>
Please update the comments accordingly.
src/common/command_utils.hpp (line 59)
<https://reviews.apache.org/r/42662/#comment177643>
s/inputFilePath/input/
src/common/command_utils.cpp (line 19)
<https://reviews.apache.org/r/42662/#comment177646>
is it used? Please make sure to remove headers that are not used.
src/common/command_utils.cpp (line 31)
<https://reviews.apache.org/r/42662/#comment177645>
is it used?
src/common/command_utils.cpp (line 45)
<https://reviews.apache.org/r/42662/#comment177649>
s/command/path/ to be consistent with subprocess.
src/common/command_utils.cpp (lines 55 - 56)
<https://reviews.apache.org/r/42662/#comment177648>
Instead of saving a lambda, can we just save the string that we want to log?
```
string command = strings::join(
", ",
path,
strings::join(", ", argv));
```
src/common/command_utils.cpp (lines 63 - 73)
<https://reviews.apache.org/r/42662/#comment177647>
You should be able to use strings::join here.
src/common/command_utils.cpp (line 143)
<https://reviews.apache.org/r/42662/#comment177651>
I don't think we need to check if 'directory' is empty or not. Otherwise, do you need to check if 'input' or 'output' is empty or not?
src/common/command_utils.cpp (line 161)
<https://reviews.apache.org/r/42662/#comment177652>
Please align '{' here accordingly.
src/common/command_utils.cpp (line 183)
<https://reviews.apache.org/r/42662/#comment177653>
Ditto on empty check.
src/tests/common/command_utils_tests.cpp (line 23)
<https://reviews.apache.org/r/42662/#comment177654>
Why do you need this? Please remove headers that are not needed please.
src/tests/common/command_utils_tests.cpp (line 33)
<https://reviews.apache.org/r/42662/#comment177655>
we try to avoid using 'using namespace' clause. For this test, I think it's pretty clean and readable to use command::tar, command::untar (i.e., no need for this clause).
src/tests/common/command_utils_tests.cpp (line 43)
<https://reviews.apache.org/r/42662/#comment177657>
I would suggest to rename it to TarTest. Two reasons: 1) we might have 'ar' util in the future 2) it's better to keep the name consistent with the command name to readability and searchability.
src/tests/common/command_utils_tests.cpp (line 46)
<https://reviews.apache.org/r/42662/#comment177659>
s/createTestFileInDir/createTestFile/
InDir is implied by the parameter 'directory'
src/tests/common/command_utils_tests.cpp (line 47)
<https://reviews.apache.org/r/42662/#comment177658>
s/filePath/file/
Path is the type, no need for the redundent Path in variable name.
src/tests/common/command_utils_tests.cpp (line 50)
<https://reviews.apache.org/r/42662/#comment177660>
s/testFilePath/testFile/
src/tests/common/command_utils_tests.cpp (line 54)
<https://reviews.apache.org/r/42662/#comment177661>
Do you need this temp variable?
src/tests/common/command_utils_tests.cpp (lines 93 - 95)
<https://reviews.apache.org/r/42662/#comment177662>
You can use EXPECT_SOME_EQ("test", os::read(testFile));
src/tests/common/command_utils_tests.cpp (line 103)
<https://reviews.apache.org/r/42662/#comment177663>
s/testFilePath/testFile/
src/tests/common/command_utils_tests.cpp (lines 122 - 124)
<https://reviews.apache.org/r/42662/#comment177664>
Ditto on using EXPECT_SOME_EQ
src/tests/common/command_utils_tests.cpp (line 135)
<https://reviews.apache.org/r/42662/#comment177665>
s/testFilePath/testFile/
src/tests/common/command_utils_tests.cpp (lines 157 - 159)
<https://reviews.apache.org/r/42662/#comment177667>
Ditto on using EXPECT_SOME_EQ
src/tests/common/command_utils_tests.cpp (lines 190 - 192)
<https://reviews.apache.org/r/42662/#comment177668>
Ditto.
src/tests/common/command_utils_tests.cpp (line 205)
<https://reviews.apache.org/r/42662/#comment177669>
s/testFilePath/testFile/
src/tests/common/command_utils_tests.cpp (lines 212 - 213)
<https://reviews.apache.org/r/42662/#comment177670>
Can you do the indentation like the following so that it's consistent with your style in BZIP2CompressFile
```
AWAIT_ASSERT_READY(tar(
testDir,
outputTarFile,
topDir,
Compression::GZIP));
```
src/tests/common/command_utils_tests.cpp (lines 229 - 231)
<https://reviews.apache.org/r/42662/#comment177671>
Ditto on using EXPECT_SOME_EQ
- Jie Yu
On Jan. 26, 2016, 2:18 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 2:18 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/common/command_utils.cpp, line 17
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224149#file1224149line17>
> >
> > Move to line 28.
According to google style guide (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes), the header file for an implementation should be included first. I thought we follow google style guide unless something is overridden.
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/tests/common/command_utils_tests.cpp, line 71
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224150#file1224150line71>
> >
> > Add a case for `tar/untar` failure.
Will add a TODO.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116665
-----------------------------------------------------------
On Jan. 27, 2016, 8:29 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2016, 8:29 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/tests/common/command_utils_tests.cpp, line 71
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224150#file1224150line71>
> >
> > Add a case for `tar/untar` failure.
>
> Jojy Varghese wrote:
> Will add a TODO.
>
> Jie Yu wrote:
> Maybe in a subsequent review. This patch is getting too big. @jojy, can you follow up with that?
@jie will do.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116665
-----------------------------------------------------------
On Jan. 28, 2016, 2:21 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2016, 2:21 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-4360
> https://issues.apache.org/jira/browse/MESOS-4360
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/common/command_utils.cpp, line 17
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224149#file1224149line17>
> >
> > Move to line 28.
>
> Jojy Varghese wrote:
> According to google style guide (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes), the header file for an implementation should be included first. I thought we follow google style guide unless something is overridden.
+1
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/common/command_utils.cpp, lines 91-92
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224149#file1224149line91>
> >
> > Move to one line if less than 80.
+1
> On Jan. 28, 2016, 12:52 a.m., Klaus Ma wrote:
> > src/tests/common/command_utils_tests.cpp, line 71
> > <https://reviews.apache.org/r/42662/diff/3/?file=1224150#file1224150line71>
> >
> > Add a case for `tar/untar` failure.
>
> Jojy Varghese wrote:
> Will add a TODO.
Maybe in a subsequent review. This patch is getting too big. @jojy, can you follow up with that?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116665
-----------------------------------------------------------
On Jan. 27, 2016, 8:29 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2016, 8:29 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Klaus Ma <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/#review116665
-----------------------------------------------------------
src/common/command_utils.cpp (line 17)
<https://reviews.apache.org/r/42662/#comment177739>
Move to line 28.
src/common/command_utils.cpp (lines 91 - 92)
<https://reviews.apache.org/r/42662/#comment177741>
Move to one line if less than 80.
src/tests/common/command_utils_tests.cpp (line 71)
<https://reviews.apache.org/r/42662/#comment177757>
Add a case for `tar/untar` failure.
- Klaus Ma
On Jan. 28, 2016, 4:29 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2016, 4:29 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This common file is a good place to add common command line utilities like tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the code
> base and this change would enable all those call sites to be replaced with a
> common code.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
> src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
> src/common/command_utils.hpp PRE-CREATION
> src/common/command_utils.cpp PRE-CREATION
> src/tests/common/command_utils_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/42662/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/
-----------------------------------------------------------
(Updated Jan. 28, 2016, 2:21 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-4360
https://issues.apache.org/jira/browse/MESOS-4360
Repository: mesos
Description
-------
This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.
Diffs
-----
src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
src/common/command_utils.hpp PRE-CREATION
src/common/command_utils.cpp PRE-CREATION
src/tests/common/command_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/42662/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/
-----------------------------------------------------------
(Updated Jan. 27, 2016, 8:29 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
addressed review.
Repository: mesos
Description
-------
This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.
Diffs (updated)
-----
src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
src/common/command_utils.hpp PRE-CREATION
src/common/command_utils.cpp PRE-CREATION
src/tests/common/command_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/42662/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 42662: Added common command utils file.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42662/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 2:18 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
review addressed.
Repository: mesos
Description
-------
This common file is a good place to add common command line utilities like tar,
digests(sha256, sha512, etc). Currently this functionality is spread in the code
base and this change would enable all those call sites to be replaced with a
common code.
Diffs (updated)
-----
src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e
src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f
src/common/command_utils.hpp PRE-CREATION
src/common/command_utils.cpp PRE-CREATION
src/tests/common/command_utils_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/42662/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese