Always suppose hpp-fcl Python binding are present when hpp-fcl C++ library is found

stack-of-tasks / pinocchio

A fast and flexible implementation of Rigid Body Dynamics algorithms and their analytical derivatives

http://stack-of-tasks.github.io/pinocchio/

BSD 2-Clause "Simplified" License

1.82k stars 383 forks source link

Always suppose hpp-fcl Python binding are present when hpp-fcl C++ library is found #2288

Closed jorisv closed 3 months ago

jorisv commented 3 months ago

Pinocchio Python binding needs hpp-fcl Python binding for collision/contact related algorithms.

We allow with the BUILD_WITH_HPP_FCL_PYTHON_BINDINGS CMake variable to build Pinocchio C++ library with hpp-fcl but to ignore hpp-fcl in the Python bindings if the hpp-fcl Python bindings are not installed.

This create the following issues:

To compute BUILD_WITH_HPP_FCL_PYTHON_BINDINGS we must call the Python interpreter. This create some issues when cross compiling.
We never test this case in our CI, so we don't know if this option is working.

This PR remove this option to simplify the build. In the case an user build Pinocchio Python binding with hpp-fcl C++ library but without hpp-fcl binding, the Python interpreter will raise an ImportError.

ManifoldFR commented 3 months ago

This seems sensible to me, noting the issues we've had over at the conda-forge feedstock with cross-building: https://github.com/conda-forge/pinocchio-feedstock/pull/117/checks?check_run_id=26326030013.

We had a similar problem with getting proxsuite-nlp over on aarch64 and ppc due to the same check happening during crossbuilding.

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

jorisv commented 3 months ago

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

I don't think we want to reintroduce this in the future. The build issues in conda-forge show this option was broken. We already have too much options in Pinocchio, and it's really hard to test all the combination.

I also think this kind of issue should be managed by a package manager instead of CMake.

ManifoldFR commented 3 months ago

It would be perhaps better to check if the hpp-fcl Python bindings' CMake target exists, if we want to reintroduce a check like this in the future.

I don't think we want to reintroduce this in the future. The build issues in conda-forge show this option was broken. We already have too much options in Pinocchio, and it's really hard to test all the combination.

I also think this kind of issue should be managed by a package manager instead of CMake.

Totally makes sense! I agree the combinatorics of all the options was too much.