You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2018/03/19 23:53:08 UTC

Re: Review Request 61118: libprocess: Building gRPC support with CMake.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/
-----------------------------------------------------------

(Updated March 19, 2018, 11:53 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.


Summary (updated)
-----------------

libprocess: Building gRPC support with CMake.


Bugs: MESOS-7810
    https://issues.apache.org/jira/browse/MESOS-7810


Repository: mesos


Description (updated)
-------

This patch enables CMake to build code for gRPC support in libprocess
along with tests.


Diffs (updated)
-----

  3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
  3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 


Diff: https://reviews.apache.org/r/61118/diff/4/

Changes: https://reviews.apache.org/r/61118/diff/3-4/


Testing (updated)
-------

`sudo make check` with CMake
`cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`


NOTE: Testing on Windowns is not done yet.


Thanks,

Chun-Hung Hsiao


Re: Review Request 61118: Building gRPC support in libprocess with CMake.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/#review201663
-----------------------------------------------------------


Ship it!




Ship It!

- Andrew Schwartzmeyer


On April 19, 2018, 7:31 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> -----------------------------------------------------------
> 
> (Updated April 19, 2018, 7:31 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7881
>     https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/CMakeLists.txt 1eca50a4d513b2fead111e453565edff0a3ee497 
>   3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/7/
> 
> 
> Testing
> -------
> 
> `cmake .. -DENABLE_GRPC=1` and then `cmake --build . --target check` on Linux and Windows.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Re: Review Request 61118: Building gRPC support in libprocess with CMake.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/
-----------------------------------------------------------

(Updated April 20, 2018, 2:31 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.


Changes
-------

Enabled Windows build.


Bugs: MESOS-7881
    https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
-------

This patch enables CMake to build code for gRPC support in libprocess
along with tests.


Diffs (updated)
-----

  3rdparty/libprocess/src/CMakeLists.txt 1eca50a4d513b2fead111e453565edff0a3ee497 
  3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 


Diff: https://reviews.apache.org/r/61118/diff/6/

Changes: https://reviews.apache.org/r/61118/diff/5-6/


Testing (updated)
-------

`cmake .. -DENABLE_GRPC=1` and then `cmake --build . --target check` on Linux and Windows.


Thanks,

Chun-Hung Hsiao


Re: Review Request 61118: Building gRPC support in libprocess with CMake.

Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/
-----------------------------------------------------------

(Updated April 17, 2018, 3:17 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.


Changes
-------

Partially addressed Andy's comment.


Summary (updated)
-----------------

Building gRPC support in libprocess with CMake.


Bugs: MESOS-7881
    https://issues.apache.org/jira/browse/MESOS-7881


Repository: mesos


Description
-------

This patch enables CMake to build code for gRPC support in libprocess
along with tests.


Diffs (updated)
-----

  3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
  3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 


Diff: https://reviews.apache.org/r/61118/diff/5/

Changes: https://reviews.apache.org/r/61118/diff/4-5/


Testing
-------

`sudo make check` with CMake
`cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`


NOTE: Testing on Windowns is not done yet.


Thanks,

Chun-Hung Hsiao


Re: Review Request 61118: Libprocess: Building gRPC support with CMake.

Posted by Chun-Hung Hsiao <ch...@apache.org>.

> On April 12, 2018, 4:55 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/tests/CMakeLists.txt
> > Lines 91 (patched)
> > <https://reviews.apache.org/r/61118/diff/4/?file=1982733#file1982733line91>
> >
> >     I think this should instead be appended above. That is, after setting `GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC ${GRPC_TESTS_PROTOS})`, to be consistent with the rest.

Since the files in `${GRPC_TESTS_PROTOS}` are generated files, should we put them into `${PROCESS_TESTS_SRC}`?


- Chun-Hung


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/#review201014
-----------------------------------------------------------


On March 19, 2018, 11:53 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> -----------------------------------------------------------
> 
> (Updated March 19, 2018, 11:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7881
>     https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/4/
> 
> 
> Testing
> -------
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Re: Review Request 61118: Building gRPC support in libprocess with CMake.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

> On April 12, 2018, 9:55 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/tests/CMakeLists.txt
> > Lines 91 (patched)
> > <https://reviews.apache.org/r/61118/diff/4/?file=1982733#file1982733line91>
> >
> >     I think this should instead be appended above. That is, after setting `GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC ${GRPC_TESTS_PROTOS})`, to be consistent with the rest.
> 
> Chun-Hung Hsiao wrote:
>     Since the files in `${GRPC_TESTS_PROTOS}` are generated files, should we put them into `${PROCESS_TESTS_SRC}`?
> 
> Andrew Schwartzmeyer wrote:
>     I think so. They are required for the build, and the exist as byproducts of some generation command, right? So they dependency graph should ensure that they get generated before the test target is built. Maybe bbannier has thoughts on that?

Yeah, we're not changing the semantics of it by adding it to the `PROCESS_TESTS_SRC` variable, since both variables are sent as the list of sources to the target. I mean, it's fine either way, just seems cleaner IMHO to get everything into the one variable and have the target use just it.


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/#review201014
-----------------------------------------------------------


On April 16, 2018, 8:17 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> -----------------------------------------------------------
> 
> (Updated April 16, 2018, 8:17 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7881
>     https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/5/
> 
> 
> Testing
> -------
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Re: Review Request 61118: Building gRPC support in libprocess with CMake.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

> On April 12, 2018, 9:55 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/tests/CMakeLists.txt
> > Lines 91 (patched)
> > <https://reviews.apache.org/r/61118/diff/4/?file=1982733#file1982733line91>
> >
> >     I think this should instead be appended above. That is, after setting `GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC ${GRPC_TESTS_PROTOS})`, to be consistent with the rest.
> 
> Chun-Hung Hsiao wrote:
>     Since the files in `${GRPC_TESTS_PROTOS}` are generated files, should we put them into `${PROCESS_TESTS_SRC}`?

I think so. They are required for the build, and the exist as byproducts of some generation command, right? So they dependency graph should ensure that they get generated before the test target is built. Maybe bbannier has thoughts on that?


- Andrew


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/#review201014
-----------------------------------------------------------


On April 16, 2018, 8:17 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> -----------------------------------------------------------
> 
> (Updated April 16, 2018, 8:17 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7881
>     https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/5/
> 
> 
> Testing
> -------
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Re: Review Request 61118: Libprocess: Building gRPC support with CMake.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61118/#review201014
-----------------------------------------------------------




3rdparty/libprocess/src/tests/CMakeLists.txt
Lines 91 (patched)
<https://reviews.apache.org/r/61118/#comment281946>

    I think this should instead be appended above. That is, after setting `GRPC_TESTS_PROTOS`, do a `list(APPEND PROCESS_TESTS_SRC ${GRPC_TESTS_PROTOS})`, to be consistent with the rest.



3rdparty/libprocess/src/tests/CMakeLists.txt
Lines 93 (patched)
<https://reviews.apache.org/r/61118/#comment281947>

    Maybe leave a note that this is for the generated gRPC headers.


- Andrew Schwartzmeyer


On March 19, 2018, 4:53 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61118/
> -----------------------------------------------------------
> 
> (Updated March 19, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-7881
>     https://issues.apache.org/jira/browse/MESOS-7881
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables CMake to build code for gRPC support in libprocess
> along with tests.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/CMakeLists.txt 0ce7dac5deea94623530820d173ce3ffe5b42ea4 
>   3rdparty/libprocess/src/tests/CMakeLists.txt d624402bc9efb43a130a2afdf27cfc1aa69010e4 
> 
> 
> Diff: https://reviews.apache.org/r/61118/diff/4/
> 
> 
> Testing
> -------
> 
> `sudo make check` with CMake
> `cd 3rdparty/libprocess/src/tests && make libprocess-tests && ./libprocess-tests`
> 
> 
> NOTE: Testing on Windowns is not done yet.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>