-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Sprint 22/23 / PD-405] [[Feature] Add implementation GRPC #10
Conversation
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/proto/ninshiki.proto | ||
) | ||
|
||
add_library(ninshiki_proto SHARED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest not making the ninshiki_proto
a shared library, if possible:
add_library(ninshiki_proto SHARED | |
add_library(ninshiki_proto |
set(protobuf_MODULE_COMPATIBLE TRUE) | ||
find_package(Protobuf CONFIG REQUIRED) | ||
message(STATUS "Using protobuf ${Protobuf_VERSION}") | ||
|
||
find_package(gRPC CONFIG REQUIRED) | ||
message(STATUS "Using gRPC ${gRPC_VERSION}") | ||
|
||
file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/proto) | ||
add_custom_command( | ||
OUTPUT | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.pb.cc" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.pb.h" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.grpc.pb.cc" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.grpc.pb.h" | ||
COMMAND $<TARGET_FILE:protobuf::protoc> | ||
ARGS | ||
--grpc_out "${CMAKE_CURRENT_BINARY_DIR}/proto" | ||
--cpp_out "${CMAKE_CURRENT_BINARY_DIR}/proto" | ||
-I ${CMAKE_CURRENT_SOURCE_DIR}/proto | ||
--plugin=protoc-gen-grpc=$<TARGET_FILE:gRPC::grpc_cpp_plugin> | ||
${CMAKE_CURRENT_SOURCE_DIR}/proto/ninshiki.proto | ||
DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/proto/ninshiki.proto | ||
) | ||
|
||
add_library(ninshiki_proto SHARED | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.pb.cc" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.pb.h" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.grpc.pb.cc" | ||
"${CMAKE_CURRENT_BINARY_DIR}/proto/ninshiki.grpc.pb.h" | ||
) | ||
|
||
target_compile_options(ninshiki_proto PRIVATE -fPIC) | ||
target_compile_features(ninshiki_proto PUBLIC cxx_std_17) | ||
|
||
target_link_libraries(ninshiki_proto PUBLIC | ||
protobuf::libprotobuf | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to simplify this flow by creating a function and placing it in an independent gRPC utility package. This way, we could simply do this to generate a Proto library from a Proto file:
find_package(YourGrpcUtilityPackage)
add_proto_library(ninshiki_proto proto/ninshiki.proto)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's refactor these on another PR then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Jira Link:
https://ichiro-its.atlassian.net/browse/PD-405?atlOrigin=eyJpIjoiZjcyMmNiZGY0YjQ5NDliYzkxYjhkZjBiODFmODk2OWUiLCJwIjoiaiJ9
Description
Added protobuf for grpc
Type of Change
How Has This Been Tested?
Checklist:
feature/JIRA-ID-SHORT-DESCRIPTION
if has a JIRA ticketenhancement/SHORT-DESCRIPTION
if has/has no JIRA ticket and contain enhancementhotfix/SHORT-DESCRIPTION
if the change doesn't need to be tested (urgent)