Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 25
> > 
> >
> > Couple of questions here:
> > 
> > * This seems useful for POSIX platforms, but I don't really understand 
> > the implications of having this enabled on Windows. Windows will never 
> > support the signal semantics of POSIX, and in general my feeling is that 
> > I'd rather not define things on platforms that don't support those 
> > semantics, unless there is a particularly pertinent reason to. Thoughts?
> > * In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it 
> > for Windows, we simply `#ifdef` the code out. So the precedent is to just 
> > not use the macro. Is there a compelling reason to change this?
> > 
> > 
> > Also, tiny, tiny nit question: is it style to have 2 spaces between 
> > `W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))`
> 
> Kevin Klues wrote:
> To answer both your bullets at once...
> If you look in `windows.hpp` all of the other `W*` macros from the posix 
> standard are defined in there (even though they may be meaningless on 
> windows). This is consistent with them all being defined in `sys/wait.h` on a 
> posix compliant system. This is why I separated the two by a simple `#ifdef`
> 
> Adding the code below to be added to both windows and poix systems:
> 
> ```
> #ifndef W_EXITCODE
> #define W_EXITCODE(ret, sig)  ((ret) << 8 | (sig))
> #endif
> ```
> 
> is just to cover a corner case where some libc variants don't actually 
> define this macro (because it's not technically posix compliant). Almost all 
> libc variants do define it, but a bug was filed against this because 
> (apparently) `musl` does not.
> 
> Given that adding this code makes both windows and any variant of libc 
> now define all of the `W*` macros symetrically, I think this is likely the 
> right way to do it for now. In the future when we completely strip out all 
> `W*` macros from windows, we can revisit this. 
> 
> ---
> 
> Regarding the formatting question. I just copy and pasted this from the 
> glibc header file. I don't think we have a preferred style for this, but I 
> tried it both ways just now, and it seems to be more readable as 2 spaces, so 
> I'll leave it.
> 
> Alex Clemmer wrote:
> I'm fine with this change being submitted because the applicable domain 
> of error is so small. But, I am hoping we can agree to not define 
> semantically empty macros in the future on Windows codepaths in the future. 
> So I will add some historical perspective to this thread. :)
> 
> The `W*` macros you bring up that made it into `windows.hpp` all made it 
> for extremely pragmatic reasons. We're not at all happy _per se_ with this 
> outcome.
> 
> For example, some of the macros are doing things like checking for the 
> results of signals that do not exist on Windows; our decision calculus, 
> essentially, was: (1) it hurt readability too much to `#ifdef` out their 
> usage in the codebase, (2) we were too time constrained with MesosCon 
> approaching to correctly factor them out correctly, and (3) the places where 
> we were checking these results had a negligible effect on Windows code paths 
> because the signal would never be triggered.
> 
> If `W_EXITCODE` is similarly justified, I don't quite see it. Maybe I'm 
> missing something. It looks like it's used twice on non-Windows codepaths.
> 
> I'm definitely sympathetic to the idea that we do not want to decrease 
> readability in the POSIX codepaths to help Windows, a minority use case, but 
> I think that dumping POSIX stuff into the Windows codepaths is a really 
> dangerous habit to get into, particularly when it's not needed. This has 
> caused me, personally, probably a hundred hours of debugging, all told.
> 
> Let me know if I'm missing something important here, but for now, my 
> recommendation is that we strongly avoid defining things we don't have a 
> strict operational justification for.

I agree on all fronts. We should always be mindful of this.


- Kevin


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


On Oct. 13, 2016, 12:08 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 13, 2016, 12:08 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated 

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Alex Clemmer]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 25
> > 
> >
> > Couple of questions here:
> > 
> > * This seems useful for POSIX platforms, but I don't really understand 
> > the implications of having this enabled on Windows. Windows will never 
> > support the signal semantics of POSIX, and in general my feeling is that 
> > I'd rather not define things on platforms that don't support those 
> > semantics, unless there is a particularly pertinent reason to. Thoughts?
> > * In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it 
> > for Windows, we simply `#ifdef` the code out. So the precedent is to just 
> > not use the macro. Is there a compelling reason to change this?
> > 
> > 
> > Also, tiny, tiny nit question: is it style to have 2 spaces between 
> > `W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))`
> 
> Kevin Klues wrote:
> To answer both your bullets at once...
> If you look in `windows.hpp` all of the other `W*` macros from the posix 
> standard are defined in there (even though they may be meaningless on 
> windows). This is consistent with them all being defined in `sys/wait.h` on a 
> posix compliant system. This is why I separated the two by a simple `#ifdef`
> 
> Adding the code below to be added to both windows and poix systems:
> 
> ```
> #ifndef W_EXITCODE
> #define W_EXITCODE(ret, sig)  ((ret) << 8 | (sig))
> #endif
> ```
> 
> is just to cover a corner case where some libc variants don't actually 
> define this macro (because it's not technically posix compliant). Almost all 
> libc variants do define it, but a bug was filed against this because 
> (apparently) `musl` does not.
> 
> Given that adding this code makes both windows and any variant of libc 
> now define all of the `W*` macros symetrically, I think this is likely the 
> right way to do it for now. In the future when we completely strip out all 
> `W*` macros from windows, we can revisit this. 
> 
> ---
> 
> Regarding the formatting question. I just copy and pasted this from the 
> glibc header file. I don't think we have a preferred style for this, but I 
> tried it both ways just now, and it seems to be more readable as 2 spaces, so 
> I'll leave it.

I'm fine with this change being submitted because the applicable domain of 
error is so small. But, I am hoping we can agree to not define semantically 
empty macros in the future on Windows codepaths in the future. So I will add 
some historical perspective to this thread. :)

The `W*` macros you bring up that made it into `windows.hpp` all made it for 
extremely pragmatic reasons. We're not at all happy _per se_ with this outcome.

For example, some of the macros are doing things like checking for the results 
of signals that do not exist on Windows; our decision calculus, essentially, 
was: (1) it hurt readability too much to `#ifdef` out their usage in the 
codebase, (2) we were too time constrained with MesosCon approaching to 
correctly factor them out correctly, and (3) the places where we were checking 
these results had a negligible effect on Windows code paths because the signal 
would never be triggered.

If `W_EXITCODE` is similarly justified, I don't quite see it. Maybe I'm missing 
something. It looks like it's used twice on non-Windows codepaths.

I'm definitely sympathetic to the idea that we do not want to decrease 
readability in the POSIX codepaths to help Windows, a minority use case, but I 
think that dumping POSIX stuff into the Windows codepaths is a really dangerous 
habit to get into, particularly when it's not needed. This has caused me, 
personally, probably a hundred hours of debugging, all told.

Let me know if I'm missing something important here, but for now, my 
recommendation is that we strongly avoid defining things we don't have a strict 
operational justification for.


- Alex


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


On Oct. 13, 2016, 12:08 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 13, 2016, 12:08 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

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

(Updated Oct. 13, 2016, 12:08 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie and Alex's comments.


Bugs: MESOS-6310
https://issues.apache.org/jira/browse/MESOS-6310


Repository: mesos


Description
---

This was motivated by the need for a default definition of
'W_EXITCODE' (since it is not technically POSIX compliant).


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am b0b08d8e0d284a88bc8daa4570540659b94dc2d0 
  3rdparty/stout/include/stout/os/wait.hpp PRE-CREATION 

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


Testing
---

sudo GTEST_FILTER="*Nested*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 17
> > 
> >
> > How much work is it to do this now? If we're going to have a dedicated 
> > header, I would greatly prefer to put all the definitions in one place.
> 
> Kevin Klues wrote:
> See comment below for why it probably makes sense to just leave things as 
> they are for now

See comment below for rationale.


- Kevin


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


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 28
> > 
> >
> > I've been out of the game awhile-- are we wrapping comments at 70 
> > characters these days?

I typically use a 72 character wrap for comments because it looks nicer. I only 
go up to (or near to) 80 if the line I'm commenting is around 80 characters 
itself.


> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 25
> > 
> >
> > Couple of questions here:
> > 
> > * This seems useful for POSIX platforms, but I don't really understand 
> > the implications of having this enabled on Windows. Windows will never 
> > support the signal semantics of POSIX, and in general my feeling is that 
> > I'd rather not define things on platforms that don't support those 
> > semantics, unless there is a particularly pertinent reason to. Thoughts?
> > * In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it 
> > for Windows, we simply `#ifdef` the code out. So the precedent is to just 
> > not use the macro. Is there a compelling reason to change this?
> > 
> > 
> > Also, tiny, tiny nit question: is it style to have 2 spaces between 
> > `W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))`

To answer both your bullets at once...
If you look in `windows.hpp` all of the other `W*` macros from the posix 
standard are defined in there (even though they may be meaningless on windows). 
This is consistent with them all being defined in `sys/wait.h` on a posix 
compliant system. This is why I separated the two by a simple `#ifdef`

Adding the code below to be added to both windows and poix systems:

```
#ifndef W_EXITCODE
#define W_EXITCODE(ret, sig)  ((ret) << 8 | (sig))
#endif
```

is just to cover a corner case where some libc variants don't actually define 
this macro (because it's not technically posix compliant). Almost all libc 
variants do define it, but a bug was filed against this because (apparently) 
`musl` does not.

Given that adding this code makes both windows and any variant of libc now 
define all of the `W*` macros symetrically, I think this is likely the right 
way to do it for now. In the future when we completely strip out all `W*` 
macros from windows, we can revisit this. 

---

Regarding the formatting question. I just copy and pasted this from the glibc 
header file. I don't think we have a preferred style for this, but I tried it 
both ways just now, and it seems to be more readable as 2 spaces, so I'll leave 
it.


> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 17
> > 
> >
> > How much work is it to do this now? If we're going to have a dedicated 
> > header, I would greatly prefer to put all the definitions in one place.

See comment below for why it probably makes sense to just leave things as they 
are for now


- Kevin


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


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Alex Clemmer]

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




3rdparty/stout/include/stout/wait.hpp (line 17)


How much work is it to do this now? If we're going to have a dedicated 
header, I would greatly prefer to put all the definitions in one place.



3rdparty/stout/include/stout/wait.hpp (line 25)


Couple of questions here:

* This seems useful for POSIX platforms, but I don't really understand the 
implications of having this enabled on Windows. Windows will never support the 
signal semantics of POSIX, and in general my feeling is that I'd rather not 
define things on platforms that don't support those semantics, unless there is 
a particularly pertinent reason to. Thoughts?
* In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it for 
Windows, we simply `#ifdef` the code out. So the precedent is to just not use 
the macro. Is there a compelling reason to change this?

Also, tiny, tiny nit question: is it style to have 2 spaces between 
`W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))`



3rdparty/stout/include/stout/wait.hpp (line 28)


I've been out of the game awhile-- are we wrapping comments at 70 
characters these days?


- Alex Clemmer


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Jie Yu]

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




3rdparty/stout/include/stout/wait.hpp (line 14)


Let's put it under stout/os/wait.hpp


- Jie Yu


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Jie Yu]

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



Can you update 3rdparty/stout/include/Makefile.am as well?

- Jie Yu


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


Bugs: MESOS-6310
https://issues.apache.org/jira/browse/MESOS-6310


Repository: mesos


Description
---

This was motivated by the need for a default definition of
'W_EXITCODE' (since it is not technically POSIX compliant).


Diffs
-

  3rdparty/stout/include/stout/wait.hpp PRE-CREATION 

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


Testing
---

sudo GTEST_FILTER="*Nested*" src/mesos-tests


Thanks,

Kevin Klues