search

artivis / manif

A small C++11 header-only library for Lie theory.
https://artivis.github.io/manif
MIT License
1.51k stars 246 forks source link

Support specifying C++ standard from CMake command line when compiling Python bindings #274

Closed traversaro closed 1 year ago

traversaro commented 1 year ago

Hello @artivis,

with @GiulioRomualdi we debugged a python crash in bipedal-locomotion-framework bindings when blf and manif are compiled with -march=native.

It turned out that the crash went away if manifpy was compiled with the same C++ standard version used in blf python bindings, i.e. C++17 (this is probably related to the fact that in Eigen -std=c++11 + -march=native and -std=c++17 + -march=native result in different ABIs, even if we did not investigated in depth).

However, long story short, we would like to be able to compile manifpy with C++17 in our setup, by passing the appropriate CMake options. However, at the moment this is not possible as the choice of C++11 is hardcoded in manif's CMakeLists (see https://github.com/artivis/manif/blob/bc1e90f85cd1ee5a0349bdb8b2e0a6e67af63ac2/python/CMakeLists.txt#L17), so the CMAKE_CXX_FLAGS option that the user can specify is ignored, and C++11 is used even if the compiler default C++ standard is different (for example in GCC11 case, is C++17, see https://gcc.gnu.org/projects/cxx-status.html).

I would be happy to fix this, however given that pybind11 and its PYBIND11_CPP_STANDARD option is involved, it is important to understand which is the minimum version of pybind11 that you want to support in manif, as that influences the kind of CMake code we need to write.

So, TL;DR: What is the minimum version of pybind11 you want to support in manif? Thanks a lot in advance! It would be great if we could assume a minimum version of pybind11 2.6, so that we could simply drop the use of the PYBIND11_CPP_STANDARD variable (see https://pybind11.readthedocs.io/en/stable/upgrade.html#v2-6).

In the Continuous Integration the minimum version of pybind11 used is 2.9, but perhaps you may want to support older version for any specific reason.

artivis commented 1 year ago

Hi @traversaro, You're absolutely right and this should be fixed. Would you be so kind as to open a PR with a default standard if none is set? Concerning the pybind11 version, given that 18.04 just went EOL it is reasonable to target 20.04 which seems to ship pybind 2.4.3. Thanks.

traversaro commented 1 year ago

Great! If we want to target 20.04, would you be open to also raise the minimum version of cmake to 3.10 (Ubuntu 18.04) or 3.16 (Ubuntu 20.04)? That would simplify the CMake boilerplate (see also the comment in https://github.com/artivis/manif/blob/bc1e90f85cd1ee5a0349bdb8b2e0a6e67af63ac2/CMakeLists.txt#L78).