-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
added missing *.pb.h *.pb.cc generation to fix distribute build issue #9415
Conversation
@@ -597,6 +597,9 @@ function(grpc_library TARGET_NAME) | |||
COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} | |||
ARGS --grpc_out "${CMAKE_CURRENT_BINARY_DIR}" -I "${PROTO_PATH}" | |||
--plugin=protoc-gen-grpc="${GRPC_CPP_PLUGIN}" "${ABS_PROTO}" | |||
COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} |
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.
line 590 already calls protobuf_generate_cpp(grpc_proto_srcs grpc_proto_hdrs "${ABS_PROTO}")
, curious why that doesn't work, and shoud line 590 be removed?
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.
hmm, i didn't notice this line, but this looks weird to me too. let me test if it's ok to remove the line 590.
also, I think it's good practice to put the generation command together, what's your idea?
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.
the build failed due to the missing of a dependency
FATALPlease specify source file or library in cc_library.
[18:58:57] -- Configuring done
[18:58:57] CMake Error at cmake/generic.cmake:206 (add_dependencies):
[18:58:57] The dependency target "sendrecvop_grpc_proto" of target "sendrecvop_grpc"
[18:58:57] does not exist.
[18:58:57] Call Stack (most recent call first):
[18:58:57] cmake/generic.cmake:616 (cc_library)
[18:58:57] paddle/fluid/operators/detail/CMakeLists.txt:2 (grpc_library)
looks we still need to add line 590 back for now
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.
If this just works, plz add a note here and merge this, we really need this unittest.
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
… fix-sendrec-ci
… fix-sendrec-ci
fix: #9309
looks grpc's cpp generation is missing the *.pb.h and *.pb.cc part. adding this back in this pr
related build error msg:
https://paddleci.ngrok.io/viewLog.html?buildId=31241&tab=buildLog&buildTypeId=Paddle_DistributedUnitTestNightly&logTab=tree&filter=all&_focus=7594