Skip to content

Fix numpy include dirs#137

Closed
Tobias-Fischer wants to merge 1 commit intoros2:rollingfrom
Tobias-Fischer:patch-1
Closed

Fix numpy include dirs#137
Tobias-Fischer wants to merge 1 commit intoros2:rollingfrom
Tobias-Fischer:patch-1

Conversation

@Tobias-Fischer
Copy link
Copy Markdown

@Tobias-Fischer Tobias-Fischer commented Jul 20, 2021 •
edited
Loading

This PR tackles two issues:

  1. Removing deprecated FindPythonInterp and replacing it with FindPython3. We have at least CMake 3.14 on all platforms, and it is Cmake 3.14 that introduced FindPython3.
  2. The previous logic only got the (correct) include path on Apple or Windows, but not Linux - on Linux, if the system numpy was found, it was used (which is undesirable in cases like RoboStack where the conda-shipped numpy must be used instead: Fix numpy include in rosidl_generator_py RoboStack/ros-galactic#23)

/cc @tfoote @wolfv @traversaro @ooeygui

@Tobias-Fischer
Copy link
Copy Markdown
Author

Tobias-Fischer commented Jul 20, 2021

Note that this patch is included in RoboStack and is working fine there.

@Tobias-Fischer
Tackle RoboStack/ros-galactic#23
3651d08 
Signed-off-by: Tobias Fischer <info@tobiasfischer.info>
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jul 21, 2021

The change looks reasonable except that I couldn't figure out if Python3_EXECUTABLE will map to the Debug python executable when appropriate. I kicked off a Windows Debug build to check:

CI (repos: https://gist.githubusercontent.com/sloretz/da9294ef618229c2fe1e6209c297bd78/raw/40dc6e2f23219197263434751c5eb20244b1def6/ros2.repos build type: Debug build: --packages-above-and-dependencies rosidl_generator_py test: --packages-above rosidl_generator_py)

  • Windows Debug Build Status

@wolfv
Copy link
Copy Markdown

wolfv commented Jul 22, 2021

hmm, it says:

01:44:23 -- Using all available rosidl_typesupport_c: rosidl_typesupport_fastrtps_c;rosidl_typesupport_introspection_c
01:44:23 CMake Error at C:/Program Files/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
01:44:23   Could NOT find Python3 (missing: Python3_NumPy_INCLUDE_DIRS NumPy) (found
01:44:23   version "3.8.3")
01:44:23 Call Stack (most recent call first):
01:44:23   C:/Program Files/CMake/share/cmake-3.20/Modules/FindPackageHandleStandardArgs.cmake:594 (_FPHSA_FAILURE_MESSAGE)
01:44:23   C:/Program Files/CMake/share/cmake-3.20/Modules/FindPython/Support.cmake:3165 (find_package_handle_standard_args)
01:44:23   C:/Program Files/CMake/share/cmake-3.20/Modules/FindPython3.cmake:485 (include)
01:44:23   cmake/rosidl_generator_py_generate_interfaces.cmake:20 (find_package)
01:44:23   C:/ci/ws/install/share/ament_cmake_core/cmake/core/ament_execute_extensions.cmake:48 (include)
01:44:23   C:/ci/ws/install/share/rosidl_cmake/cmake/rosidl_generate_interfaces.cmake:292 (ament_execute_extensions)
01:44:23   CMakeLists.txt:56 (rosidl_generate_interfaces)

It might be possible to set teh interpreter directly for teh debug case, with

https://cmake.org/cmake/help/git-stage/module/FindPython3.html#artifacts-specification

set(Python3_EXECUTABLE mypathtopython)

What do you think @sloretz ?

@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Jul 22, 2021

Yeah, setting the debug interpreter directly seems reasonable. There is logic for finding the Python debug executable in a find module provided by python_cmake_module. It looks like this package already depends on python_cmake_module, so I like the idea of replacing PythonInterp with Python3 in that find module and using the result here.

rosidl_python/rosidl_generator_py/package.xml

Line 21 in 17de859

<buildtool_export_depend>python_cmake_module</buildtool_export_depend>

@sloretz sloretz mentioned this pull request Jul 22, 2021
Merged
@Tobias-Fischer
Copy link
Copy Markdown
Author

Tobias-Fischer commented Jul 25, 2021

That sounds like a good idea to me @sloretz!

@sloretz sloretz mentioned this pull request Jul 26, 2021
Closed
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:24
@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Feb 11, 2024

I just merged in #140, which basically does this and a bunch more. So going ahead and closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@sloretz sloretz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Tobias-Fischer @sloretz @wolfv @clalancette