You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Aaron Wood via Review Board <no...@reviews.apache.org> on 2017/07/06 22:44:05 UTC
Review Request 60693: Move permissions.hpp under posix
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60693/
-----------------------------------------------------------
Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
Bugs: MESOS-7764
https://issues.apache.org/jira/browse/MESOS-7764
Repository: mesos
Description
-------
The way permissions are dealt with are not cross-compatible with Windows.
Diffs
-----
3rdparty/stout/include/stout/posix/os.hpp 8511dfd41
src/common/http.cpp 2f7718cbc
src/common/protobuf_utils.cpp 4e5ab02c9
src/credentials/credentials.hpp c790793c7
src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0
src/tests/fetcher_tests.cpp 99149baa1
Diff: https://reviews.apache.org/r/60693/diff/1/
Testing
-------
`mkdir build && cd build && cmake .. && make -j$(nproc) && make check -j$(nproc)`
Thanks,
Aaron Wood
Re: Review Request 60693: Move permissions.hpp under posix
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 7, 2017, 6:43 p.m., Jie Yu wrote:
> > src/common/protobuf_utils.cpp
> > Line 41 (original), 41 (patched)
> > <https://reviews.apache.org/r/60693/diff/1/?file=1770790#file1770790line41>
> >
> > It's really weird that this file is cross platform, but we have posix only header includes.
> >
> > That means the permissions.hpp compiles on Windows previously?
>
> Andrew Schwartzmeyer wrote:
> Not necessarily. With CMake, header files are not explicitly added to the build system. They're only compiled if a `cpp` file includes it; so we'd need to look at any file that includes `permissions.hpp` to determine if was previously compiled on Windows.
Verified: at the least `src\common\protobuf_utils.cpp` is including it, so it's compiling on Windows.
(Oh dur, it's this file!)
It doesn't surprise me that this file compiles though. The struct `Permissions` is fine; the `S_IRUSR` flags etc. are stubbed in `stout/windows.hpp`, and `::stat` is part of the CRT. I'd be surprised if this code actually returned what the user expected though.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60693/#review179901
-----------------------------------------------------------
On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 10:44 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
>
>
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The way permissions are dealt with are not cross-compatible with Windows.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8511dfd41
> src/common/http.cpp 2f7718cbc
> src/common/protobuf_utils.cpp 4e5ab02c9
> src/credentials/credentials.hpp c790793c7
> src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0
> src/tests/fetcher_tests.cpp 99149baa1
>
>
> Diff: https://reviews.apache.org/r/60693/diff/1/
>
>
> Testing
> -------
>
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check -j$(nproc)`
>
>
> Thanks,
>
> Aaron Wood
>
>
Re: Review Request 60693: Move permissions.hpp under posix
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On July 7, 2017, 6:43 p.m., Jie Yu wrote:
> > src/common/protobuf_utils.cpp
> > Line 41 (original), 41 (patched)
> > <https://reviews.apache.org/r/60693/diff/1/?file=1770790#file1770790line41>
> >
> > It's really weird that this file is cross platform, but we have posix only header includes.
> >
> > That means the permissions.hpp compiles on Windows previously?
Not necessarily. With CMake, header files are not explicitly added to the build system. They're only compiled if a `cpp` file includes it; so we'd need to look at any file that includes `permissions.hpp` to determine if was previously compiled on Windows.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60693/#review179901
-----------------------------------------------------------
On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 10:44 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
>
>
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The way permissions are dealt with are not cross-compatible with Windows.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8511dfd41
> src/common/http.cpp 2f7718cbc
> src/common/protobuf_utils.cpp 4e5ab02c9
> src/credentials/credentials.hpp c790793c7
> src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0
> src/tests/fetcher_tests.cpp 99149baa1
>
>
> Diff: https://reviews.apache.org/r/60693/diff/1/
>
>
> Testing
> -------
>
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check -j$(nproc)`
>
>
> Thanks,
>
> Aaron Wood
>
>
Re: Review Request 60693: Move permissions.hpp under posix
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60693/#review179901
-----------------------------------------------------------
src/common/protobuf_utils.cpp
Line 41 (original), 41 (patched)
<https://reviews.apache.org/r/60693/#comment254820>
It's really weird that this file is cross platform, but we have posix only header includes.
That means the permissions.hpp compiles on Windows previously?
- Jie Yu
On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 10:44 p.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
>
>
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The way permissions are dealt with are not cross-compatible with Windows.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/posix/os.hpp 8511dfd41
> src/common/http.cpp 2f7718cbc
> src/common/protobuf_utils.cpp 4e5ab02c9
> src/credentials/credentials.hpp c790793c7
> src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0
> src/tests/fetcher_tests.cpp 99149baa1
>
>
> Diff: https://reviews.apache.org/r/60693/diff/1/
>
>
> Testing
> -------
>
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check -j$(nproc)`
>
>
> Thanks,
>
> Aaron Wood
>
>