Backport #1367 to Garden: Fix find Python3 logic and macOS workflow #1370
Conversation
* Find Python3 with find_package instead of GzPython, adapting the approach from gz-sim. * macos workflow: use brew --prefix, not /usr/local * macos workflow: set Python3_EXECUTABLE cmake var Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## sdf13 #1370 +/- ##
=======================================
Coverage 87.58% 87.58%
=======================================
Files 128 128
Lines 17095 17095
=======================================
Hits 14972 14972
Misses 2123 2123 ☔ View full report in Codecov by Sentry. |
| # Find python | ||
| if (SKIP_PYBIND11) | ||
| message(STATUS "SKIP_PYBIND11 set - disabling python bindings") | ||
| find_package(Python3 COMPONENTS Interpreter) |
There was a problem hiding this comment.
It looks like we are looking for the Interpreter unconditionally. Both cases of SKIP_PYBIND11 look for ir. I see that is being used for embedSdf.py, if it is mandatory could we move it out of the if block to the main CMakeLists.txt course.
If that is valid, if can probably integrate this block with the add_directory(python) a bit below in this file to avoid extra checks in different locations.
There was a problem hiding this comment.
Without the changes in this PR, the include(GzPython) line will unconditionally search for the Python interpreter, and then we would separately search for the Development component. I think that can be problematic on systems with multiple versions of python that don't all have the Development component, since it could first find an Interpreter from a non-Development version of Python, and then find a different version of python with the Development component.
So my goal here is to unconditionally search for Python3 one time with the full set of components that are needed, either INTERPRETER or INTERPRETER DEVELOPMENT. Does that make sense?
There was a problem hiding this comment.
It makes total sense yes and actually the CMake documentation recommends to do it in an atomic call. Reducing the call to just one outside of the if block and moving the block to merge the add_directory(python) would result in:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index fa983cee..dd7bf5d2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -78,29 +78,6 @@ if (BUILD_SDF)
# available during build time
set(GZ_TOOLS_VER 2)
- #################################################
- # Find python
- if (SKIP_PYBIND11)
- message(STATUS "SKIP_PYBIND11 set - disabling python bindings")
- find_package(Python3 COMPONENTS Interpreter)
- else()
- find_package(Python3 COMPONENTS Interpreter Development
- )
- if (NOT Python3_Development_FOUND)
- GZ_BUILD_WARNING("Python development libraries are missing: Python interfaces are disabled.")
- else()
- set(PYBIND11_PYTHON_VERSION 3)
- find_package(pybind11 2.4 CONFIG QUIET)
-
- if (pybind11_FOUND)
- message (STATUS "Searching for pybind11 - found version ${pybind11_VERSION}.")
- else()
- GZ_BUILD_WARNING("pybind11 is missing: Python interfaces are disabled.")
- message (STATUS "Searching for pybind11 - not found.")
- endif()
- endif()
- endif()
-
#################################################
# Copied from catkin/cmake/empy.cmake
function(find_python_module module)
@@ -159,8 +136,27 @@ if (BUILD_SDF)
add_subdirectory(sdf)
add_subdirectory(conf)
add_subdirectory(doc)
- if (pybind11_FOUND AND NOT SKIP_PYBIND11)
- add_subdirectory(python)
+
+ #################################################
+ # Find python
+ find_package(Python3 COMPONENTS Interpreter Development)
+ if (SKIP_PYBIND11)
+ message(STATUS "SKIP_PYBIND11 set - disabling python bindings")
+ else()
+ if (NOT Python3_Development_FOUND)
+ GZ_BUILD_WARNING("Python development libraries are missing: Python interfaces are disabled.")
+ else()
+ set(PYBIND11_PYTHON_VERSION 3)
+ find_package(pybind11 2.4 CONFIG QUIET)
+
+ if (pybind11_FOUND)
+ message (STATUS "Searching for pybind11 - found version ${pybind11_VERSION}.")
+ add_subdirectory(python)
+ else()
+ GZ_BUILD_WARNING("pybind11 is missing: Python interfaces are disabled.")
+ message (STATUS "Searching for pybind11 - not found.")
+ endif()
+ endif()
endif()
endif(BUILD_SDF)whatever you prefer, the atomic call seems to be the most critical point here.
There was a problem hiding this comment.
to avoid deviation from newer branches, I'm going to keep this as is
| GZ_BUILD_WARNING("Python development libraries are missing: Python interfaces are disabled.") | ||
| else() | ||
| set(PYBIND11_PYTHON_VERSION 3) | ||
| find_package(pybind11 2.4 CONFIG QUIET) |
There was a problem hiding this comment.
Dobule checking: do we prefer to call it with QUITE and add custom STATUS messages below instead of removing the QUIET and the messages?
There was a problem hiding this comment.
it changes the formatting of the console messages, and I think it's subjective; I'm flexible on this
There was a problem hiding this comment.
up to you, no strong feeling either.
There was a problem hiding this comment.
since this is a backport; I'm going to leave this as is to match the behavior in newer branches
Signed-off-by: Steve Peters <[email protected]>
Backport
Backport #1367 to Garden. The patch required small modifications since Python3 is required in
sdf14and later but is not required in Garden.Original commit message:
Fix find Python3 logic and macOS workflow
Note to maintainers: Remember to use Rebase-and-Merge.