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