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/09 22:44:25 UTC
Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for
cmake.
-----------------------------------------------------------
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
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 14, 2018, 7:50 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/65997/diff/3/?file=1975055#file1975055line96>
> >
> > We put CPP_OUT files under an build/include/lib?
>
> Chun-Hung Hsiao wrote:
> Yeah. The reason is that, for CSI support we introduce the `DiskProfileAdaptor` Mesos module that uses `csi::VolumeCapability`: https://github.com/apache/mesos/blob/1.5.x/include/mesos/resource_provider/storage/disk_profile.hpp#L66
> For this reason, we put `spec.hpp` under `include/csi`, which includes `csi/csi.pb.h` from the `build` dir.
>
> Now I rethink about this, actually we don't need the `{PUBLIC,INTERNAL}_PROTOBUF_INCLUDE_DIR` at all since we have `csi/` prepended in `spec.hpp`. I'll remove this var.
Hmm due to the way the `protoc`-generated files include headers, we still need to put "include/csi" in the include path.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199202
-----------------------------------------------------------
On March 14, 2018, 2:56 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 2:56 a.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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in cmake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/3/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 14, 2018, 7:50 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 94 (patched)
> > <https://reviews.apache.org/r/65997/diff/3/?file=1975055#file1975055line96>
> >
> > We put CPP_OUT files under an build/include/lib?
Yeah. The reason is that, for CSI support we introduce the `DiskProfileAdaptor` Mesos module that uses `csi::VolumeCapability`: https://github.com/apache/mesos/blob/1.5.x/include/mesos/resource_provider/storage/disk_profile.hpp#L66
For this reason, we put `spec.hpp` under `include/csi`, which includes `csi/csi.pb.h` from the `build` dir.
Now I rethink about this, actually we don't need the `{PUBLIC,INTERNAL}_PROTOBUF_INCLUDE_DIR` at all since we have `csi/` prepended in `spec.hpp`. I'll remove this var.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199202
-----------------------------------------------------------
On March 14, 2018, 2:56 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 2:56 a.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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in cmake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/3/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199202
-----------------------------------------------------------
src/cmake/MesosProtobuf.cmake
Line 24 (original), 24 (patched)
<https://reviews.apache.org/r/65997/#comment279494>
Nit: s/support/supported
src/cmake/MesosProtobuf.cmake
Line 29 (original), 37 (patched)
<https://reviews.apache.org/r/65997/#comment279495>
Nit: s/if s/S
src/cmake/MesosProtobuf.cmake
Lines 51 (patched)
<https://reviews.apache.org/r/65997/#comment279496>
Same nit
src/cmake/MesosProtobuf.cmake
Lines 94 (patched)
<https://reviews.apache.org/r/65997/#comment279497>
We put CPP_OUT files under an build/include/lib?
src/cmake/MesosProtobuf.cmake
Lines 72-74 (original), 125-132 (patched)
<https://reviews.apache.org/r/65997/#comment279498>
Yeah! This looks reasonable.
src/cmake/MesosProtobuf.cmake
Lines 178-187 (patched)
<https://reviews.apache.org/r/65997/#comment279500>
I had previously added the custom mkdir targets to another file, and just added a dependency here. We should at least add a TODO to bring those here too (it seems to be a more appropriate location), and maybe a TODO to find a better way to ensure the directories are created. This always seemed hacky to me.
- Andrew Schwartzmeyer
On March 13, 2018, 7:56 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 13, 2018, 7:56 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in cmake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/3/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On March 14, 2018, 6:45 p.m., Andrew Schwartzmeyer wrote:
> > Ship It!
Tested on Windows and Linux (at least the build and tests all pass).
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199234
-----------------------------------------------------------
On March 14, 2018, 4:05 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 4:05 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in CMake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/4/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199234
-----------------------------------------------------------
Ship it!
Ship It!
- Andrew Schwartzmeyer
On March 14, 2018, 4:05 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 4:05 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in CMake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/4/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199226
-----------------------------------------------------------
Let me apply these patches on Windows and make sure everything is G2G before a ship it. Just a few minutes...
- Andrew Schwartzmeyer
On March 14, 2018, 4:05 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 4:05 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in cmake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/4/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 14, 2018, 11:51 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 105 (patched)
> > <https://reviews.apache.org/r/65997/diff/4/?file=1975954#file1975954line107>
> >
> > There needs to be an equivalent to these two `set(PROTO` lines in the `PROTOC_INTERNAL` logic to resolve the following dependency error:
> >
> > ```
> > > ninja tests
> > ninja: error: '3rdparty/csi-0.1.0/src/csi-0.1.0/csi.proto', needed by 'include/csi/csi.pb.cc', missing and no known rule to make it
> > ```
Missing `csi.proto` as a byproduct. Fixed in r66015.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199228
-----------------------------------------------------------
On March 14, 2018, 11:05 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 11:05 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in CMake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/4/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/#review199228
-----------------------------------------------------------
src/cmake/MesosProtobuf.cmake
Lines 105 (patched)
<https://reviews.apache.org/r/65997/#comment279529>
There needs to be an equivalent to these two `set(PROTO` lines in the `PROTOC_INTERNAL` logic to resolve the following dependency error:
```
> ninja tests
ninja: error: '3rdparty/csi-0.1.0/src/csi-0.1.0/csi.proto', needed by 'include/csi/csi.pb.cc', missing and no known rule to make it
```
src/cmake/MesosProtobuf.cmake
Line 111 (original), 215 (patched)
<https://reviews.apache.org/r/65997/#comment279531>
This line sets up the build dependency between `${CC}` and `${PROTO}`. The build error noted above is due to `${PROTO}` being empty when `${PROTOC_LIB}` is true.
- Andrew Schwartzmeyer
On March 14, 2018, 4:05 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65997/
> -----------------------------------------------------------
>
> (Updated March 14, 2018, 4:05 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_GENERATE now has the following new features:
> (1) With the `LIB` option, compile .proto files found in a third-party
> specification library.
> (2) Provides the `GRPC` option that, once we support gRPC in cmake,
> will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
> (3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
> or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
> include directory, and append to list variable
> `PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
> the fully qualified path to the directory where the generated
> `.pb.h` files are placed.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
> src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
>
>
> Diff: https://reviews.apache.org/r/65997/diff/4/
>
>
> Testing
> -------
>
> `make check` with cmake.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/
-----------------------------------------------------------
(Updated March 14, 2018, 11:05 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Andy's comments.
Bugs: MESOS-8657
https://issues.apache.org/jira/browse/MESOS-8657
Repository: mesos
Description
-------
PROTOC_GENERATE now has the following new features:
(1) With the `LIB` option, compile .proto files found in a third-party
specification library.
(2) Provides the `GRPC` option that, once we support gRPC in cmake,
will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
(3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
include directory, and append to list variable
`PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
the fully qualified path to the directory where the generated
`.pb.h` files are placed.
Diffs (updated)
-----
src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
Diff: https://reviews.apache.org/r/65997/diff/4/
Changes: https://reviews.apache.org/r/65997/diff/3-4/
Testing
-------
`make check` with cmake.
Thanks,
Chun-Hung Hsiao
Re: Review Request 65997: Made `PROTOC_GENERATE` compile proto files
from 3rd-party libraries.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65997/
-----------------------------------------------------------
(Updated March 14, 2018, 2:56 a.m.)
Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and Joseph Wu.
Changes
-------
Addressed Andy's comments.
Summary (updated)
-----------------
Made `PROTOC_GENERATE` compile proto files from 3rd-party libraries.
Bugs: MESOS-8657
https://issues.apache.org/jira/browse/MESOS-8657
Repository: mesos
Description (updated)
-------
PROTOC_GENERATE now has the following new features:
(1) With the `LIB` option, compile .proto files found in a third-party
specification library.
(2) Provides the `GRPC` option that, once we support gRPC in cmake,
will generate the `.grpc.pb.h` and `.grpc.pb.cc` files.
(3) With the `LIB` option, append to list variable `PUBLIC_PROTO_PATH`
or `INTERNAL_PROTO_PATH` the fully qualified path to the library's
include directory, and append to list variable
`PUBLIC_PROTOBUF_INCLUDE_DIR` or `INTERNAL_PROTOBUF_INCLUDE_DIR`
the fully qualified path to the directory where the generated
`.pb.h` files are placed.
Diffs (updated)
-----
src/CMakeLists.txt 0c135034982a6a4844509cd0e9b2cc41804333ef
src/cmake/MesosProtobuf.cmake ef90c15a6f2afaedaf7559c4a411098924528505
Diff: https://reviews.apache.org/r/65997/diff/3/
Changes: https://reviews.apache.org/r/65997/diff/2-3/
Testing
-------
`make check` with cmake.
Thanks,
Chun-Hung Hsiao
Re: Review Request 65997: Added `PROTOC_SPEC_GENERATE` helper for
cmake.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 13, 2018, 6:54 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 174-176 (patched)
> > <https://reviews.apache.org/r/65997/diff/2/?file=1973227#file1973227line174>
> >
> > 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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On March 13, 2018, 6:54 p.m., Andrew Schwartzmeyer wrote:
> > src/cmake/MesosProtobuf.cmake
> > Lines 125-163 (patched)
> > <https://reviews.apache.org/r/65997/diff/2/?file=1973227#file1973227line125>
> >
> > 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.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
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
>
>