Skip to content

Remove SHARED from pybind11_add_module (take two) to avoid segfaults on macOS with Python statically linked with libpython#1418

Closed
traversaro wants to merge 1 commit intoros2:rollingfrom
traversaro:patch-1
Closed

Remove SHARED from pybind11_add_module (take two) to avoid segfaults on macOS with Python statically linked with libpython#1418
traversaro wants to merge 1 commit intoros2:rollingfrom
traversaro:patch-1

Conversation

@traversaro
Copy link
Copy Markdown

@traversaro traversaro commented Feb 27, 2025

This PR rebases #1305 on top of the latest rolling. #1305 was blocked on some kind of CI failure on Windows, I want to understand if this is still the case or not.

To give a bit more of context, removing SHARED from pybind11_add_module ensures that the built extension is compiled as MODULE (default setting for pybind11_add_module) and that can be loaded fine (i.e. without segfaults) on macOS with Python versions that statically linked libpython. conda-forge is an example of distribution that provides a Python version that statically links libpython for performance reasons, and other distributions are also considering switching to statically linking libpython for performance reasons (see astral-sh/python-build-standalone#540 for example).

As already mentioned in #1305, there should be no downside in building _rclpy_pybind11 as MODULE, unless somewhere _rclpy_pybind11 is linked as a shared libraries via target_link_libraries instead of dlopen-ed by python, but I don't think there is any use case for which that make sense, and I could not find any such usage of _rclpy_pybind11 in public repos.

…S with Python statically linked with libpython

Signed-off-by: Silvio Traversaro <silvio@traversaro.it>
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

@traversaro thanks for the ping.

since #1305 is original PR, i just rebased that one, and restarted the CI.

@traversaro
Copy link
Copy Markdown
Author

@traversaro thanks for the ping.

since #1305 is original PR, i just rebased that one, and restarted the CI.

Perfect, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants