Add pb_cc_library in generic.cmake #2653

reyoung · 2017-06-28T13:50:36Z

	pb_cc_library	pb_py_library
Description	generate a C++ protobuf sources and headers, create a cc_library, add to include dirs	generate Python protobuf sources to target directory(default is CMAKE_CURRENT_BINARY_DIR)
Example	`pb_cc_library(paddle_proto SRCS ${proto_filenames})`	`pb_py_library(gen_proto_py SRCS ${proto_filenames} TARGET_DIR ${CMAKE_CURRENT_SOURCE_DIR})`

Fix PaddlePaddle#2567

wangkuiyi

Thanks for this PR!

wangkuiyi · 2017-06-28T15:11:39Z

cmake/generic.cmake

@@ -331,3 +331,52 @@ function(go_test TARGET_NAME)
  add_custom_target(${TARGET_NAME} ALL DEPENDS ${TARGET_NAME}_timestamp ${go_test_DEPS})
  add_test(${TARGET_NAME} ${CMAKE_CURRENT_BINARY_DIR}/${TARGET_NAME})
 endfunction(go_test)
+
+function(pb_cc_library TARGET_NAME)


Thanks for this PR! I do NOT think we need rules that compile .proto files into a specific programming language; instead, we need just a proto_library, similar to Bazel's https://bazel.build/blog/2017/02/27/protocol-buffers.html, to

convert .proto into .h and .cc using the standard function protobuf_generate_cpp, and

build a static library from the generated C++ source files by calling cc_library.

I have a draft of proto_library at
https://github.com/PaddlePaddle/Paddle/pull/2569/files#diff-7a3600c73304487a6d5f6bd3fa3fd662R175:

+function(proto_library TARGET_NAME) + if (NOT ${PROTOBUF_FOUND}) + error("Using proto_library but CMake cannot find installed protobuf.") + endif() + + set(options STATIC static SHARED shared) + set(oneValueArgs "") + set(multiValueArgs SRCS DEPS) + cmake_parse_arguments(proto_library "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN}) + + protobuf_generate_cpp(${TARGET_NAME}_PROTO_SRCS ${TARGET_NAME}_PROTO_HDRS ${proto_library_SRCS}) + cc_library(${TARGET_NAME} SRCS ${${TARGET_NAME}_PROTO_SRCS} DEPS ${${proto_library}_DEPS} protobuf) +endfunction(proto_library)

we cannot use protobuf_generate_cpp directly because we are manually compiling Protobuf instead of using Find(Protobuf) in our system. If we do not use Find(Protobuf), the protobuf_generate_cpp is not defined.

I think we can. We just need to set the corresponding variable so that protobuf_generate_cpp knows where protoc is. In my PR I tried this technique; it works.

I have tested the use of protobuf_generate_cpp in #2582 and found out the standard function prptobuf_generate_cpp is not good enough to use because of two reasons:

Function protobuf_generate_cpp is defined in cmake's Protobuf.cmake. In the case that find_package(Protobuf) is not called, the build process will be aborted because of Unknown CMake command "protobuf_generate_cpp" error.

In protobuf_generate_cpp, we cannot set the dependency of others, and the function may be called before the protoc executable in third_party generated. The error information is:

No rule to make target `../third_party/install/protobuf/bin/protoc', needed by `paddle/framework/example.pb.cc'. Stop.

@Xreki

We could use Find_PACKAGE(Protobuf QUIET) to import function protobuf_generate_cpp.

We could define ${Protobuf_PROTC_EXECUTABLE}, let protobuf_generate_cpp handle the dependencies.

@Xreki See here

@reyoung I decided not to call find_package(Protobuf), because when we do cross-compiling, it may find the protobuf library of the host. That must be avoided.

Oh, I see you set PROTOBUF_FOUND to OFF after calling find_package(Protobuf QUIET). It can avoid the problem I listed.

wangkuiyi · 2017-06-28T15:13:44Z

cmake/generic.cmake

+    list(APPEND proto_srcs "${CMAKE_CURRENT_BINARY_DIR}/${FIL_WE}.pb.cc")
+    list(APPEND proto_hdrs "${CMAKE_CURRENT_BINARY_DIR}/${FIL_WE}.pb.h")
+
+    add_custom_command(


If we are to generate source files from .proto files, we can call protobuf_generate_cpp, which is much more convenient than calling protoc from a customized command.

wangkuiyi · 2017-06-28T15:18:37Z

cmake/generic.cmake

+  cc_library(${TARGET_NAME} SRCS ${proto_srcs})
+endfunction()
+
+function(pb_py_library TARGET_NAME)


I think our Python code will call C++ code via the C-API, and only C++ code fills in and serializes protobuf messages. Given that Python code doesn't work with protobuf messages directly, we don't need a pb_py_library, at least at the current time.

If we need it in the future, we can add it, by please keep naming consistent with Bazel, as we have always been doing in generic.cmake.

FYR: Bazel has the following rules in https://github.com/pubref/rules_protobuf#rules

Language Compile ¹ Build ² gRPC ³

C++ cc_proto_compile cc_proto_library v1.0.1

C# csharp_proto_compile csharp_proto_library 1.0.0

Closure closure_proto_compile closure_proto_library

Go go_proto_compile go_proto_library v1.0.5

Go (gogo) gogo_proto_compile gogo_proto_library fb8a35

gRPC gateway grpc_gateway_proto_compile
grpc_gateway_swagger_compile grpc_gateway_proto_library
grpc_gateway_binary v1.0.4

Java java_proto_compile java_proto_library v1.0.1

Node node_proto_compile node_proto_library 1.0.0

Objective-C objc_proto_compile objc_proto_library ⁴ v1.0.0^673f

Python py_proto_compile v1.0.0^673f

Ruby ruby_proto_compile v1.0.0^673f

Custom proto_language proto_compile

wangkuiyi

I think this is the proto_library we need!

wangkuiyi · 2017-06-29T05:40:01Z

cmake/generic.cmake

+  set(proto_srcs)
+  set(proto_hdrs)
+  protobuf_generate_cpp(proto_srcs proto_hdrs ${proto_library_SRCS})
+  include_directories(${CMAKE_CURRENT_BINARY_DIR})


I'd thought that we should moved include_directories(${CMAKE_CURRENT_BINARY_DIR}) out from this function to the top of generic.cmake?

wangkuiyi · 2017-06-29T05:40:39Z

cmake/generic.cmake

+  set(proto_hdrs)
+  protobuf_generate_cpp(proto_srcs proto_hdrs ${proto_library_SRCS})
+  include_directories(${CMAKE_CURRENT_BINARY_DIR})
+  cc_library(${TARGET_NAME} SRCS ${proto_srcs})


I think cc_library needs to depend on the external target protobuf?

wangkuiyi · 2017-06-29T05:48:50Z

TeamCity complains that

[22:40:05]	In file included from /paddle/paddle/cuda/include/hl_gpu.h:28:0,
[22:40:05]	from /paddle/paddle/utils/Util.h:34,
[22:40:05]	from /paddle/paddle/testing/TestMain.cpp:16:
[22:40:05]	/paddle/paddle/cuda/include/hl_warpctc_wrap.h:18:17: fatal error: ctc.h: No such file or directory
[22:40:05]	compilation terminated.
[22:40:05]	In file included from /paddle/paddle/cuda/include/hl_gpu.h:28:0,
[22:40:05]	from /paddle/paddle/utils/Util.h:34,
[22:40:05]	from /paddle/paddle/utils/ThreadLocal.h:25,
[22:40:05]	from /paddle/paddle/math/Matrix.h:22,
[22:40:05]	from /paddle/paddle/testing/TestUtil.h:18,
[22:40:05]	from /paddle/paddle/testing/TestUtil.cpp:15:
[22:40:05]	/paddle/paddle/cuda/include/hl_warpctc_wrap.h:18:17: fatal error: ctc.h: No such file or directory
[22:40:05]	compilation terminated.

Xreki · 2017-06-29T05:49:10Z

cmake/generic.cmake

+  set(proto_srcs)
+  set(proto_hdrs)
+  protobuf_generate_cpp(proto_srcs proto_hdrs ${proto_library_SRCS})
+  include_directories(${CMAKE_CURRENT_BINARY_DIR})


I have tested this PR, and it works fine to build the example_proto through a simple command:

proto_library(example_proto SRCS example.proto)

No undefined problem and no dependency problem! Great work!
Just remind here that what kind of including path we want. If we want to use #include "paddle/framework/example.pb.h", we may need to change CMAKE_CURRENT_BINARY_DIR to CMAKE_BINARY_DIR.

Add pb_cc_library in generic.cmake

b1a311c

Fix PaddlePaddle#2567

reyoung requested review from wangkuiyi and gangliao June 28, 2017 13:50

Also add pb_py_library

30b75a5

reyoung force-pushed the feature/protobuf_lib branch 3 times, most recently from bfa57a0 to 6991da8 Compare June 28, 2017 15:02

Fix TravisCI

64b78b1

reyoung force-pushed the feature/protobuf_lib branch from 6991da8 to 64b78b1 Compare June 28, 2017 15:03

wangkuiyi requested changes Jun 28, 2017

View reviewed changes

reyoung added 4 commits June 29, 2017 12:11

Remove Python protobuf function

456f9cc

Use protobuf_generate_cpp

32d6587

Rename pb_cc_library -> proto_library

b88ca54

Fix TravisCI

4a4ec31

reyoung force-pushed the feature/protobuf_lib branch from 38c3bde to 4a4ec31 Compare June 29, 2017 05:06

wangkuiyi approved these changes Jun 29, 2017

View reviewed changes

Xreki reviewed Jun 29, 2017

View reviewed changes

reyoung added 4 commits June 29, 2017 14:14

Follow comments

2d3c186

Follow yiqun's comments

3d44fd5

Fix CI tests

52efb24

Fix TravisCI

a1e7284

reyoung merged commit c8e56d3 into PaddlePaddle:develop Jun 29, 2017

reyoung deleted the feature/protobuf_lib branch July 13, 2017 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pb_cc_library in generic.cmake #2653

Add pb_cc_library in generic.cmake #2653

reyoung commented Jun 28, 2017 •

edited

Loading

wangkuiyi left a comment

wangkuiyi Jun 28, 2017

reyoung Jun 29, 2017

wangkuiyi Jun 29, 2017

Xreki Jun 29, 2017 •

edited

Loading

reyoung Jun 29, 2017

reyoung Jun 29, 2017

Xreki Jun 29, 2017

Xreki Jun 29, 2017

wangkuiyi Jun 28, 2017

wangkuiyi Jun 28, 2017

wangkuiyi left a comment

wangkuiyi Jun 29, 2017

wangkuiyi Jun 29, 2017

wangkuiyi commented Jun 29, 2017

Xreki Jun 29, 2017

reyoung Jun 29, 2017

Language	Compile ¹	Build ²	gRPC ³
C++	cc_proto_compile	cc_proto_library	v1.0.1
C#	csharp_proto_compile	csharp_proto_library	1.0.0
Closure	closure_proto_compile	closure_proto_library
Go	go_proto_compile	go_proto_library	v1.0.5
Go (gogo)	gogo_proto_compile	gogo_proto_library	fb8a35
gRPC gateway	grpc_gateway_proto_compile grpc_gateway_swagger_compile	grpc_gateway_proto_library grpc_gateway_binary	v1.0.4
Java	java_proto_compile	java_proto_library	v1.0.1
Node	node_proto_compile	node_proto_library	1.0.0
Objective-C	objc_proto_compile	objc_proto_library ⁴	v1.0.0^673f
Python	py_proto_compile		v1.0.0^673f
Ruby	ruby_proto_compile		v1.0.0^673f
Custom proto_language	proto_compile

Add pb_cc_library in generic.cmake #2653

Add pb_cc_library in generic.cmake #2653

Conversation

reyoung commented Jun 28, 2017 • edited Loading

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xreki Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyoung commented Jun 28, 2017 •

edited

Loading

Xreki Jun 29, 2017 •

edited

Loading