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

Fix numpy include dirs #137

Closed
wants to merge 1 commit into from
Closed

Conversation

Tobias-Fischer
Copy link

@Tobias-Fischer Tobias-Fischer commented Jul 20, 2021

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
Author

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

Signed-off-by: Tobias Fischer <info@tobiasfischer.info>
@sloretz
Copy link
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

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
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.

<buildtool_export_depend>python_cmake_module</buildtool_export_depend>

@Tobias-Fischer
Copy link
Author

That sounds like a good idea to me @sloretz!

@clalancette
Copy link
Contributor

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants