Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of `shell.hpp`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 29, 2016, 12:04 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40936/
> ---
> 
> (Updated Jan. 29, 2016, 12:04 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Alex Clemmer, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Unified POSIX and Windows implementation of `shell.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
> b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
> bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 74af0077a39ef4cfa636b0b9e0c6b93eabc04bc8 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 061bb4a9b9efe4ddd364f589175446b07152c315 
> 
> Diff: https://reviews.apache.org/r/40936/diff/
> 
> 
> Testing
> ---
> 
> OSX: make check
> Windows: Build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of `shell.hpp`.

[Michael Park]

> On Jan. 28, 2016, 10:41 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, 
> > lines 41-55
> > 
> >
> > I'm curious as to whether this struct is meant to be widely used aside 
> > from `os::system`?
> 
> Daniel Pravat wrote:
> These are just use for parameterss for shell in different place like 
> lunch.cpp, test code.
> 
> Michael Park wrote:
> Ok, how about we do the following?
> ```cpp
> struct Shell {
>   static constexpr const char* name = "cmd.exe";
>   static constexpr const char* arg0 = "cmd.exe";
>   static constexpr const char* arg1 = "/c";
> };
> ```

I'll commit this patch with this change, since other patches depend on this. If 
you don't agree with this change, let me know and we can tackle it in a 
separate patch :)


- Michael


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


On Jan. 29, 2016, 12:04 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40936/
> ---
> 
> (Updated Jan. 29, 2016, 12:04 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Alex Clemmer, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Unified POSIX and Windows implementation of `shell.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
> b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
> bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 74af0077a39ef4cfa636b0b9e0c6b93eabc04bc8 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 061bb4a9b9efe4ddd364f589175446b07152c315 
> 
> Diff: https://reviews.apache.org/r/40936/diff/
> 
> 
> Testing
> ---
> 
> OSX: make check
> Windows: Build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of `shell.hpp`.

[Michael Park]

> On Jan. 28, 2016, 10:41 a.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, 
> > lines 41-55
> > 
> >
> > I'm curious as to whether this struct is meant to be widely used aside 
> > from `os::system`?
> 
> Daniel Pravat wrote:
> These are just use for parameterss for shell in different place like 
> lunch.cpp, test code.

Ok, how about we do the following?
```cpp
struct Shell {
  static constexpr const char* name = "cmd.exe";
  static constexpr const char* arg0 = "cmd.exe";
  static constexpr const char* arg1 = "/c";
};
```


- Michael


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


On Jan. 29, 2016, 12:04 a.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40936/
> ---
> 
> (Updated Jan. 29, 2016, 12:04 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Alex Clemmer, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Unified POSIX and Windows implementation of `shell.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
> b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
> bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 74af0077a39ef4cfa636b0b9e0c6b93eabc04bc8 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 061bb4a9b9efe4ddd364f589175446b07152c315 
> 
> Diff: https://reviews.apache.org/r/40936/diff/
> 
> 
> Testing
> ---
> 
> OSX: make check
> Windows: Build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of `shell.hpp`.

[Daniel Pravat]

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

(Updated Jan. 29, 2016, 12:04 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, M Lawindi, and Yi Sun.


Summary (updated)
-

Windows: Unified POSIX and Windows implementation of `shell.hpp`.


Repository: mesos


Description (updated)
---

Windows: Unified POSIX and Windows implementation of `shell.hpp`.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
74af0077a39ef4cfa636b0b9e0c6b93eabc04bc8 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
061bb4a9b9efe4ddd364f589175446b07152c315 

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


Testing
---

OSX: make check
Windows: Build


Thanks,

Daniel Pravat

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp

[Daniel Pravat]

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

(Updated Jan. 14, 2016, 6:31 p.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Windows: Unified POSIX and Windows implementation of shell.hpp


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
4cf693fb7e8c6bb3ad1920ebe90d61f0adb5dc99 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
27edf1b4f8647a2720f2962eafa110d694b3baed 

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


Testing
---

OSX: make check
Windows: Build


Thanks,

Daniel Pravat

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp

[Daniel Pravat]

> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, line 
> > 73
> > 
> >
> > How semantically similar is this to the POSIX code? If it's very 
> > similar, does it make sense to transition the POSIX code to use the POSIX 
> > equivalent too?

Posix equivalne uses ::fork(). We cannot use that call. In theroy should have 
the same bahaviour.


> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, line 
> > 68
> > 
> >
> > The comments on the POSIX version of this function claims that it's 
> > async signal safe. Is that true of this version as well? Does that even 
> > make sense in Windows?

We don't support POSIX signals in Windows. The comment is not need it.


> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp, 
> > lines 25-28
> > 
> >
> > Historically this stuff has gone in `stout/windows.hpp`, so that if 
> > another function needs them, because (1) they don't all have to redefine 
> > the symbols, and (2) we wanted to keep all the Windows-compat stuff in one 
> > place until we had a clearer picture of what we wanted to do with this 
> > stuff in the long term.
> > 
> > In the long term, I think the answer is that we've already started 
> > moving toward a model where each compat header gets its own file (_e.g._, 
> > `dirent.h` gets `stout/internal/windows/dirent.hpp`), so I think we'll 
> > eventually come back and refactor all of `windows.hpp` to fit this model.
> > 
> > For the time being, though, I think it makes sense to just move this 
> > there.

I see similar file with platform defines in libprocess/src/config.hpp. Maybe we 
should use a single file with all the types/define missing in windows.  I will 
keep this in mind for further refactoring.


> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 
> > 120
> > 
> >
> > Sorry, template noob here. Why the variadic template here? Why not:
> > 
> > ```
> > int execlp(const char* path, const char* arg, ...)
> > {
> >   return ::execlp(path, arg);
> > }
> > ```
> > 
> > My understanding here is that `T` could be any number of type 
> > arguments, and the result of `const T*... t` is that you could pass in a 
> > bunch of different types into the `os::execlp` function.
> > 
> > Is that all correct?
> > 
> > If so, then I don't really understand what this freedom buys us, since 
> > it seems like in practice we'll only call it with `const char*`.

Removed the code. This is not requered


> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, line 
> > 110
> > 
> >
> > `arg0` and `arg1` seem like they might not be the best names for these. 
> > Perhaps something more descriptive?

The code has been removed. arg0 and arg1 follows the convention on the API 
param naming.


> On Jan. 6, 2016, 2:23 a.m., Alex Clemmer wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp, lines 
> > 104-118
> > 
> >
> > Looks like `shell_const` is used later, for commit of launch.cpp, right?
> > 
> > I'm fine with that, but I have a couple suggestions:
> > 
> > * Can we please indent the static members according to the style we use 
> > for structs?
> > * Brief search of the codebase reveals that calls to `exec*("sh", "sh", 
> > ...)` are not that numerous. By my count, there are a few in fork.hpp, 
> > launch.cpp, executor.cpp, and here in shell.hpp, on line 144. It seems like 
> > it's worth just transitioning them all right now, since they're not that 
> > numerous, and since it seems bad to have the code in an inconsistent state. 
> > What do you think?

We should mmake the transition when we add the files.


- Daniel


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


On Jan. 13, 2016, 7:14 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40936/
> ---
> 
> (Updated

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp

[Daniel Pravat]

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

(Updated Jan. 13, 2016, 7:14 p.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, and M Lawindi.


Repository: mesos


Description
---

Windows: Unified POSIX and Windows implementation of shell.hpp


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
4cf693fb7e8c6bb3ad1920ebe90d61f0adb5dc99 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp

[Daniel Pravat]

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

(Updated Dec. 23, 2015, 7:17 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, and M Lawindi.


Repository: mesos


Description
---

Windows: Unified POSIX and Windows implementation of shell.hpp


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
42733d429814bf0512540188264830aeaabcabbe 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 40936: Windows: Unified POSIX and Windows implementation of shell.hpp

[Daniel Pravat]

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

(Updated Dec. 22, 2015, 2:32 a.m.)


Review request for mesos, Alex Naparu, Alex Clemmer, and M Lawindi.


Repository: mesos


Description
---

Windows: Unified POSIX and Windows implementation of shell.hpp


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/shell.hpp 
b18cae1118302e18d2cfb7ce4089ab5079a01d1a 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/shell.hpp 
4e8d56bf778556341637487b7e6dcecedc8dcfbd 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/shell.hpp 
bf737c87b8a337cc46e6c16d6fec2eef61e6ea05 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
42733d429814bf0512540188264830aeaabcabbe 

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


Testing
---


Thanks,

Daniel Pravat