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

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

Closed
traversaro opened this issue Jul 5, 2023 · 2 comments · Fixed by #275
Closed

Comments

@traversaro
Copy link
Contributor

traversaro commented Jul 5, 2023

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

set_property(TARGET manifpy PROPERTY CXX_STANDARD 11)
), 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
Copy link
Owner

artivis commented Jul 5, 2023

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

traversaro commented Jul 5, 2023

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

# CMake 3.8 ...
).

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 a pull request may close this issue.

2 participants