mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Schwartzmeyer <and...@schwartzmeyer.com>
Subject Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.
Date Tue, 13 Mar 2018 18:54:19 GMT

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




src/CMakeLists.txt
Lines 106 (patched)
<https://reviews.apache.org/r/65997/#comment279390>

    Should these be `PRIVATE` since they're "internal"?



src/CMakeLists.txt
Line 487 (original), 496 (patched)
<https://reviews.apache.org/r/65997/#comment279392>

    I guess this comment is out-of-date here; it should be deleted.



src/CMakeLists.txt
Lines 497-498 (patched)
<https://reviews.apache.org/r/65997/#comment279391>

    This is not necessary because mesos links to mesos-protobufs, the protobufs library/interface.



src/cmake/MesosProtobuf.cmake
Lines 125-163 (patched)
<https://reviews.apache.org/r/65997/#comment279395>

    Most of this function looks duplicated from `function(PROTOC_GENERATE)` but with additional
GRPC logic. Can they be combined or refactored? Looking at it, it seems we could add the same
options instead to `function(PROTOC_GENERATE)` and re-use it. Is there anything that `function(PROTOC_GENERATE)`
does which `function(PROTOC_SPEC_GENERATE)` _shouldn't_ do, and can that be conditionalized?
    
    I'm not saying this is a great idea; it may not make sense to share the code. But let's
take a look.



src/cmake/MesosProtobuf.cmake
Lines 174-176 (patched)
<https://reviews.apache.org/r/65997/#comment279393>

    This is probably something for later, but these essentially hard-coded directories have
never sat well with me.


- Andrew Schwartzmeyer


On March 9, 2018, 2:44 p.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
> 
> (Updated March 9, 2018, 2:44 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph
Wu.
> 
> 
> Bugs: MESOS-8657
>     https://issues.apache.org/jira/browse/MESOS-8657
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> PROTOC_SPEC_GENERATE is a convenience function that will:
>   (1) Compile .proto files found in the third-party specification
>       library under its include directory and place the generated files
>       in the build folder, under the `include/` directory, or with the
>       option `INTERNAL` the `src/` directory.
>   (2) Append to list variables `PUBLIC_PROTO_PATH` or
>       `INTERNAL_PROTO_PATH` the fully qualified path to the library's
>       include directory depending on options passed in.
>   (3) Append to list variables `PUBLIC_PROTOBUF_INCLUDE_DIR` or
>       `INTERNAL_PROTOBUF_INCLUDE_DIR` (depending on options passed in)
>       the fully qualified path to the directory where the files are
>       generated.
>   (4) Append to list variables `PUBLIC_PROTOBUF_SRC` or
>       `INTERNAL_PROTOBUF_SRC` (depending on options passed in) the fully
>       qualified path to the generated .cc file.
> Where the exports in (2)(3)(4) are *side effects* and modifies the
> variables in the parent scope.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
>   src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505 
> 
> 
> Diff: https://reviews.apache.org/r/65997/diff/2/
> 
> 
> Testing
> -------
> 
> `make check` with cmake.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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