mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66163: Built storage local resource provider with CMake.
Date Fri, 20 Apr 2018 02:19:29 GMT


> On April 12, 2018, 5:06 p.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 387-390 (patched)
> > <https://reviews.apache.org/r/66163/diff/3/?file=1996903#file1996903line387>
> >
> >     Do you know which targets actually require `ENABLE_GRPC` set as a pre-processor
definition? Since it's new code being added, we should be able to resovle this TODO before
committing.
> 
> Chun-Hung Hsiao wrote:
>     For now it's just `src/resource_provider/local.cpp`. What's the best way to achieve
this? IMHO it's not that bad to make this global. ;)
> 
> Andrew Schwartzmeyer wrote:
>     Change
>     ```
>     target_compile_definitions(mesos PUBLIC USE_CMAKE_BUILD_CONFIG)
>     ```
>     to
>     ```
>     target_compile_definitions(
>       mesos
>       PUBLIC USE_CMAKE_BUILD_CONFIG
>       PRIVATE ENABLE_GRPC)
>     ```
>     
>     But yeah, we still have `add_definitions(-DUSE_SSL_SOCKET=1)`, so this can be a `TODO`.

Actually I'm not sure if having an explicit list of targets that the definition takes effects
would reduce the cost of code maintanance or it would increase the cost. It seems to me that
people that are unfamiliar with this list could easily run into a case where a definition
doesn't take effects on a new target and they will spend a lot of time debugging it.


- Chun-Hung


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


On April 17, 2018, 3:20 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66163/
> -----------------------------------------------------------
> 
> (Updated April 17, 2018, 3:20 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph
Wu.
> 
> 
> Bugs: MESOS-8698
>     https://issues.apache.org/jira/browse/MESOS-8698
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds CMake rules for compiling necessary source code for
> building storage local resource provider and related tests.
> 
> 
> Diffs
> -----
> 
>   cmake/CompilationConfigure.cmake 08d154e6e4b718b6059afb70e0b0235d1725d72d 
>   src/CMakeLists.txt 31af9eaf302122b844e7599944097ad1c3faae3a 
>   src/examples/CMakeLists.txt e85eed607715d2b3ee596c3c0b51f16682491900 
>   src/resource_provider/storage/CMakeLists.txt PRE-CREATION 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
> 
> 
> Diff: https://reviews.apache.org/r/66163/diff/4/
> 
> 
> Testing
> -------
> 
> `sudo make check` with CMake
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message