-
Notifications
You must be signed in to change notification settings - Fork 67
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 #636
Conversation
Signed-off-by: Silvio Traversaro <[email protected]>
Use CMAKE_INSTALL_LIBDIR from GNUInstallDirs instead of GZ_LIB_INSTALL_DIR, which won't be available if only building python bindings. Signed-off-by: Steve Peters <[email protected]>
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.
Do you mind to add some instructions in the source compilation ?
Refer to https://brew.sh instead of duplicating the brew installation command. Signed-off-by: Steve Peters <[email protected]>
* Describe Pybind11 as a dependency * Document how to build bindings separately from the main gz-math library Signed-off-by: Steve Peters <[email protected]>
I added some in 54978b9; let me know if there is more that I should add |
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.
No problems found, just some comments to avoid duplicating code. Great work!
# 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.16) | ||
set(GZ_MATH_VER 8) |
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'm afraid about keeping this number in sync with bumps. As a workaround we can put a comment in the main CMakeLists.txt
pointing to here. In the mid-term, it would be nice to integrate our CMake code to grab the version/name from git directly, probably using something like https://github.com/LecrisUT/CMakeExtraUtils/blob/main/cmake/DynamicVersion.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.
I am not a big fan of using git for version information as fails for tarballs. Something a bit ugly but effective could be to grep the major version from the parent folder CMakeLists.txt .
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.
or we could extract the version numbers from package.xml, since we also have a workflow to keep those versions in sync
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.
prototype for extracting version numbers from package.xml: #639
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've also opened gazebosim/gz-cmake#456 adding the python script from #639 and a cmake helper function. If you don't mind approving this PR for now so we can start to fix CI, I will follow-up by using the gz-cmake helper once it has been merged and released
src/python_pybind11/CMakeLists.txt
Outdated
cmake_minimum_required(VERSION 3.16) | ||
set(GZ_MATH_VER 8) | ||
project(gz-math${GZ_MATH_VER}-python VERSION ${GZ_MATH_VER}) | ||
find_package(Python3 COMPONENTS Interpreter Development 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.
It would be nice if we can move the find_package
logic from the main CMakeLists.txt
to here so we don't have a duplicate logic.
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've moved the find_package(pybind11)
call here in 06b8ab8 using the CMAKE_REQUIRE_FIND_PACKAGE_pybind11
variable to support finding it as an optional or required package depending on the context.
It's tricky to move the find_package(Python3)
call to this folder because the Development
component can be optional or required, depending on the context, but the FindPython3
module doesn't support CMAKE_REQUIRE_FIND_PACKAGE_*
for its components. So I just left the find_package(Python3)
code as is
Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
Moves find_package(pybind11) call to src/python_pybind11 folder. When invoked through the root CMakeLists.txt, it treats pybind11 as an optional dependency, but when invoked from that folder, it treats it as required by setting CMAKE_REQUIRE_FIND_PACKAGE_pybind11 to TRUE. Signed-off-by: Steve Peters <[email protected]>
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.
LGTM!
https://github.com/Mergifyio backport gz-math7 |
✅ Backports have been created
|
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
🎉 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.