support cmake compiling #227
Conversation
commit 2ee4027 Author: zengda <happy_zengda@126.com> Date: Tue Jan 31 13:23:35 2017 +0800 1,Revert the proto import back to the original files so scons will not fail to build the project 2,add a macro to build protobuf files commit 7c52112 Author: zengda <happy_zengda@126.com> Date: Mon Jan 30 22:19:55 2017 +0800 add test runner compile command commit 16d889a Author: zengda <happy_zengda@126.com> Date: Mon Jan 30 20:10:50 2017 +0800 add example add storage tool commit c379fd4 Author: zengda <happy_zengda@126.com> Date: Mon Jan 30 19:34:13 2017 +0800 add server control and client lib commit 193cbfe Author: zengda <happy_zengda@126.com> Date: Mon Jan 30 12:44:06 2017 +0800 first commit to cmake
CMakeLists.txt
Outdated
| ${TREE_PROTO_SRC} | ||
| ) | ||
|
|
||
| add_custom_command( |
There was a problem hiding this comment.
Would file(MAKE_DIRECTORY [<directories>...]) work here? Same with the other mkdir -p below (line 234).
CMakeLists.txt
Outdated
| ) | ||
| add_custom_command( | ||
| OUTPUT gtest/src/gtest-all.cc | ||
| COMMAND git submodule update --init |
There was a problem hiding this comment.
please add --recursive here (in case of nested submodules in the future)
CMakeLists.txt
Outdated
| foreach(FLAG ${FLAG_LIST}) | ||
| set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLAG}" ) | ||
| endforeach() | ||
| # For some unknown reason, this suppresses some definitions |
There was a problem hiding this comment.
This comment was attached to -DSWIG before; better to place the comment on -DSWIG and/or mention -DSWIG in the text.
| -Wno-used-but-marked-unused | ||
| -Wno-vla | ||
| -Wno-vla-extension | ||
| -Wno-weak-vtables |
There was a problem hiding this comment.
looks like -Weverything got lost during the port. I don't recall why it's on a separate env.Prepend() call in Sconsfile.
| COMMAND scripts/cpplint.py | ||
| ) | ||
| add_custom_target(lint | ||
| COMMAND scripts/cpplint.py |
There was a problem hiding this comment.
Please update the regex on line 3143 of cpplint.py to exclude the CMakeFiles directory.
CMakeLists.txt
Outdated
| COMMAND doxygen docs/Doxyfile | ||
| ) | ||
| add_custom_target(tags | ||
| COMMAND ctags -R --exclude=build --exclude=docs . |
There was a problem hiding this comment.
Also exclude CMakeFiles here, please.
| COMMAND scripts/cpplint.py | ||
| ) | ||
| add_custom_target(docs | ||
| COMMAND doxygen docs/Doxyfile |
There was a problem hiding this comment.
docs/Doxyfile EXCLUDE (around line 800) should exclude CMakeFiles directory too.
.gitignore
Outdated
| CMakeFiles/ | ||
| CMakeCache.txt | ||
| Makefile | ||
| *.cmake |
There was a problem hiding this comment.
For consistency, please prefix all of these with / and sort them above alphabetically.
Also, if *.cmake is just for cmake_install.cmake, please just expand that out here. If there are other possible *.cmake files, then disregard this.
|
Hey @johnzeng, thanks for doing the heavy lifting here! It's looking pretty good already. A few more thoughts:
logcabind might be better to match the install name.
I can't tell quite how bad this is. We should time clean and incremental scons vs cmake builds and double-check that compile times haven't gotten much worse. The protobuf warnings are indeed painful. You might want to prioritize that just because it'll make it easier to see what's going on without all the clutter. You might not be aware of |
|
Thanks for your comment ,I do know that there is a |
Fix doxy ignore Fix ctag ignore Fix mkdir command Fix flag comment Fix missing flags for clang
2, add lib checking 3, some code move
|
I've made some improvement.
about cmake object filesIt's a default behaviour that
And then linking these libs into the executable files instead of inputing about compiler version detectingIn And this will only affect the gcc's Hopes you know this inconvinence. |
There was a problem hiding this comment.
Great work, @johnzeng.
I'm encountering errors with parallel builds (make -j):
build/Protocol/RaftLogMetadata.proto: File not found.
build/Storage/SimpleFileLog.proto: Import "build/Protocol/RaftLogMetadata.proto" was not found or had errors.
build/Storage/SimpleFileLog.proto:26:14: "Protocol.RaftLogMetadata.Metadata" is not defined.
Perhaps we need to copy all the proto files over into build/ before any are compiled. Is that difficult?
CMakeLists.txt
Outdated
| elseif("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") | ||
| message("using gcc") | ||
|
|
||
| ## detect gcc version to decide wich c std should we use |
There was a problem hiding this comment.
There was a problem hiding this comment.
I am using 2.8.7 version of cmake and this var is not available on this version. On ubuntu12.04, apt-get will install v2.8.7 by default
CMakeLists.txt
Outdated
| set(PATH_VAR_NAME "${LIB_NAME}_PATH") | ||
| find_library(${PATH_VAR_NAME} NAMES ${LIB_NAME}) | ||
| if("${${PATH_VAR_NAME}}" STREQUAL "${PATH_VAR_NAME}-NOTFOUND") | ||
| ## sending warrning alows user to compile the lib without unnecessary dependency |
CMakeLists.txt
Outdated
| check_lib_exist(cryptopp) | ||
|
|
||
| # collect all libs when they all exist | ||
| SET(LIBRERYS pthread protobuf rt cryptopp) |
There was a problem hiding this comment.
LIBRERYS -> LIBRARIES or LIBS? This one took me a minute to decipher :)
There was a problem hiding this comment.
LIBRARYS is closer but still not correct. The plural of library is libraries. Programmers often shorten it to libs, so pick one of those two please.
CMakeLists.txt
Outdated
| set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/build) | ||
|
|
||
| if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang") | ||
| message("using clang") |
There was a problem hiding this comment.
I think you can safely get rid of this message now, since it's shown in the default cmake output anyway. Same with "using gcc".
|
Compile times are looking good. Here's some rough times from my dual-core 2012 laptop:
From running the following script (keep this outside your git clone or it'll delete itself): #!/bin/bash
set -e
# ProtoBuf files to copy to build/ first as workaround for cmake parallel build
# issue.
PROTO="
Core/ProtoBufTest.proto
Protocol/Client.proto
Protocol/Raft.proto
Protocol/RaftLogMetadata.proto
Protocol/ServerControl.proto
Protocol/ServerStats.proto
Server/SnapshotMetadata.proto
Server/SnapshotStateMachine.proto
Server/SnapshotStats.proto
Storage/SegmentedLog.proto
Storage/SimpleFileLog.proto
Tree/Snapshot.proto
"
INCRFILE=Storage/Log.h
build() {
case $BUILD in
"cmake")
for FILE in $PROTO; do
mkdir -p $(dirname build/$FILE)
cp $FILE build/$FILE
done
time sh -c "cmake . && make -j$CPUS" > /dev/null
;;
"scons")
time scons NUMCPUS=$CPUS > /dev/null
;;
esac
}
for RUN in {1..3}; do
for CPUS in 1 5; do
for BUILD in cmake scons; do
echo "---"
echo "Running clean build with $BUILD on $CPUS CPUs"
git clean -fdx > /dev/null
git checkout $INCRFILE
build
echo "---"
echo "Running incremental build with $BUILD on $CPUS CPUs"
echo >> $INCRFILE
build
done
done
doneThe full output was: |
|
I think we should merge this once you've taken care of those minor nits, then address the remaining to-do items in subsequent PRs. Sound good? |
|
It's ok to copy proto files at the begin. Will fix it soon |
copy all proto files at the beginning of building remvoe useless debug message
|
Please clean up the |
|
Thanks. I tried a couple of |
|
BTW the Ninja build is broken for me: I don't know how much we care (I'm perfectly OK with not supporting Ninja), except that this may be symptom of having incorrectly defined dependencies. That's with cmake 3.0.2 and ninja 1.3.4. |
|
Fixed the spelling error. As for ninja build, I haven't really used it yet, but it looks like the dependency is wrong, |
|
@ongardie Ninja works for me, with cmake 3.6.3 and 1.3.4. @johnzeng I found a problem, this version of CMakeList.txt only works in the root direcotry of the project, cause of copying of the .proto files. If I build the project in a subdirectory, I got errors. It's expected to work. |
|
Well, 7ecba30 didn't quite fix Ninja for me: Maybe @tigerzhang's newer cmake version makes a difference. |
|
I did get a successful diff --git a/CMakeLists.txt b/CMakeLists.txt
index 2d5391d..4b7de53 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -174,6 +174,7 @@ set(SERVER_PROTO_FOR_STORAGE_TOOL_SRC
set(SERVER_PROTO_SRC
build/Server/SnapshotStats.pb.cc
${SERVER_PROTO_FOR_STORAGE_TOOL_SRC}
+ ${PROTOCOL_SRC}
)
set(STORAGE_PROTO
@@ -184,6 +185,7 @@ set(STORAGE_PROTO
set(STORAGE_PROTO_SRC
build/Storage/SegmentedLog.pb.cc
build/Storage/SimpleFileLog.pb.cc
+ ${PROTOCOL_SRC}
)
set(TREE_PROTO
@@ -381,7 +383,6 @@ add_library(ServerSrcObject STATIC
${SERVER_SRC}
${SERVER_PROTO_SRC}
)
-add_dependencies(ServerSrcObject ${PROTOCOL_SRC})
add_library(StorageSrcObject STATIC
${STORAGE_PROTO_SRC}But I don't know how much wasted compiler effort this might be causing. |
|
@ongardie CMake shipped a protobuf module. That module should work well and efficiently. And I guess I know why @johnzeng was trying to write an ad-hoc protobuf compiling command for logcabin. The shipped protobuf module doesn't work with our protobuf importing manner, ex: import "build//.proto" I modified all these imports and removed the leading "build/" in my forked repo. But there are a couple of modifications needed for .h/.cc files, for header file including. I agree we can go with @johnzeng's version at this stage. Nothing changed, just a new CMakeLists.txt file added. |
|
@ongardie Including these files into Another thing is about copying @tigerzhang For |
|
In your pull request description you gave some bad advice to do an in-tree build: It is much better to do an out-of-tree build so you do not dirty up the source tree with build files, and to use separate output directories for each build type. This makes building exactly what you want easier to do. You can also use cmake to run the build in a platform-independent manner that will work on all platforms: Note that on Windows, setting CMAKE_BUILD_TYPE when running cmake doesn't do anything, as you select the build type when building; however on Linux you set the build type while running cmake. |
|
It would be advisable, before merging this, to:
|
| -Weverything | ||
| -Wno-unreachable-code ## clang 3.4 is knon to emit warnings without -Wno-unreachable-code, I will move this out when I have idea to get compiler version | ||
| ) | ||
| foreach(FLAG in FLAG_LIST) |
| ${SERVER_SRC} | ||
| ${SERVER_PROTO_SRC} | ||
| ) | ||
| add_dependencies(ServerSrcObject ${PROTOCOL_SRC}) |
There was a problem hiding this comment.
This doesn't work with cmake 3.1x plus clang++ 6.0, as StorageSrcObject will be built first. Add ${PROTOCOL_SRC} to above add_library solves the issue.
| ${STORAGE_PROTO_SRC} | ||
| ${STORAGE_SRC} | ||
| ) | ||
| add_dependencies(StorageSrcObject ${PROTOCOL_SRC}) |
This is like a pre-staging PR, I am not so sure about your code style, so please do some review and let me know what needs to be changed.
My dev env:
compile command
Supported:
gtestSomething changed
build/test/test->build/TestRunnerthe daemon exec file is changedbuild/LogCabin->build/LogCabinServerbuildcmakegenerates the.ofiles intoCMakeFiles/${executable_name}.dir/${src_dir}.oso it will do some extra compiling for each executable files.TODO
currently, I am using-std=c++0xto compile I will add support to-std=c++11in the future.lib detectingSeparate the flags forprotocgenerated files ,gtestfiles andlogcabinproject files. You will see tons of warning when compiling thoseprotocgenerated files now..travis.ymlLet me know if I still have something missed.