Skip to content
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

make kiss_icp c++ package installable via cmake #408

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Pyrestone
Copy link

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Collaborator

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

@Pyrestone
Copy link
Author

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
Copy link
Member

benemer commented Dec 13, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants