search

RoboStack / ros-galactic

Vinca build files for ROS 2 Galactic Geochelone
https://robostack.github.io
21 stars 14 forks source link

Fix numpy include in rosidl_generator_py #23

Closed Tobias-Fischer closed 3 years ago

Tobias-Fischer commented 3 years ago

Currently finds the system numpy

Tobias-Fischer commented 3 years ago

See https://github.com/Tobias-Fischer/rosidl_python/commit/1c353799377ae5e294226ef67ad877777bc4a9d6 for a potential solution. What do you think @wolfv @traversaro?

wolfv commented 3 years ago

I think @tfoote was also looking into this yesterday. Maybe he's got an opinion

traversaro commented 3 years ago

See Tobias-Fischer/rosidl_python@1c35379 for a potential solution. What do you think @wolfv @traversaro?

FindPythonInterp is deprecated at the CMake level, and FindPython3 is instead the recommended way to go, see https://cmake.org/cmake/help/v3.21/module/FindPythonInterp.html , so I fully agree!

Tobias-Fischer commented 3 years ago

I think the more critical fix is that currently the numpy path is only added on OSX and Win .. the change to FindPython3 is cosmetics on top ;)

Tobias-Fischer commented 3 years ago

Should have mentioned this earlier

traversaro commented 3 years ago

Ahh, that was a tricky one. In any case it make sense.

tfoote commented 3 years ago

I believe that this is the right fix. The additional path is the critical part, but the new API is much cleaner.

Jumping to the Python3 find logic is much cleaner. I'm confirming with @cottsay whether or not the minimum of 3.14 is ok to use(That's when the new api was added https://cmake.org/cmake/help/latest/module/FindPython3.html#result-variables )

As of Galactic only RHEL is a supported platform that doesn't have a new enough CMake Version: https://www.ros.org/reps/rep-2000.html#id38 to jump forward. And they may have a backports available or we can move forward.

tfoote commented 3 years ago

It looks like RHEL 8.4 is using CMake 3.18 so we should be able to push forward. @cottsay will be working to update the REP.

Edit: PR for the REP: https://github.com/ros-infrastructure/rep/pull/323

Tobias-Fischer commented 3 years ago

This seems to be working nicely now: -- Using numpy include directory: $PREFIX/lib/python3.8/site-packages/numpy/core/include