norlab-ulaval / libnabo

A fast K Nearest Neighbor library for low-dimensional spaces
http://norlab-ulaval.github.io/libnabo/
BSD 3-Clause "New" or "Revised" License
431 stars 142 forks source link

Support multiple build systems such as pure make, catkin, ament, etc #109

Open YoshuaNava opened 3 years ago

YoshuaNava commented 3 years ago

This PR introduces new cmake files that allow libpointmatcher to be built natively with ROS catkin, or to rather be built as it has been before with a pure cmake setup that works on Windows, Mac OS or Linux.

I created new cmake files with the config needed. With this new config it would also be possible to build and release a libnabo ROS package, if a build server sets the right options for ROS build.

Finally, I moved all the cmake files to a separate folder called cmake

All feedback is warmly welcome.

ethzasl-jenkins commented 3 years ago

Can one of the admins verify this patch?

pomerlef commented 3 years ago

add to whitelist

YoshuaNava commented 3 years ago

The pipeline failed. Do you know what could be the cause?

pomerlef commented 3 years ago
+ export PATH=/usr/texbin/:/usr/lib/ccache:/usr/local/bin/:/usr/local/opt/ccache/libexec/:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ pwd
+ SRC_DIR=/home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty
+ BUILD_DIR=/home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ rm -rf /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ mkdir -p /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ cd /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty

CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.5.1 or higher is required.  You are running version 2.8.12.2
YoshuaNava commented 3 years ago
+ export PATH=/usr/texbin/:/usr/lib/ccache:/usr/local/bin/:/usr/local/opt/ccache/libexec/:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
+ pwd
+ SRC_DIR=/home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty
+ BUILD_DIR=/home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ rm -rf /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ mkdir -p /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ cd /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty/build
+ cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo /home/jenkins/workspace/libnabo/compiler/clang/label/ubuntu-trusty

CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.5.1 or higher is required.  You are running version 2.8.12.2

Mmmm, I was planning on proposing an increase to the cmake version but the build server does not seem compatible. Do you know which OS runs in that infrastructure?

pomerlef commented 3 years ago

That's one of the reason I wanted to transfer the library on our side. I don't have control over the testing server, and I'm not sure who is in charge at ASL.

Here are the architectures: image

YoshuaNava commented 3 years ago

Ok, I reduced the target version to 2.8.12.

On a different note, knowing that the package is always built for trusty by default means that we can't really provide support for features that weren't available in 2014. Trusty ships with gcc 4.8.2, and C++14 received support on 5.2, according to this link

Is ASL actively using Trusty for development? Can some build platforms be switched on/off and others integrated to the CI?. (e.g. to support Ubuntu Focal)

YoshuaNava commented 3 years ago

@peci1 Can you also help us verify the changes? I have tested locally with catkin (linked devel and install space) and pure cmake builds, but I don't really use the second option as often.

PS: Sorry for the spam

peci1 commented 3 years ago

I have it on my stack ;)

YoshuaNava commented 3 years ago

@peci1 Have you had time to look at this PR?

stephanemagnenat commented 3 years ago

Just two notes:

peci1 commented 3 years ago

@YoshuaNava It's still on my todo list, but more higher-priority things sneaked in... I've finished some of them, so I'll try to have a look today.

@stephanemagnenat What older systems are you talking about? Ubuntu 14.04 had CMake 2.8.12. Debian 8 (Jessie) has CMake 3, Debian 7 (Wheezy) has CMake 2.8.9. I don't think staying in the pre-history is a good strategy :) And if somebody really needs to use a super-old system, he can always pull a local install of newer cmake.

stephanemagnenat commented 3 years ago

@peci1 out of experience we never know what kind of antique build system some strange industrial users might have for example. But it was also a question of principle: not to add additional constraints if there is no need (and therefore they do not bring value). That said, I'm not against changing the required CMake version, I just wanted to ask what was the need. If there is any reason, sure, let's do the change.

YoshuaNava commented 3 years ago

Honestly, the reason to increase the cmake version was because I locally use 3.5.1. I implemented and tested the changes with it. Once I noticed that it was not available to the server, I tried out 2.8.12, which I read is the one that ships with Ubuntu Trusty.

I'll test the changes with the cmake version on master to verify that they run without issues.

On a different note, this package is quite awesome. Is there some way in which in the future we could support newer cmake versions while staying communicated with users?

peci1 commented 3 years ago

@stephanemagnenat In my opinion, modern CMake starts with 2.8.12 (which adds target_compile_definitions, target_compile_options etc.). And the code requires C++11 and gcc 4.8.2 (according to CMakeLists) which started shipping with Trusty. MSVC is required in version 2015+. So sticking to versions in Trusty is a good way to determine what can you use. And as said earlier - building custom CMake (or just downloading the prebuilt-binaries) is not such a big issue.

peci1 commented 3 years ago

in the future we could support newer cmake versions while staying communicated with users?

If by "support" you mean "it runs on them", it shouldn't be such an issue, maybe a few CMake policies to be set, but CMake is largely backwards-compatible. If you mean "use modern CMake constructs on modern CMake", that's a completely different problem (and I wouldn't do that because it would make the cmake files more complex; of course, unless the modern feature doesn't bring in something that old CMake can't do).

peci1 commented 3 years ago

Actually, reading through this, I stumbled upon an important question - what's the idea behind building this package as a ROS package? Do you want to continue with libpointmatcher afterwards to give it the same ability and make it find libnabo as a ROS package? It's important to know the answer to this, because that defines what kind of CMake variables libnabo needs to set in the generated files.

YoshuaNava commented 3 years ago

Do you want to continue with libpointmatcher afterwards to give it the same ability and make it find libnabo as a ROS package?

Exactly. Having both packages with the option of being built by catkin has multiple advantages:

It's important to know the answer to this, because that defines what kind of CMake variables libnabo needs to set in the generated files.

If you have further feedback I would be glad to address it :+1:

YoshuaNava commented 3 years ago

@peci1 Thank you for the extensive review.

However, I noticed that you made some comments on the file cmake/PureCmakeBuild.cmake. This was the original cmake file of libnabo, which provides support for Linux, OSX and Windows builds. Apart from moving the source and header blobs, I didn't modify this file.

Would it be possible to address those comments in another PR? I can create it now, and initially target to this branch (so the diff is minimal). Once this branch is merged we can re-target the PR to master and merge when ready.

peci1 commented 3 years ago

Would it be possible to address those comments in another PR?

Absolutely.

peci1 commented 3 years ago

Also, there seem to be some things that need to be done to correctly install the python bindings in catkin. This looks like a good example: https://github.com/luator/boost_python_catkin_example .

stephanemagnenat commented 3 years ago

Another note about using libnabo without ROS: we are using libpointmatcher as part of a Unity plugin to process combined visual/LiDAR data on iPad Pro for doing Augmented Reality, and there is an ongoing discussion on how to reduce the dependencies of libpointmatcher to ease such building. Making libnabo depend on Catkin by default would not help for such use outside robotics in the field of embedded AR.

YoshuaNava commented 3 years ago

@stephanemagnenat I agree with your view. This PR would not make catkin or any other build system mandatory, only provide the necessary cmake lines for alternative build systems to succeed, and for their outcomes to fulfill the additional build system guidelines (e.g. if a user chooses pure cmake or catkin, have a nicely configured setup so that the build is usable, installable, and can be packaged).

Timple commented 2 years ago

colcon is the ROS2 build tool. It can build pure cmake. So no need to depend on ROS specific tooling.

As a bonus, colcon can also compile ros1 packages. So folks wanting this package in their workspace, simply switch to colcon as your buildtool.

Edit: #116 to remove buildtooling as dependency.