-
Notifications
You must be signed in to change notification settings - Fork 280
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
Migrate away from PythonInterp CMake module #1195
base: noetic-devel
Are you sure you want to change the base?
Conversation
71c2816
to
93deae8
Compare
find_package(Python ${PYTHON_VERSION} REQUIRED) | ||
|
||
# Set these legacy variables for compatibility with what PythonInterp used to do | ||
set(PYTHON_EXECUTABLE ${Python_EXECUTABLE}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's safe to just migrate to the new variable names— the ROS 1 codebase is pretty dependent on PYTHON_EXECUTABLE
in particular existing, see for example the stuff around message generation:
- https://github.com/ros/genmsg/blob/noetic-devel/cmake/pkg-genmsg.cmake.em
- https://github.com/ros/dynamic_reconfigure/blob/noetic-devel/cmake/dynamic_reconfigure-macros.cmake
Amusingly, dynamic_reconfigure does actually call find_package(PythonInterp)
, but does so after PYTHON_EXECUTABLE
has already been used once already.
Doesn't this formally requires all packages to declare a minimum required cmake version of 3.12? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR I run into issues with pybind11: pybind/pybind11#5472
For this reason, I will revert the corresponding commit for ROS-O.
@@ -1,7 +1,13 @@ | |||
# the CMake variable PYTHON_INSTALL_DIR has the same value as the Python function catkin.builder.get_python_install_dir() | |||
|
|||
set(PYTHON_VERSION "$ENV{ROS_PYTHON_VERSION}" CACHE STRING "Specify specific Python version to use ('major.minor' or 'major')") | |||
find_package(PythonInterp ${PYTHON_VERSION} REQUIRED) | |||
find_package(Python ${PYTHON_VERSION} REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the docs, you should specify both components, Interpreter
and Development
:
find_package(Python ${PYTHON_VERSION} REQUIRED) | |
find_package(Python ${PYTHON_VERSION} REQUIRED COMPONENTS Interpreter Development) |
Without this change, every catkin package built spams this into the log:
The new policy for CMake 3.27 is: https://cmake.org/cmake/help/latest/policy/CMP0148.html
The replacement FindPython module has been available since CMake 3.12, released in 2.18: https://cmake.org/cmake/help/latest/module/FindPython.html
ROS Noetic's stated minimum target is the CMake on Debian Buster, which is 3.13.4: https://www.ros.org/reps/rep-0003.html#noetic-ninjemys-may-2020-may-2025