search

koide3 / small_gicp

Efficient and parallel algorithms for point cloud registration [C++, Python]
MIT License
364 stars 46 forks source link

Improve the project's CMake and Python structure to aid packaging #6

Closed valgur closed 5 months ago

valgur commented 5 months ago

Hi!

Thank you for sharing this great new library!

I thought this library would make for a good candidate for packaging as a Conan and/or Vcpkg package, which I would be happy to help with. But first, some minor tweaks could be made to the project's CMakeLists.txt to aid this process and avoid the need for patches on the package repo side.

This PR makes the following changes to the project's CMake setup:

I also replaced the setup.py with a pure pyproject.toml and scikit-build-core, which is allows for a much cleaner wrapping of the CMake build.

koide3 commented 5 months ago

Hi @valgur , Thank you so much for your proposal! I'm not familiar with packaging, and your help is very appreciated.

Renamed small_gicp_helper to just small_gicp. I assume the _helper suffix was there just to avoid the naming conflict with the Python module?

Yes, removing _helper is desired. All other changes look good to me.

If you think it is ready, please let me know. I'll test it on my PC and run CI before merging it.

valgur commented 5 months ago

@koide3 I made some further fixes to the exported small_gicp-config.cmake to ensure that the project can be used with a simple find_package(small_gicp CONFIG) after installation.

For packaging, it would not hurt to also move the vendored third-party library headers into a separate directory and make their use configurable. That way they could be easily substituted with full external depndencies at the package manager side. This is usually preferable as it allows the consumers to ensure that a consistent version of the dependency is used everywhere and that there are no accidental multiple definitions of the same symbols. But I think I'll make it a separate PR to keep things clearer.

The PR is ready to merge, as far as I'm concerned.

koide3 commented 5 months ago

For packaging, it would not hurt to also move the vendored third-party library headers into a separate directory and make their use configurable. That way they could be easily substituted with full external depndencies at the package manager side. This is usually preferable as it allows the consumers to ensure that a consistent version of the dependency is used everywhere and that there are no accidental multiple definitions of the same symbols. But I think I'll make it a separate PR to keep things clearer.

Good. Because the dependency on Sophus is only two small functions in lie.hpp, it would be good to consider moving only nanoflann.hpp (and its omp and tbb versions) to a thirdparty directory later.

koide3 commented 5 months ago

Because most pars of the PR are fine, I merge it and fix FindTBB on my side. Thanks for your help!

valgur commented 5 months ago

Oh., thanks for the merge. The exported CMake config still needed some work, though. I had some pending commits that I'll turn into a separate PR. Sorry for not getting back to you sooner.

koide3 commented 5 months ago

No problem. I wanted to modify CMakeLists and merged the PR before the modifications ruin your updates. I'm looking forward to seeing the coming PR. Thanks a lot for your help!

valgur commented 5 months ago

Sorry, I misremembered. The commit I had in mind was present after all and merged with this PR: https://github.com/koide3/small_gicp/pull/6/commits/a9ebba4f079161cc308565d83a1d7351f2a3dd7b