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 2018/06/08 22:58:54 UTC
Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag
for Windows IOCP backend.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67391/#review204504
-----------------------------------------------------------
3rdparty/libprocess/include/process/io.hpp
Lines 29-39 (original), 29-41 (patched)
<https://reviews.apache.org/r/67391/#comment287021>
Should we re-use the header guard like this, or add a define to CMake?
3rdparty/libprocess/src/CMakeLists.txt
Lines 86-87 (patched)
<https://reviews.apache.org/r/67391/#comment287020>
There shouldn't be headers in CMake source lists... I'm not sure why the others are in here either...
3rdparty/libprocess/src/CMakeLists.txt
Lines 118-121 (original), 130-140 (patched)
<https://reviews.apache.org/r/67391/#comment287019>
I think you could just replace `,libev` with `,$<$<NOT:$<PLATFORM_ID:Windows>>:libev>` and get rid of the outside conditional.
cmake/CompilationConfigure.cmake
Line 87 (original), 87-92 (patched)
<https://reviews.apache.org/r/67391/#comment287022>
I'm sorry... I definitely remember saying I was going to do this...
cmake/CompilationConfigure.cmake
Line 87 (original), 87-92 (patched)
<https://reviews.apache.org/r/67391/#comment287023>
We should sanity check if this got defined on a non-Windows platform.
- Andrew Schwartzmeyer
On May 30, 2018, 11:42 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67391/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:42 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Windows IOCP backend to the build system. Now, there are two async
> backends, which are libevent through `ENABLE_LIBEVENT` and the Windows
> IOCP (libwinio) through `ENABLE_LIBWINIO`.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/CMakeLists.txt cf443dffd0663ecf02b7efd6f7094175b94aae19
> cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d
>
>
> Diff: https://reviews.apache.org/r/67391/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag
for Windows IOCP backend.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On June 8, 2018, 3:58 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/include/process/io.hpp
> > Lines 29-39 (original), 29-41 (patched)
> > <https://reviews.apache.org/r/67391/diff/1/?file=2032289#file2032289line29>
> >
> > Should we re-use the header guard like this, or add a define to CMake?
>
> Akash Gupta wrote:
> What's the difference between the two? I don't know enough about CMake.
IIRC the pattern is usually something like `option(ENABLE_LIBWINIO ...)`, `if (ENABLE_LIBWINIO) target_compile_definitions(libprocess PRIVATE ENABLE_LIBWINIO)` and then `#ifdef ENABLE_LIBWINIO`, rather than the side effect of the header guard getting defined when the files are included. Your way works too, and even seems simpler, just wondering if it'll confuse anyone.
> On June 8, 2018, 3:58 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/CMakeLists.txt
> > Lines 86-87 (patched)
> > <https://reviews.apache.org/r/67391/diff/1/?file=2032290#file2032290line90>
> >
> > There shouldn't be headers in CMake source lists... I'm not sure why the others are in here either...
>
> Akash Gupta wrote:
> Are they for the "private" headers? The headers I see here are all in process/src/*.hpp.
I fixed it upstream; when you rebase you can delete these headers from CMake.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67391/#review204504
-----------------------------------------------------------
On May 30, 2018, 11:42 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67391/
> -----------------------------------------------------------
>
> (Updated May 30, 2018, 11:42 a.m.)
>
>
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added Windows IOCP backend to the build system. Now, there are two async
> backends, which are libevent through `ENABLE_LIBEVENT` and the Windows
> IOCP (libwinio) through `ENABLE_LIBWINIO`.
>
>
> Diffs
> -----
>
> 3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/CMakeLists.txt cf443dffd0663ecf02b7efd6f7094175b94aae19
> cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d
>
>
> Diff: https://reviews.apache.org/r/67391/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>