-
Notifications
You must be signed in to change notification settings - Fork 157
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
wip: Add native cmake build support #15
Conversation
Update README for cmake build
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.
Hi @jwdinius ,
Thanks for this PR. I marked a few sections that we can try and still improve. In general I would try to make the dependency on gtest and threads optional, so that the user doesn't have to worry about that if not installed. Concerning the general structure, I will also look into some more modern cmake (>3.0.0) functions that might help to link the separate packages. In general I want to go a bit into the direction as done here (https://github.com/ethz-asl/kindr), with .cmake files and find scripts.
Anyway, this is good work and I'll start looking into this now as well. We have to still figure out how/if some of these issues can be solved elegantly, if I should merge this.
Best
Alex
MESSAGE(STATUS "CATKIN_BUILD is " ${CATKIN_BUILD}) | ||
|
||
if(${CATKIN_BUILD}) | ||
find_package(catkin REQUIRED) |
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.
This actually returns a variable CATKIN_FOUND that could be used instead of the CATKIN_BUILD, that way this flag does not have to be set by the user.
# do native cmake build | ||
# add gtest and gmock support | ||
# - see http://www.kaizou.org/2014/11/gtest-cmake/ | ||
# ------------- BEGIN FROM BLOG ------------------ |
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.
This whole section for unit testing with gtest seems a bit long, as well as there exist some native cmake functions that should handle that (https://cmake.org/cmake/help/v2.8.3/cmake.html#module:FindGTest). I would prefer to do something like they did here (create and download manually, cmake only checks if folder exists,...):
https://github.com/ethz-asl/kindr
They also nicely use .cmake and find scripts, which I think is a good guidline.
${PROJECT_NAME} | ||
pthread | ||
) | ||
endif() |
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 great if the code for cmake build wouldn't have to be repeated for catkin build. But i'm not sure how to handle this in the most flexible way.
@@ -1,20 +1,21 @@ | |||
cmake_minimum_required(VERSION 2.8.3) | |||
project(ifopt_core) | |||
option(CATKIN_BUILD "CATKIN_BUILD" ON) |
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.
This could be set from the return value of
find_package(catkin
QUIET)`. That way the user doesn't have to worry about it
enable_testing() | ||
|
||
# add subdirectories | ||
add_subdirectory("../ifopt_core" "../../ifopt_core/build") |
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'm not sure if we want to add the base and the "/build" repos here.
|
||
$ git clone https://github.com/ethz-adrl/ifopt.git | ||
$ mkdir build ; cd build | ||
$ cmake -DCATKIN_BUILD=OFF -DCMAKE_INSTALL_PREFIX=path_to_build_dir .. |
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.
Best case this is just cmake ..
and the system automatically detects that it should build without catkin and where the files should be installed. Possibly its even better to avoid system-wide installation and just keep all files local in the build
folder.
It would be nice not to have to rely upon catkin bindings to build ifopt. I have created a process to do just that. I have also updated the README to explain it.
The main thing is the addition of the
CATKIN_BUILD
option. By default,CATKIN_BUILD=ON
, meaning that cmake will try to build using catkin; it will look for all of the correct packages etc... The build process with catkin should be just the same as before, but someone should verify via CI or manual test.I was able to build and run using the instructions. Please let me know if you encounter any issues with this PR. Cheers!