Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-25 Thread Daniel Pravat 


> On Aug. 22, 2016, 6:39 p.m., Joseph Wu wrote:
> > Note: Your summary/description says `execlp`, but the code is for `execvp` 
> > :P

I will change the summary.


- Daniel


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


On Aug. 19, 2016, 5:38 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-24 Thread Joseph Wu 

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/windows/shell.hpp (lines 114 - 115)


Suggestion for the comment:
```
// On Windows, the `_spawnvp` call creates a new process.
// In order to emulate the semantics of `execvp`, we spawn with `_P_WAIT`,
// which force the parent process to block on the child.  When the child 
exits,
// the exit code is propagated back through the parent (via `exit()`).
```

Similar for the `execlp` comment, but s/vp/lp/.


- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-24 Thread Daniel Pravat 


> On Aug. 22, 2016, 6:39 p.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp, lines 114-115
> > 
> >
> > I'm not sure I understand what this comment is saying.  Are you 
> > explaining the reason behind the `exit(...)`?

On Posix: 
After a process X calls execlp to execute the application Y, the application Y 
runs in the process originally created to run the application X. Therefore the 
parent of X can use PID/pipes from the invocation of X to control the 
application Y
On Windows: 
After a process X calls execlp to execute the application Y, the application Y 
start a new process Y. The process X terminates at this time (it will never 
return from execlp https://msdn.microsoft.com/en-us/library/1kxct8h0.aspx). The 
parent of X cannot use X process handle to control the process Y. With this 
change the parent of X can use the X process handle to wait for the termination 
of Y, as in POSIX.


- Daniel


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


On Aug. 19, 2016, 5:38 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-22 Thread Joseph Wu 

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



Note: Your summary/description says `execlp`, but the code is for `execvp` :P


3rdparty/stout/include/stout/os/windows/shell.hpp (lines 114 - 115)


I'm not sure I understand what this comment is saying.  Are you explaining 
the reason behind the `exit(...)`?


- Joseph Wu


On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-19 Thread Daniel Pravat 

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

(Updated Aug. 19, 2016, 5:38 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Added `os::execlp' in `stout` library.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/shell.hpp 
1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
c6e141aba0abe2c7fe5410e867f7db47d632e765 

Diff: https://reviews.apache.org/r/51217/diff/


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-18 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [51210, 51217]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Aug. 18, 2016, 6:20 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 18, 2016, 6:20 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-18 Thread Daniel Pravat 

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

(Updated Aug. 18, 2016, 6:20 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Added `os::execlp' in `stout` library.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/shell.hpp 
1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
c6e141aba0abe2c7fe5410e867f7db47d632e765 

Diff: https://reviews.apache.org/r/51217/diff/


Testing
---


Thanks,

Daniel Pravat

Review Request 51217: Added `os::execlp' in `stout` library.

2016-08-18 Thread Daniel Pravat 

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Added `os::execlp' in `stout` library.


Diffs
-

  3rdparty/stout/include/stout/os/posix/shell.hpp 
1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
c6e141aba0abe2c7fe5410e867f7db47d632e765 

Diff: https://reviews.apache.org/r/51217/diff/


Testing
---


Thanks,

Daniel Pravat