Add build support for CXX consumers #69
Conversation
CMakeLists.txt
Outdated
| cmake_minimum_required(VERSION 3.3) | ||
|
|
||
| set(PACKAGE_NAME "csi") | ||
| set(PACKAGE_VERSION "0.0.1") |
There was a problem hiding this comment.
Shouldn't the version be "0.1.0"?
CMakeLists.txt
Outdated
| COMMENT "Generating protobuf file from the spec" | ||
| ) | ||
|
|
||
| add_library(${PACKAGE_NAME} ${CSI_CXX_PROTO_SOURCES}) |
There was a problem hiding this comment.
When the protobuf and grpc headers are in non-standard paths, the compilation of this library will fail. Let's add the following lines after Line 52:
find_path(PROTOBUF_INCLUDE_DIR google/protobuf/message.h)
find_path(GRPC_INCLUDE_DIR grpc++/grpc++.h)and the following after this line:
target_include_directories(
${PACKAGE_NAME}
PUBLIC ${PROTOBUF_INCLUDE_DIR}
PUBLIC ${GRPC_INCLUDE_DIR}
)There was a problem hiding this comment.
To use the headers in non-standard paths, we can then add -DCMAKE_INCLUDE_PATH="<protobuf_path>;<grpc_path>;..." when running cmake.
There was a problem hiding this comment.
On a second thought, let's not do this but ask the user to set up CMAKE_CXX_FLAGS instead.
CMakeLists.txt
Outdated
|
|
||
| set(CMAKE_CXX_STANDARD 11) | ||
|
|
||
| find_program(PROTOBUF_COMPILER NAMES protoc) |
There was a problem hiding this comment.
NAMES not required here. Ditto below.
| @@ -228,6 +228,9 @@ There are three sets of RPCs: | |||
| * **Node Service**: The Node Plugin MUST implement this sets of RPCs. | |||
|
|
|||
| ```protobuf | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
These lines will make the original make command fail due to Lines 124 & 138 in csi.mk. We probably want to either: 1. remove this and re-generate them in CMake; or 2. fix csi.mk; or 3. deprecate csi.mk and change the CI test.
CMakeLists.txt
Outdated
|
|
||
| set(CMAKE_CXX_STANDARD 11) | ||
|
|
||
| find_program(PROTOBUF_COMPILER NAMES protoc) |
There was a problem hiding this comment.
How about doing find_program only if PROTOBUF_COMPILER is not set? Then the user can set up it directly instead of setting up CMAKE_PROGRAM_PATH. Ditto below.
|
|
||
| ```bash | ||
| $ CSI_SPEC_FILE=spec.md make csi.proto | ||
| $ mkdir build |
There was a problem hiding this comment.
if the point of including golang build support here is to encourage people to reuse these bits then ... we've failed. this is too much to ask of golang devs. a typical golang devs workflow is go get github.com/org/project/package.
this all feels too complex for what the original CI scripts were attempting to do: validate that the protos in the spec actually compiled to something. I somewhat regret accepting a PR that complicated the process. This PR adds additional prerequisites and process .. things that I don't think are actually appropriate for this repo.
I guess I could be convinced otherwise, but as of this moment I don't see the justification for making this repo more complex.
| set(PROTOBUF_COMPILER ${PROTOBUF_COMPILER_PATH}) | ||
| endif () | ||
|
|
||
| if (NOT GRPC_CXX_PLUGIN) |
There was a problem hiding this comment.
if (NOT GRPC_CXX_PLUGIN_PATH)
Also, do we want to document these variables in the README file?
| set(GRPC_CXX_PLUGIN ${GRPC_CXX_PLUGIN_PATH}) | ||
| endif () | ||
|
|
||
| if (NOT GRPC_GO_PLUGIN) |
There was a problem hiding this comment.
if (NOT GRPC_GO_PLUGIN_PATH)
|
|
||
| if (NOT GRPC_CXX_PLUGIN) | ||
| find_program(GRPC_CXX_PLUGIN grpc_cpp_plugin) | ||
| else() |
There was a problem hiding this comment.
Let's follow the same style and put a space between else and (). Ditto below.
|
Close this for now. I'll resend a PR later that rebase onto the current master. |
This patch chain adds the support for CXX consumers. It uses cmake as the configuration tooling. Currently, it only generates the headers and a static library.