You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Daniel Pravat <dp...@outlook.com> on 2016/04/11 06:20:31 UTC
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/
-----------------------------------------------------------
(Updated April 11, 2016, 4:20 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
Repository: mesos
Description
-------
Stout: [1/2] Implemented assorted `os::` functions on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/44123/diff/
Testing (updated)
-------
OSX: make check
Thanks,
Daniel Pravat
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/
-----------------------------------------------------------
(Updated April 19, 2016, 12:09 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
Repository: mesos
Description
-------
Stout: [1/2] Implemented assorted `os::` functions on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/44123/diff/
Testing
-------
OSX: make check
Thanks,
Daniel Pravat
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review129407
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp
<https://reviews.apache.org/r/44123/#comment192833>
I think this breaks the linux build?
- Joris Van Remoortere
On April 15, 2016, 7:10 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 15, 2016, 7:10 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/
-----------------------------------------------------------
(Updated April 15, 2016, 7:10 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
Repository: mesos
Description
-------
Stout: [1/2] Implemented assorted `os::` functions on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/44123/diff/
Testing
-------
OSX: make check
Thanks,
Daniel Pravat
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Daniel Pravat <dp...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/
-----------------------------------------------------------
(Updated April 15, 2016, 12:23 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
Repository: mesos
Description
-------
Stout: [1/2] Implemented assorted `os::` functions on Windows.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
Diff: https://reviews.apache.org/r/44123/diff/
Testing
-------
OSX: make check
Thanks,
Daniel Pravat
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 12, 2016, 2:57 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, lines 188-189
> > <https://reviews.apache.org/r/44123/diff/2/?file=1338568#file1338568line188>
> >
> > Where did `processes()` go?
It goes into a later review. We can add back the `= delete` until then.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review128413
-----------------------------------------------------------
On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 4:20 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review128413
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp (line 464)
<https://reviews.apache.org/r/44123/#comment191835>
why does this return a `const` string by value?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 97 - 98)
<https://reviews.apache.org/r/44123/#comment191836>
We don't need to break after return.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 128 - 130)
<https://reviews.apache.org/r/44123/#comment191837>
Please put the `NOTE:` on a new line.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 184 - 185)
<https://reviews.apache.org/r/44123/#comment191838>
Where did `processes()` go?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 212 - 213)
<https://reviews.apache.org/r/44123/#comment191839>
2 spaces for expression continuation.
If an expression does not fit on 1 line, please leave a blank line after it.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (lines 212 - 216)
<https://reviews.apache.org/r/44123/#comment191840>
Why does a `value_size` of 0 mean `None`?
Is it not just the empty string?
Can you add a comment here?
- Joris Van Remoortere
On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 4:20 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Alex Clemmer <cl...@gmail.com>.
> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 168
> > <https://reviews.apache.org/r/44123/diff/2/?file=1338568#file1338568line168>
> >
> > Do we no longer have a "not implemented" error?
Per our conversation on Slack, we have decided that we never did.
> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 86
> > <https://reviews.apache.org/r/44123/diff/2/?file=1338568#file1338568line86>
> >
> > Is it worth handling the error case here?
Ok, let's do it. In the case that the `FormatMessage` call failed, I choose to just output the error code instead of calling `FormatMessage` again, though. It can't be turtles _all_ the way down!
> On April 13, 2016, 2:38 a.m., Alex Naparu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 232
> > <https://reviews.apache.org/r/44123/diff/2/?file=1338568#file1338568line232>
> >
> > We don't have a X_OK flag in Windows, so we might want to mask that out before calling `::access`.
Here's my perpective. If we try to call this with `X_OK` we will get a symbol undefined error. Also, if someone `#define`s it themselves, masking won't help. Also, we have a pattern for dealing with `#define`s, which is usually to explicitly not mask out flags that we don't `#define`.
Thoughts?
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review128602
-----------------------------------------------------------
On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 4:20 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Alex Naparu <al...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review128602
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 86)
<https://reviews.apache.org/r/44123/#comment192052>
Is it worth handling the error case here?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 164)
<https://reviews.apache.org/r/44123/#comment192053>
Do we no longer have a "not implemented" error?
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 228)
<https://reviews.apache.org/r/44123/#comment192054>
We don't have a X_OK flag in Windows, so we might want to mask that out before calling `::access`.
- Alex Naparu
On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 4:20 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>
Re: Review Request 44123: Stout: [1/2] Implemented assorted `os::`
functions on Windows.
Posted by Alex Naparu <al...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44123/#review128561
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 134)
<https://reviews.apache.org/r/44123/#comment191995>
You can probably inline this.
3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp (line 212)
<https://reviews.apache.org/r/44123/#comment191996>
This would probably fit on one line if you wrote it as "if (!::GetEnvironmentVariable..."
- Alex Naparu
On April 11, 2016, 4:20 a.m., Daniel Pravat wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44123/
> -----------------------------------------------------------
>
> (Updated April 11, 2016, 4:20 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Joris Van Remoortere, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: [1/2] Implemented assorted `os::` functions on Windows.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp edaa76a5322d0bf60b7172405aa754b5aca95458
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp c48106e5905e3be0faeba7177ef534766089faff
>
> Diff: https://reviews.apache.org/r/44123/diff/
>
>
> Testing
> -------
>
> OSX: make check
>
>
> Thanks,
>
> Daniel Pravat
>
>