Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.

2018-03-13 Thread Chun-Hung Hsiao 


> On March 13, 2018, 6:54 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 174-176 (patched)
> > 
> >
> > This is probably something for later, but these essentially hard-coded 
> > directories have never sat well with me.

Yeah this is kinda unfortunate. It would be better if the CSI package itself 
comes with its own makefile to build the proto files, and I might work on a PR 
to the CSI repo for this in the future. As a workaround for now, I think this 
hard-coded directory structure is good enough for us to handle similar cases we 
may have in the future.


- Chun-Hung


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


On March 9, 2018, 10: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, 10: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
> 
>

Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.

2018-03-13 Thread Chun-Hung Hsiao 


> On March 13, 2018, 6:54 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 125-163 (patched)
> > 
> >
> > 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.

Let me come up with another prototype that combines the two and we can see if 
it looks cleaner.


- Chun-Hung


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


On March 9, 2018, 10: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, 10: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
> 
>

Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.

2018-03-13 Thread Andrew Schwartzmeyer 

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




src/CMakeLists.txt
Lines 106 (patched)


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



src/CMakeLists.txt
Line 487 (original), 496 (patched)


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



src/CMakeLists.txt
Lines 497-498 (patched)


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



src/cmake/MesosProtobuf.cmake
Lines 125-163 (patched)


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)


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

Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for cmake.

2018-03-09 Thread Chun-Hung Hsiao 

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

(Updated March 9, 2018, 10:44 p.m.)


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


Changes
---

Added a `GRPC` option and made this reade for review.


Summary (updated)
-

Added `PROTOC_SPEC_GENERATE` helper for cmake.


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


Repository: mesos


Description (updated)
---

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

  src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef 
  src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505 


Diff: https://reviews.apache.org/r/65997/diff/2/

Changes: https://reviews.apache.org/r/65997/diff/1-2/


Testing (updated)
---

`make check` with cmake.


Thanks,

Chun-Hung Hsiao