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.

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

Add macros for generating pybind11 bindings #400

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Bi0T1N
Copy link
Contributor

@Bi0T1N Bi0T1N commented Dec 22, 2023

🎉 New feature

Closes #215

Summary

This adds CMake macros to remove code duplication for creating pybind11 bindings.

The only problem that occurs is that it destroys the used structure in CMakeLists.txt (e.g. for gz-math):

#============================================================================
# Set project-specific options
#============================================================================

option(SKIP_SWIG
      "Skip generating ruby bindings via Swig"
      OFF)


include(GzPython) <-- here we don't only set project-specific options - we search for Python
gz_add_pybind_project_settings()

The other two macros are fine:

########################################
# Python bindings
gz_search_for_pybind()

src/CMakeLists.txt:

# Bindings subdirectories
gz_add_pybind_directory(python_pybind11)

Test it

  1. Compile packages up to gz-math7 to /ws/install
  2. In gz-cmake project, run mkdir build && cd build && cmake .. -DCMAKE_INSTALL_PREFIX=/ws/install/
  3. Run make -j && make install
  4. Open a new terminal and change directory to gz-math
  5. Run source /ws/install/setup.bash, change CMakeLists.txt to use gz-cmake4 and the new macros
  6. Run mkdir build && cd build && cmake .. && make -j

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@azeey
Copy link
Contributor

azeey commented Jan 2, 2024

LGTM! but I'll hold off merging so @j-rivero can take a look since he commented on #215.

if (SKIP_PYBIND11)
message(STATUS "SKIP_PYBIND11 set - disabling python bindings")
else()
find_package(Python3 ${GZ_PYTHON_VERSION} QUIET COMPONENTS Interpreter Development)
Copy link
Contributor

Choose a reason for hiding this comment

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

gazebosim/gz-math#588 updated the code in gz-math to use OPTIONAL_COMPONENTS here.
https://github.com/gazebosim/gz-math/blob/ef2211c6292929ea6a8471815cf83315c1b891cb/CMakeLists.txt#L102-L104

Mind updating this to match?

cc @scpeters

Copy link
Member

Choose a reason for hiding this comment

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

I think I haven't been completely consistent with using COMPONENTS or OPTIONAL_COMPONENTS

Copy link
Member

Choose a reason for hiding this comment

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

ok, in gz-math8 and sdformat15, I used made finding the Python3 Interpreter REQUIRED and used OPTIONAL_COMPONENTS to find Development, while we didn't apply this logic in gz-transport or gz-sim, probably because I didn't realize the inconsistency

I think I do recommend the approach used in gz-math and sdformat, but I also recognize the lack of flexibility in finding packages from a macro. I would suggest calling find_package(Python3 ...) from within the macro if NOT Python3_Development_FOUND, which would allow a package to use their own call to find_package(Python3) but fall back to a good default if it hasn't been searched for yet

# USE_SYSTEM_PATHS_FOR_PYTHON_INSTALLATION Install modules in default system path if ON
# USE_DIST_PACKAGES_FOR_PYTHON Use dist-package over site-package for module installation if ON
#
macro(gz_add_pybind_project_settings)
Copy link
Member

Choose a reason for hiding this comment

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

apologies for not reviewing this sooner

I actually deprecated GzPython in #431; I think these new macros should be in a new file like GzPybind.cmake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants