-
Notifications
You must be signed in to change notification settings - Fork 68
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
Permit building python bindings separately from gz-math library (backport #636) #640
Conversation
This allows the src/python_pybind11/CMakeLists.txt file to be built as a top-level cmake project against an external gz-math library, with documentation added to the installation tutorial. The logic for finding pybind11 is also moved from the root CMakeLists.txt to src/python_pybind11/CMakeLists.txt to reduce code duplication. When invoked through the root CMakeLists.txt, pybind11 is treated as an optional dependency, but when invoked from the src/python_pybind11 folder, pybind11 is treated it as required by setting CMAKE_REQUIRE_FIND_PACKAGE_pybind11 to TRUE. Signed-off-by: Silvio Traversaro <[email protected]> Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Silvio Traversaro <[email protected]> (cherry picked from commit 17deea2) # Conflicts: # CMakeLists.txt # src/python_pybind11/CMakeLists.txt
Cherry-pick of 17deea2 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
Signed-off-by: Steve Peters <[email protected]>
I've resolved the conflicts in a48b893 |
to unbreak CI while this awaits review, I've opened osrf/homebrew-simulation#2840 to apply these patches to the gz-math7 formula |
if(${CMAKE_VERSION} VERSION_LESS "3.12.0") | ||
# Detect if we are doing a standalone build of the bindings, using an external gz-math | ||
if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) | ||
cmake_minimum_required(VERSION 3.22.1) |
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.
The top level CMakeLists.txt file has a minimum version of 3.10.2. Is this necessary? If we require 3.22.1 to be able to run this, we should document the requirement in installation.md
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.
the CMAKE_REQUIRE_FIND_PACKAGE_pybind11
variable on line 10 requires cmake 3.22, so I used the same version as Ionic to simplify merges
I will update the documentation to indicate the required cmake version
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.
Signed-off-by: Steve Peters <[email protected]>
🎉 New feature
Part of osrf/homebrew-simulation#2834
Summary
This allows the
src/python_pybind11/CMakeLists.txt
file to be built as a top-level cmake project against an external gz-math library. The first commit (646de60) is a patch from @traversaro used in conda, and the second commit (d2bae80) replaces theGZ_LIB_INSTALL_DIR
cmake variable withCMAKE_INSTALL_LIBDIR
(see GzPackaging.cmake:111) sinceGZ_LIB_INSTALL_DIR
is not defined with the minimal cmake project added in 646de60.Test it
CMakeLists.txt
with using-DSKIP_PYBIND11=ON
to build and install the gz-math library without python bindings.src/python_pybind11/CMakeLists.txt
with-DPython_EXECUTABLE=/path/to/python
to build and install python bindings for a given python versionI have a draft of an updated homebrew formula using this branch in osrf/homebrew-simulation#2836
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.This is an automatic backport of pull request #636 done by [Mergify](https://mergify.com).