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

Enhancement/dependency #3

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

LimHyungTae
Copy link
Member

This pull request includes several changes to the CMake build system, Python bindings, and documentation. The most important changes focus on updating dependencies, improving the build process, and restructuring the Python bindings.

Build System Updates:

  • Updated the minimum required CMake version to 3.18 and project version to 1.1.0 in CMakeLists.txt.
  • Introduced a new option USE_SYSTEM_EIGEN3 to use a pre-installed Eigen library and added a custom function to handle external dependencies in CMakeLists.txt and cmake/DownloadExternal.cmake. [1] [2]
  • Replaced find_package(Eigen3) with find_external_dependency to fetch Eigen3 if not available in the system in CMakeLists.txt.

Python Bindings:

  • Removed the BUILD_PYTHON_BINDINGS option and related code from CMakeLists.txt, and updated the Python bindings build process to use pybind11 and scikit-build in python/CMakeLists.txt. [1] [2]
  • Added pyproject.toml for Python build system configuration, specifying dependencies and build backend.

Documentation:

  • Updated the README.md to reflect changes in the build process, including new installation instructions and updated CMake options. [1] [2]

These changes aim to modernize the build process, simplify dependency management, and improve the overall maintainability of the project.


Dear @jingnanshi , could you check the last commit? I wanted to tackle #2. Happy to discuss how to resolve that issue!

@jingnanshi
Copy link
Member

Thanks! @LimHyungTae for the last commit (#2) we can instead use the latest version of catch2 catchorg/Catch2#2178 to fix it. Let me do that real quick.

@jingnanshi
Copy link
Member

Okay @LimHyungTae I pushed a new commit to master with the latest Catch2 (v2 instead of v3 to keep compatibility).

Copy link
Member

@jingnanshi jingnanshi left a comment

Choose a reason for hiding this comment

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

Looks good! I just have two comments.

Copy link
Member

Choose a reason for hiding this comment

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

What is this patch file for?

tests/catch.hpp Outdated
@@ -10818,6 +10818,8 @@ PVOID FatalConditionHandler::exceptionHandlerHandle = nullptr;

#elif defined( CATCH_CONFIG_POSIX_SIGNALS )

#undef MINSIGSTKSZ
Copy link
Member

Choose a reason for hiding this comment

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

Remove this if after you test with the latest Catch2 the error disappears.

@LimHyungTae
Copy link
Member Author

Yay! Finally we become MINSIGSTKSZ issue-free (see 901501e)!

Regarding the patch, I just brought the patch from KISS-ICP, and they said it's to support BLAS and LAPACK options (see PRBonn/kiss-icp#329), but it might not be necessary.

In addition to those things,
a) I've thoroughly test this in Docker environments, and I've found that there are wrong things, so I fixed it.
b) As our future plan, I'd like to upload this one to the Pypi, but I've realized there are so many 'ROBIN' in Pypi already. Thus, I'd like to suggest chaing 'robin_py' to 'spark_robin' (I've felt that you don't like '_py' in the package name, right?)

Pybinding is really annoying, haha. I confess how it actually works in details, so there are some redundant parts.

@jingnanshi
Copy link
Member

Yay! Finally we become MINSIGSTKSZ issue-free (see 901501e)!

Regarding the patch, I just brought the patch from KISS-ICP, and they said it's to support BLAS and LAPACK options (see PRBonn/kiss-icp#329), but it might not be necessary.

In addition to those things, a) I've thoroughly test this in Docker environments, and I've found that there are wrong things, so I fixed it. b) As our future plan, I'd like to upload this one to the Pypi, but I've realized there are so many 'ROBIN' in Pypi already. Thus, I'd like to suggest chaing 'robin_py' to 'spark_robin' (I've felt that you don't like '_py' in the package name, right?)

Pybinding is really annoying, haha. I confess how it actually works in details, so there are some redundant parts.

spark_robin sounds great! Thanks for the note on the patch & testing it in docker.

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.

2 participants