-
Notifications
You must be signed in to change notification settings - Fork 549
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
Modernize cmake; make cmake compatible with git submoduling #382
Conversation
Can one of the admins verify this patch? |
ok to test |
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.
@jlblancoc I left some comments in your PR, mostly questions.
Have you also tested that the library builds with ROS? In merged and linked devel (--no-install and --install flags when using catkin build
)
I tried with catkin, and it failed due to failed to find "nabo/nabo.h"... But I'm not sure it's due to these changes. Perhaps it's assumed that the user should do a "sudo make install" after building "libnabo"? I will re-check this area, anyway, another day. |
Hi @jlblancoc, For the ones that require improvements that go beyond the scope of this PR, could you open issues? So we can actively keep track of them. Thank you, and have a nice weekend :) Yoshua |
All done: it was tested to build both, as a standalone project, and with "catkin build". Note that there's another related PR in the queue: norlab-ulaval/libnabo#103 |
Nice! Thank you. |
|
||
add_custom_target(uninstall | ||
COMMAND ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_uninstall.cmake) | ||
if (NOT TARGET uninstall) |
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.
Does this require a documentation update?
@jlblancoc Is this PR in a state ready for merging? |
I'm alive ;-)
Will come back soon, sorry...
|
Please, test and merge norlab-ulaval/libnabo#103 first so this PR makes sure of using the updated version of libnabo too. |
@jlblancoc so I guess we can continue on this PR. Could you update your branch? |
@pomerlef After the last commit, it builds cleanly locally... let's see what happens with Jenkins. |
ok to test |
the branch is still out-of-date. Here is the error log:
|
I just integrated @peci1 's fixes (thanks!) into this PR via a cherry-pick. The order of operations to get this sorted out is:
|
ok to test |
@jlblancoc could you bring your branch to master so I can merge? |
I will
|
rebase done. |
Builds nicely again - thanks @jlblancoc @pomerlef |
Not yet there...
We need to find a way for the generator expressions to actually get evaluated. |
See #407. |
This PR does:
GNUInstallDirs
to correctly install files in their standard directories (multiarch-compatible).install(EXPORT ...
to cmake scripts to allow better use of exported targets.libnabo
if built as part of the same source tree (e.g. git submodules).