Use FindPython3 explicitly instead of FindPythonInterp implicitly #345
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
|
CI (build:
|
| @@ -16,6 +16,8 @@ include(CheckLibraryExists) | |||
| find_package(ament_cmake_python REQUIRED) | |||
| find_package(ament_cmake_ros REQUIRED) | |||
|
|
|||
| find_package(Python3 REQUIRED COMPONENTS Interpreter) | |||
There was a problem hiding this comment.
So this is great that we are making this an explicit dependency in CMakeLists.txt.
I wonder if we should also make python3 an explicit build_depend/buildtool_depend in the package.xml. It will always implicitly get pulled in because of the buildtool_depend on python3-empy, but it is a little weird to find_package something that doesn't have an entry in the package.xml. Thoughts?
There was a problem hiding this comment.
I think it's reasonable to add python3 as the buildtool_depend. This is getting that for free from ament_cmake, so adding it to the package.xml fits the theme of being explicit. It seems unlikely to have an impact in the future as I imagine ament_cmake will always depend on Python.
it is a little weird to find_package something that doesn't have an entry in the package.xml
The FindPython3 module is provided by CMake, so this is find_packaging() is a module from CMake itself.
There was a problem hiding this comment.
OK, that's a fair point. In that case, I don't feel strongly about whether we have a package.xml depends on python or not. I'm going to go ahead and approve this, and let you decide whether to add the depends.
There was a problem hiding this comment.
I'll leave the package.xml as is for expediency.
Rcutils implicitly depends on FindPythonInterp being called by
ament_cmake_core. I noticed it will fail to build while testing a branch to useFindPython3inament_cmake. This PR makes the python interpreter dependency explicit, and usesFindPython3instead ofFindPythonInterpRelated to ros2/python_cmake_module#6
Blocks ament/ament_cmake#355