Skip to content

Permit building python bindings separately from gz-math library (backport #636)#640

Merged
scpeters merged 3 commits intogz-math7from
mergify/bp/gz-math7/pr-636
Nov 1, 2024
Merged

Permit building python bindings separately from gz-math library (backport #636)#640
scpeters merged 3 commits intogz-math7from
mergify/bp/gz-math7/pr-636

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify bot commented Oct 29, 2024

🎉 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 the GZ_LIB_INSTALL_DIR cmake variable with CMAKE_INSTALL_LIBDIR (see GzPackaging.cmake:111) since GZ_LIB_INSTALL_DIR is not defined with the minimal cmake project added in 646de60.

Test it

  1. Invoke cmake on the root CMakeLists.txt with using -DSKIP_PYBIND11=ON to build and install the gz-math library without python bindings.
  2. Invoke cmake on src/python_pybind11/CMakeLists.txt with -DPython_EXECUTABLE=/path/to/python to build and install python bindings for a given python version
  3. Repeat 2 with for each desired version of python

I have a draft of an updated homebrew formula using this branch in osrf/homebrew-simulation#2836

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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).

@scpeters @mergify
Permit building python bindings separately from gz-math library (#636)
97ad436 
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 <silvio.traversaro@iit.it>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Silvio Traversaro <silvio.traversaro@iit.it>
(cherry picked from commit 17deea2)

# Conflicts:
#	CMakeLists.txt
#	src/python_pybind11/CMakeLists.txt
@mergify mergify bot requested a review from scpeters as a code owner October 29, 2024 07:14
@mergify
Copy link
Copy Markdown
Contributor Author

mergify bot commented Oct 29, 2024

Cherry-pick of 17deea2 has failed:

On branch mergify/bp/gz-math7/pr-636
Your branch is up to date with 'origin/gz-math7'.

You are currently cherry-picking commit 17deea2b.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   src/CMakeLists.txt
	modified:   tutorials/installation.md

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   CMakeLists.txt
	both modified:   src/python_pybind11/CMakeLists.txt

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

@mergify mergify bot requested a review from adityapande-1995 as a code owner October 29, 2024 07:14
@mergify mergify bot added the conflicts label Oct 29, 2024
@mergify mergify bot mentioned this pull request Oct 29, 2024
Merged
9 tasks
@github-actions github-actions bot added 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic labels Oct 29, 2024
@scpeters
Adapt package finding logic
a48b893 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 29, 2024

Cherry-pick of 17deea2 has failed:

On branch mergify/bp/gz-math7/pr-636
Your branch is up to date with 'origin/gz-math7'.

You are currently cherry-picking commit 17deea2b.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   src/CMakeLists.txt
	modified:   tutorials/installation.md

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   CMakeLists.txt
	both modified:   src/python_pybind11/CMakeLists.txt

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

I've resolved the conflicts in a48b893

@scpeters scpeters mentioned this pull request Oct 31, 2024
Merged
@scpeters
Copy link
Copy Markdown
Member

scpeters commented Oct 31, 2024

to unbreak CI while this awaits review, I've opened osrf/homebrew-simulation#2840 to apply these patches to the gz-math7 formula

@scpeters scpeters mentioned this pull request Oct 31, 2024
Closed
32 tasks
azeey
azeey reviewed Oct 31, 2024
View reviewed changes
src/python_pybind11/CMakeLists.txt
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)
Copy link
Copy Markdown
Contributor

@azeey azeey Oct 31, 2024

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

Copy link
Copy Markdown
Member

@scpeters scpeters Oct 31, 2024

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

Copy link
Copy Markdown
Member

@scpeters scpeters Nov 1, 2024

Choose a reason for hiding this comment

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

2448824

@scpeters
installation.md: mention cmake required version
2448824 
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters merged commit 255f118 into gz-math7 Nov 1, 2024
@scpeters scpeters deleted the mergify/bp/gz-math7/pr-636 branch November 1, 2024 19:06
@traversaro traversaro mentioned this pull request Nov 16, 2024
Merged
3 tasks
@azeey azeey mentioned this pull request Feb 10, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azeey azeey azeey approved these changes

@scpeters scpeters Awaiting requested review from scpeters scpeters is a code owner

@adityapande-1995 adityapande-1995 Awaiting requested review from adityapande-1995 adityapande-1995 is a code owner

Assignees

No one assigned

Labels

conflicts 🌱 garden Ignition Garden 🎵 harmonic Gazebo Harmonic

Projects

Core development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@scpeters @azeey