Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

traversaro
Copy link

@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 <[email protected]>
Copy link
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
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