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.

Sign up for GitHub

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

Backport #1367 to Garden: Fix find Python3 logic and macOS workflow #1370

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Backport #1367 to Garden: Fix find Python3 logic and macOS workflow #1370

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a9e70d
Fix find Python3 logic and macOS workflow (#1367)
scpeters Jan 31, 2024
8de28a1
macos.yml: use $(brew --prefix python3)
scpeters Feb 2, 2024
c934030
one-line find_package call
scpeters Feb 8, 2024
548d4fe
Merge branch 'sdf13' into scpeters/pick_fix_find_python
scpeters Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 23 additions & 26 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,30 @@ 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)
Copy link
Contributor

@j-rivero j-rivero Feb 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid deviation from newer branches, I'm going to keep this as is

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobule checking: do we prefer to call it with QUITE and add custom STATUS messages below instead of removing the QUIET and the messages?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it changes the formatting of the console messages, and I think it's subjective; I'm flexible on this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to you, no strong feeling either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is a backport; I'm going to leave this as is to match the behavior in newer branches


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
include(GzPython)
function(find_python_module module)
# cribbed from http://www.cmake.org/pipermail/cmake/2011-January/041666.html
string(TOUPPER ${module} module_upper)
Expand Down Expand Up @@ -130,30 +151,6 @@ if (BUILD_SDF)
gz_find_package(gz-utils2 REQUIRED COMPONENTS cli)
set(GZ_UTILS_VER ${gz-utils2_VERSION_MAJOR})

########################################
# Python interfaces
if (NOT PYTHON3_FOUND)
GZ_BUILD_WARNING("Python is missing: Python interfaces are disabled.")
message (STATUS "Searching for Python - not found.")
else()
message (STATUS "Searching for Python - found version ${Python3_VERSION}.")

if (SKIP_PYBIND11)
message(STATUS "SKIP_PYBIND11 set - disabling python bindings")
else()
set(PYBIND11_PYTHON_VERSION 3)
find_package(pybind11 2.4 QUIET)

if (${pybind11_FOUND})
find_package(Python3 ${GZ_PYTHON_VERSION} REQUIRED COMPONENTS Development)
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()

gz_configure_build(HIDE_SYMBOLS_BY_DEFAULT QUIT_IF_BUILD_ERRORS)

gz_create_packages()
Expand All @@ -162,7 +159,7 @@ if (BUILD_SDF)
add_subdirectory(conf)
add_subdirectory(doc)
if (pybind11_FOUND AND NOT SKIP_PYBIND11)
add_subdirectory(python)
add_subdirectory(python)
endif()
endif(BUILD_SDF)

Expand Down
Loading