Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag for Windows IOCP backend.

2018-06-21 Thread Andrew Schwartzmeyer 

---
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)


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.

2018-06-21 Thread Akash Gupta 

---
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.

2018-06-20 Thread Andrew Schwartzmeyer 


> On June 8, 2018, 3:58 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/include/process/io.hpp
> > Lines 29-39 (original), 29-41 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

2018-06-08 Thread Andrew Schwartzmeyer 

---
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)


Should we re-use the header guard like this, or add a define to CMake?



3rdparty/libprocess/src/CMakeLists.txt
Lines 86-87 (patched)


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)


I think you could just replace `,libev` with 
`,$<$>:libev>` and get rid of the outside 
conditional.



cmake/CompilationConfigure.cmake
Line 87 (original), 87-92 (patched)


I'm sorry... I definitely remember saying I was going to do this...



cmake/CompilationConfigure.cmake
Line 87 (original), 87-92 (patched)


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
> 
>

Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag for Windows IOCP backend.

2018-05-30 Thread Akash Gupta 

---
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