make kiss_icp c++ package installable via cmake

[Pyrestone]

Hi there!
Cool project!

I saw this was an open task in #330 a while ago, and since I wanted to use KISS-ICP in another project, I re-structured the C++ project so that it is able to be installed via CMake, and then found and loaded in other C++/CMake projects.

I also built a minimal working example on how to include KISS-ICP in another CMake project in the cpp/include_example directory. It's up to you if that's something you find valuable of keeping or not.

Unfortunately, I had to change the directory structure of the package slightly (using separate src/ and include/kiss_icp/ subdirectories to separate out the headers), and I'm not sure if that maybe breaks some ROS 2 components that rely on the C++ library paths. I wasn't yet able to check that, so please let me know if that breaks anything.

The python package still seems to build fine with make install, and the python/tests/test_kiss_icp.py test still works fine.

[Pyrestone]

@nachovizzo @benemer @tizianoGuadagnino I think I fixed the things that made the CI break in the last run. Can you please re-run them?
I applied the coding style, which should fix the style checker.
I also restored the old default behavior of fetching missing dependencies (which the CI pipelines for building packages rely on, because they don't seem to install any prior dependencies...).
(Exporting the cpp targets (sudo cmake --install ) still does rely on having the dependencies installed system-wide so dependent packages can find them, but I don't know how to fix that, or if that's even a desirable behavior.)

[Pyrestone]

oh... right! I added an error case if installing is not possible with non-system-wide dependencies.
I guess that should be a warning, since for most current users being able to build but not install the cpp lib is an acceptable end-state.
I also fixed the styling issue. Apparently running pre-commit run --all-files locally does not do the exact same thing as the CI version expects. Not sure what the problem is there. Something about versions probably... I think that check should be fixed now.
For the others, let's see what the CI says...

[tizianoGuadagnino]

@Pyrestone, we get a build error on cpp API on 20.04 ;(

[Pyrestone]

Damn, CMake really insists that the installation is missing dependencies, even if it's never going to be run 🤦‍♂️
I've toggled off the install() commands when at least one dependency is a fetched one. Maybe someone more proficient at CMake can patch in the export of fetched targets later, but I can't seem to figure it out right now.
Nevertheless, It should finally build now, and just not try to add install targets.

A CI test which actually checks that the lib can be built, exported and included when building cpp/include_example would probably be a good follow-up addition, since the added functionality is essentially switched off now in the current CI build process...

[benemer]

Hey @Pyrestone, first of all, thank you for your contribution!

Now the CI passes :) We need more time to review this PR, because we are currently busy with other parts of the project. Also, I know that @tizianoGuadagnino also wanted to look into this or even started working on it in #330, so I guess he is the one who can provide the first feedback.

[AlexandrePTJ]

Hi @benemer, any feedback on this PR ?
I would like to create a recipe for conan and this updates might greatly ease the integration.

[benemer]

I had a brief look and added a few comments. There are a few todos before we can merge this:

1. Resolve merge conflicts
2. Further testing with ROS 2
3. Additional reviews from at least @nachovizzo and @tizianoGuadagnino

Optional:
4. Add CI step to test installation

Unfortunately, I will not have much time soon to do this, so any help is appreciated here.

Thanks again for the PR and sorry for the delay!

[benemer]

Is there a reason to link this privately?

[Pyrestone]

Why make it public?
It's only used internally, and private linking avoids downstream libraries needing to find / include and depend on tbb as well.
IIRC At first I tried to make every dependency private but ended up with this combination since some of the other dependencies are included in the outward-facing APIs. But tbb is only used in internal utilities, so it's safe to not expect code using the kiss-icp library to need to know about tbb.

[benemer]

Why installing to share/cmake? Isn't lib/cmake more common?

[Pyrestone]

I think I just copied the way the dependencies (e.g. tsl::robin-map) did it.
I'm not a cmake expert, so this was just cargo cult convention and trial&error until cmake finds it. Once I'm back from vacation I can take a look if using lib instead of share works as well.

[benemer]

Same here, maybe change to lib

[benemer]

I think this file can be improved, because we currently have contradicting options. For example, I can set DOWNLOAD_MISSING_DEPS to ON and at the same time have, for example, USE_SYSTEM_EIGEN3 to ON as well.

Can we move the logic of checking whether all dependencies are system-wide installed by checking that all USE_SYSTEM_${DEPENDENCY} are set to ON? If they are supposed to be installed, but the target cannot be found, we can disable the installation. Or am I missing anything here?

The calls to find_external_dependency also contain a lot of boilerplate print messages, which I would remove, or at least move to the function.