You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Akash Gupta <ak...@hotmail.com> on 2018/05/30 18:42:43 UTC
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/
-----------------------------------------------------------
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67391/#review205217
-----------------------------------------------------------
Ship it!
I'll probably add TODOs around a couple changes here for when we remove libevent support from Windows.
3rdparty/libprocess/src/CMakeLists.txt
Lines 78-87 (patched)
<https://reviews.apache.org/r/67391/#comment288161>
NOTE: Right now, this adds POSIX files to Windows when libwinio is not enabled. This is so we have a staging period to test libwinio in "production" before removing libevent support.
When everyone is comfortable (and perhaps also when SSL support is added on top of libwinio), the option to use libevent (and so this POSIX files) will be removed from Windows, with libwinio as the only available option (that is, mandatory).
- Andrew Schwartzmeyer
On June 21, 2018, 4:58 a.m., Akash Gupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67391/
> -----------------------------------------------------------
>
> (Updated June 21, 2018, 4:58 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 367df4b1318167aef3aeba60a31e87eee65f7a71
> 3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
> 3rdparty/libprocess/src/CMakeLists.txt 619183eff6d6d301a011ab03f007410f50a0aa4f
> cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d
>
>
> Diff: https://reviews.apache.org/r/67391/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Akash Gupta
>
>
Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag
for Windows IOCP backend.
Posted by Akash Gupta <ak...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67391/
-----------------------------------------------------------
(Updated June 21, 2018, 11:58 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Changes
-------
Addressed feedback.
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 (updated)
-----
3rdparty/CMakeLists.txt 367df4b1318167aef3aeba60a31e87eee65f7a71
3rdparty/libprocess/include/process/io.hpp cc2caf44e065bed40263f3820e95a4f7c378bb98
3rdparty/libprocess/src/CMakeLists.txt 619183eff6d6d301a011ab03f007410f50a0aa4f
cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d
Diff: https://reviews.apache.org/r/67391/diff/2/
Changes: https://reviews.apache.org/r/67391/diff/1-2/
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
>
>
Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag
for Windows IOCP backend.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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
>
>