You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2016/12/08 00:21:14 UTC
Re: Review Request 54335: Add os::var() to Stout.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 12:21 a.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
Add os::var() to Stout.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows API to look up correct location for persistent,
app-local variable data. Returns standard location on POSIX.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp c37e64db662ba3cee83d2f55de0f9d71ad72c038
3rdparty/stout/include/stout/windows/os.hpp de9b04ad82443038a0f4408bc72cae1540a1beaf
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:15 p.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Fix formatting of description.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows. Uses Windows
COM API to look up correct location for persistent, app-local (but not per user)
variable data. Returns standard location on POSIX.
The addition of `os::var()` is a continuation of the fix in #54336. The correct
place for variable runtime data on Windows is not necessarily in `os::temp()`, but
in the analogous location `ProgramData`. Thus we need a platform-agnostic way to
refer to var.
The call to `ShGetKnownFolder` is not RAII because it is a C API, and the ATL
`CComHeapPtr` class is not used in this commit due to Windows header issues. Thus
the buffer allocated by the C API is freed immediately after the data is copied
into a `std::wstring`. Because the Windows API returns a UTF-16 string, and
Unicode characters are valid in Windows path names, we have to correctly convert
it to UTF-8 using `<codecvt>`.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:07 p.m.)
Review request for mesos and Alex Clemmer.
Changes
-------
Put back updated description.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows. Uses Windows
COM API to look up correct location for persistent, app-local (but not per user)
variable data. Returns standard location on POSIX.
The addition of os::var() is a continuation of the fix in #54336. The correct
place for variable runtime data on Windows is not necessarily in os::temp(), but
in the analogous location ProgramData. Thus we need a platform-agnostic way to
refer to var.
The call to ShGetKnownFolder is not RAII because it is a C API, and the ATL
CComHeapPtr class is not used in this commit due to Windows header issues. Thus
the buffer allocated by the C API is freed immediately after the data is copied
into a std::wstring. Because the Windows API returns a UTF-16 string, and
Unicode characters are valid in Windows path names, we have to correctly convert
it to UTF-8 using <codecvt>.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 8:02 p.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data.
Returns standard location on POSIX.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows.hpp d89c70902cf60544441608c2cb290b0727cbb45c
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158503
-----------------------------------------------------------
Ship it!
Oops, sorry, meant to mark this as "Fix it, then ship it!" My bad.
- Alex Clemmer
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 20
> > <https://reviews.apache.org/r/54335/diff/1-2/?file=1575119#file1575119line20>
> >
> > Tiny nit: while you're messing around with the headers, could we alphabetize them?
No problem.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 772
> > <https://reviews.apache.org/r/54335/diff/1-2/?file=1575119#file1575119line772>
> >
> > I think it's worth mentioning specifically something like "will not cause us to return different directories for different users." I think when I read this in x years it will not be super clear to me what you mean when you say "will not change the result." :) Change the result, with respect to what?
Ah, yeah that could be more clear.
> On Dec. 8, 2016, 6:14 a.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 754
> > <https://reviews.apache.org/r/54335/diff/2/?file=1579535#file1579535line754>
> >
> > Mesos style is to use `wchar_t* var_folder` rather than `wchar_t *var_folder`.
> >
> > Also, smaller question: is there a particular reason we switched from `PWSTR` to `wchar_t`? If not, it seems to me that if the win32 API is going to go through the trouble of using opaque typedefs, we should use those when we call their functions, too. (And convert to something more appropriate to Mesos before returning any data.) Here and elsewhere. Or, alternatively, we might consider putting a `static_assert` into Windows to make sure these are the same type. (Perhaps we should do this anyway since I believe the `wstring` constructor below doesn't take `PWSTR`.)
Who puts the pointers on the types?!? Google Style Guide says either works (I did check :). I'll fix it, thanks.
`PWSTR` to `wchar_t` because who on earth wants Hungarian notation in this code. I like your idea of a `static_assert`, that's the best solution here. `PWSTR` *should be* pointer-to-wide-c-string, but since we require that in our use, we should ensure it.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/#review158500
-----------------------------------------------------------
3rdparty/stout/include/stout/windows/os.hpp (line 20)
<https://reviews.apache.org/r/54335/#comment229265>
Tiny nit: while you're messing around with the headers, could we alphabetize them?
3rdparty/stout/include/stout/windows/os.hpp (line 756)
<https://reviews.apache.org/r/54335/#comment229266>
I think it's worth mentioning specifically something like "will not cause us to return different directories for different users." I think when I read this in x years it will not be super clear to me what you mean when you say "will not change the result." :) Change the result, with respect to what?
3rdparty/stout/include/stout/windows/os.hpp (line 754)
<https://reviews.apache.org/r/54335/#comment229264>
Mesos style is to use `wchar_t* var_folder` rather than `wchar_t *var_folder`.
Also, smaller question: is there a particular reason we switched from `PWSTR` to `wchar_t`? If not, it seems to me that if the win32 API is going to go through the trouble of using opaque typedefs, we should use those when we call their functions, too. (And convert to something more appropriate to Mesos before returning any data.) Here and elsewhere. Or, alternatively, we might consider putting a `static_assert` into Windows to make sure these are the same type. (Perhaps we should do this anyway since I believe the `wstring` constructor below doesn't take `PWSTR`.)
3rdparty/stout/include/stout/windows/os.hpp (lines 757 - 760)
<https://reviews.apache.org/r/54335/#comment229263>
I actually forget whether this is style compliant. We definitely do this in other parts of the codebase, (_e.g._, [1]), but I have occasionally been told in cases like these to put the first argument on the next line, and to indent those lines 4 spaces. I prefer this way, because it makes it easier to read the `!= S_OK`.
I'm not opening this as an issue, but I'm still leaving this as a comment to give Joseph the opportunity to set me straight here. :)
[1] https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/io.cpp#L91-L97
- Alex Clemmer
On Dec. 8, 2016, 1:49 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54335/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:49 a.m.)
>
>
> Review request for mesos and Alex Clemmer.
>
>
> Bugs: MESOS-6677 and MESOS-6722
> https://issues.apache.org/jira/browse/MESOS-6677
> https://issues.apache.org/jira/browse/MESOS-6722
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
> Uses Windows COM API to look up correct location for persistent,
> app-local (but not per user) variable data. Returns standard location on POSIX.
>
> The addition of `os::var()` is a continuation of the fix in #54336.
> The correct place for variable runtime data on Windows is not
> necessarily in `os::temp()`, but in the analogous location `ProgramData`.
> Thus we need a platform-agnostic way to refer to `var`.
>
> The call to `ShGetKnownFolder` is not RAII because it is a C API,
> and the ATL `CComHeapPtr` class is not used in this commit
> due to Windows header issues.
> Thus the buffer allocated by the C API is freed immediately after
> the data is copied into a `std::wstring`.
> Because the Windows API returns a UTF-16 string,
> and Unicode characters are valid in Windows path names,
> we have to correctly convert it to UTF-8 using `<codecvt>`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
> 3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
>
> Diff: https://reviews.apache.org/r/54335/diff/
>
>
> Testing
> -------
>
> make && make check on Linux: no failures.
> msbuild and attach to a master on Windows: no failures.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 1:49 a.m.)
Review request for mesos and Alex Clemmer.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data. Returns standard location on POSIX.
The addition of `os::var()` is a continuation of the fix in #54336.
The correct place for variable runtime data on Windows is not
necessarily in `os::temp()`, but in the analogous location `ProgramData`.
Thus we need a platform-agnostic way to refer to `var`.
The call to `ShGetKnownFolder` is not RAII because it is a C API,
and the ATL `CComHeapPtr` class is not used in this commit
due to Windows header issues.
Thus the buffer allocated by the C API is freed immediately after
the data is copied into a `std::wstring`.
Because the Windows API returns a UTF-16 string,
and Unicode characters are valid in Windows path names,
we have to correctly convert it to UTF-8 using `<codecvt>`.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing (updated)
-------
make && make check on Linux: no failures.
msbuild and attach to a master on Windows: no failures.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 54335: Add `os::var()` to Stout.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54335/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 12:39 a.m.)
Review request for mesos and Alex Clemmer.
Summary (updated)
-----------------
Add `os::var()` to Stout.
Bugs: MESOS-6677 and MESOS-6722
https://issues.apache.org/jira/browse/MESOS-6677
https://issues.apache.org/jira/browse/MESOS-6722
Repository: mesos
Description (updated)
-------
Returns `/var` on POSIX and (usually) `C:\ProgramData` on Windows.
Uses Windows COM API to look up correct location for persistent,
app-local (but not per user) variable data.
Returns standard location on POSIX.
Diffs (updated)
-----
3rdparty/stout/include/stout/posix/os.hpp 8443aa0cf0a8d8d52e36282611c2ab15ca4dd354
3rdparty/stout/include/stout/windows/os.hpp 2f20ccc64e255a60a1b7f33d684969942f12e45f
Diff: https://reviews.apache.org/r/54335/diff/
Testing
-------
Thanks,
Andrew Schwartzmeyer