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

YoshuaNava · 2020-10-26T14:26:34Z

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.

…ilds

ethzasl-jenkins · 2020-10-26T14:26:36Z

Can one of the admins verify this patch?

pomerlef · 2020-10-26T14:37:00Z

add to whitelist

YoshuaNava · 2020-10-26T15:22:18Z

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

pomerlef · 2020-10-26T17:03:46Z

+ 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 · 2020-10-26T22:52:52Z

+ 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 · 2020-10-27T00:20:14Z

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:

YoshuaNava · 2020-10-29T15:40:49Z

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 · 2020-10-29T15:42:38Z

@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 · 2020-10-29T15:56:00Z

I have it on my stack ;)

YoshuaNava · 2020-11-18T10:19:50Z

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

stephanemagnenat · 2020-11-18T10:34:13Z

Just two notes:

libnabo is also used outside the ROS context, for example as an R package. For that reason, I think that it is cleaner that the default build does not depend on ROS. Of course, having a Catkin build as an option is nice, maybe if it is easier, through a ROS-specific repository that uses this one as submodule, to keep the ability to build a ROS package that this MR adds.
This MR pushes the cmake_minimum_required from 2.6 to 2.8.12 which might harm users on older systems. Is there a reason for this push? Especially for the non-ROS build, is seems currently version 2.6 works.

peci1 · 2020-11-18T10:50:45Z

@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 · 2020-11-18T11:41:58Z

@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 · 2020-11-18T12:03:43Z

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

I've just gone through the PR on the web, did not (yet) try building it. I'll be back with more feedback about the files it generates and installs.

cmake/CatkinBuild.cmake

peci1 · 2020-11-18T12:45:59Z

package.xml

-	<build_depend>eigen</build_depend>
+	<!-- If a pure-cmake build is desired, the buildtool should be switched to 'cmake' -->
+	<buildtool_depend>catkin</buildtool_depend>
+
 	<build_depend>boost</build_depend>


If python bindings are not built, then remove it.

package.xml

peci1 · 2020-11-18T12:49:45Z

cmake/PureCMakeBuild.cmake

+install(FILES nabo/third_party/any.hpp DESTINATION include/nabo/third_party)
+install(FILES README.md DESTINATION share/doc/${PROJECT_NAME})
+if (DOXYGEN_FOUND)
+	install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/doc/html DESTINATION ${DOC_INSTALL_TARGET})


COMPONENT doc?

peci1 · 2020-11-18T12:51:29Z

cmake/PureCMakeBuild.cmake

+endif()
+
+option(LIBNABO_BUILD_PYTHON "Build libnabo python" ON)
+if(LIBNABO_BUILD_PYTHON)


What about specifying a separate python component?

peci1 · 2020-11-18T12:53:13Z

cmake/PureCMakeBuild.cmake

+	EXPORT ${PROJECT_NAME}-targets
+	DESTINATION share/${PROJECT_NAME}/cmake
+	NAMESPACE ${PROJECT_NAME}::
+)


COMPONENT dev

peci1 · 2020-11-18T13:01:04Z

@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 · 2020-11-18T13:03:49Z

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 · 2020-11-18T13:30:35Z

cmake/PureCMakeBuild.cmake

+	endif ()
+	if (OPENCL_INCLUDE_DIR AND OPENCL_LIBRARIES)
+		add_definitions(-DHAVE_OPENCL)
+		set(EXTRA_LIBS ${OPENCL_LIBRARIES} ${EXTRA_LIBS})


Shouldn't this be more like target_link_libraries?

peci1 · 2020-11-18T19:17:13Z

cmake/CatkinBuild.cmake

+# Find catkin macros and libraries
+find_package(catkin REQUIRED)
+find_package(Eigen3 REQUIRED)
+find_package(Boost REQUIRED COMPONENTS python)


Only when Python bindings are built?

Good point. I can add the Python bindings

Hmm, but isn't this find_package anyways done inside python/CMakeLists.txt? It might be superfluous here...

cmake/CatkinBuild.cmake

peci1 · 2020-11-18T23:06:06Z

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 · 2020-11-20T05:39:34Z

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:

Finding libnabo from libpointmatcher is straightforward.
Whenever you work with ROS, the overlay system would also work perfectly, which doesn't really happen with the current cmake builds. In the past I have had to uninstall versions of libnabo and libpointmatcher from my PC so that changes in the code are detected when building libpointmatcher.
It would make it easier to have a ROS build server and package distribution in place for both libpointmatcher and libnabo, in case the maintainers are interested.

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 👍

YoshuaNava · 2020-11-20T05:41:28Z

@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.

YoshuaNava · 2020-11-18T11:12:37Z

CMakeLists.txt

+#	* catkin.
+#	* cmake.
+#	* ament (future).
+set(BUILD_TYPE "cmake")


@stephanemagnenat Related to your comment, I would keep the default build type to cmake, so support for non-ROS users is kept as before.

cmake/CatkinBuild.cmake

YoshuaNava · 2020-11-20T05:17:50Z

cmake/CatkinBuild.cmake

+# Find catkin macros and libraries
+find_package(catkin REQUIRED)
+find_package(Eigen3 REQUIRED)
+find_package(Boost REQUIRED COMPONENTS python)


Good point. I can add the Python bindings

YoshuaNava · 2020-11-20T05:22:04Z

cmake/CatkinBuild.cmake

+find_package(Eigen3 REQUIRED)
+find_package(Boost REQUIRED COMPONENTS python)
+
+find_package(OpenMP REQUIRED)


I was unsure whether it was possible to forward options with catkin, so I made the default options available.

cmake/CatkinBuild.cmake

package.xml

cmake/CatkinBuild.cmake

YoshuaNava · 2020-11-20T06:28:20Z

CMakeLists.txt

+#	* catkin.
+#	* cmake.
+#	* ament (future).
+set(BUILD_TYPE "cmake")


Shouldn't this rather check a CMake variable and fill in cmake only in case it's unset or empty?

Can you give an example?

I suggest also testing if CATKIN_DEVEL_PREFIX cmake var is set, and automatically choose catkin build if it is. I've just tested that this variable is set at the very beginning of the build even before find_package(catkin). And, it is the only one that has this property. Tested both with catkin and catkin tools.

I thought about something similar, but I think it's better to just allow the user to be explicit about the build type. I also don't know if e.g. the ethz-asl CI is already packaging libnabo as a sort-of ROS package with pure cmake build.

peci1 · 2020-11-20T11:55:15Z

cmake/CatkinBuild.cmake

-
-target_link_libraries(nabo
+target_include_directories(nabo SYSTEM PRIVATE
+  ${EIGEN3_INCLUDE_DIR}


Eigen is a public dependency (it's in nabo.h). Move it above and add <build_export_depend>eigen</build_export_depend> to package.xml.

peci1 · 2020-11-20T11:58:43Z

cmake/CatkinBuild.cmake

-target_link_libraries(nabo
+target_include_directories(nabo SYSTEM PRIVATE
+  ${EIGEN3_INCLUDE_DIR}
+  ${Boost_INCLUDE_DIRS}


Boost isn't used in the core library, just for the python bindings. So remove it from here.

peci1 · 2020-11-20T11:59:05Z

cmake/CatkinBuild.cmake

+  ${Boost_INCLUDE_DIRS}
+  ${OpenMP_CXX_INCLUDE_DIRS}
+)
+target_link_libraries(nabo PRIVATE
  ${catkin_LIBRARIES}
  ${Boost_LIBRARIES}


And, remove it from here, too.

peci1 · 2020-11-20T12:27:05Z

cmake/CatkinBuild.cmake

+)
+
+# Python bindings
+add_subdirectory(python)


Respect LIBNABO_BUILD_PYTHON setting.

peci1 · 2020-11-20T12:29:40Z

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

Absolutely.

peci1 · 2020-11-20T12:44:48Z

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 · 2020-11-23T08:35:16Z

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 · 2020-11-23T09:30:44Z

@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 · 2021-12-09T12:41:13Z

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.

YoshuaNava added 4 commits October 26, 2020 15:17

[libpointmatcher] Added cmake files to support pure make or catkin bu…

f8fdd06

…ilds

[package.xml] Changed default buildtool

3cc0712

[CMakeLists.txt] File rename and move

89d1da7

[cmake] Added catkin cmake file

c9414bb

YoshuaNava added 2 commits October 26, 2020 15:31

[package.xml] Fixed depends

0b57425

[cmake] Default build tool set to catkin

641f177

Reduce target cmake version to 2.8.12

3177e0f

peci1 suggested changes Nov 18, 2020

View reviewed changes

peci1 reviewed Nov 18, 2020

View reviewed changes

cmake/CatkinBuild.cmake Outdated Show resolved Hide resolved

YoshuaNava commented Nov 20, 2020

View reviewed changes

YoshuaNava added 2 commits November 20, 2020 07:28

Address comments from pull request

4bbfee9

Package.xml update

be08549

peci1 reviewed Nov 20, 2020

View reviewed changes

cmake/CatkinBuild.cmake

)

# Python bindings

add_subdirectory(python)

Copy link

Contributor

peci1 Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Respect LIBNABO_BUILD_PYTHON setting.

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

Are you sure you want to change the base?

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

Conversation

YoshuaNava commented Oct 26, 2020 • edited

ethzasl-jenkins commented Oct 26, 2020

pomerlef commented Oct 26, 2020

YoshuaNava commented Oct 26, 2020

pomerlef commented Oct 26, 2020

YoshuaNava commented Oct 26, 2020

pomerlef commented Oct 27, 2020

YoshuaNava commented Oct 29, 2020 • edited

YoshuaNava commented Oct 29, 2020 • edited

peci1 commented Oct 29, 2020

YoshuaNava commented Nov 18, 2020

stephanemagnenat commented Nov 18, 2020 • edited

peci1 commented Nov 18, 2020

stephanemagnenat commented Nov 18, 2020 • edited

YoshuaNava commented Nov 18, 2020 • edited

peci1 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

Choose a reason for hiding this comment

peci1 commented Nov 18, 2020

peci1 commented Nov 18, 2020

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

peci1 commented Nov 18, 2020

YoshuaNava commented Nov 20, 2020

YoshuaNava commented Nov 20, 2020

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

Choose a reason for hiding this comment

peci1 commented Nov 20, 2020

peci1 commented Nov 20, 2020

stephanemagnenat commented Nov 23, 2020

YoshuaNava commented Nov 23, 2020

Timple commented Dec 9, 2021 • edited

YoshuaNava commented Oct 26, 2020 •

edited

YoshuaNava commented Oct 29, 2020 •

edited

YoshuaNava commented Oct 29, 2020 •

edited

stephanemagnenat commented Nov 18, 2020 •

edited

stephanemagnenat commented Nov 18, 2020 •

edited

YoshuaNava commented Nov 18, 2020 •

edited

Timple commented Dec 9, 2021 •

edited