Stop linking the Python bindings library into C++ consumers of interface packages (fixes dyld _PyExc_RuntimeError / ros-kilted#76) - #51
Conversation
a084fe4 to
2ed71d4
Compare
|
Macos failing with: After adding the tests (1st commit) |
73971ba to
7d891c3
Compare
|
All green after adding the patch (2nd commit) |
|
@traversaro I added a few tests related to RoboStack/ros-kilted#76, ros2/rosidl_python#253, and a longstanding issue I have encountered when developing ROS 2 on macOS. Let me first describe the macOS issue. So far I have been using ROS Humble, Lyrical, and Rolling on macOS for ROS 2 development, for example compiling Nav2 from source and working on personal projects. Across all of these distros, whenever I start a new project I eventually run into errors involving symbols such as My workaround has usually been to add something like this where needed: component_container_env = {}
if sys.platform == 'darwin':
python_library = f'libpython{sysconfig.get_python_version()}.dylib'
component_container_env['DYLD_INSERT_LIBRARIES'] = os.path.join(
sys.prefix, 'lib', python_library
)I believe something similar could also be added through the Pixi activation environment, but either way this feels more like a workaround than a good development experience. Based on my investigation, this problem does not normally show up in the RoboStack prebuilt binaries. The reason seems to be that those binaries are built in an environment where the linker drops unused libraries, so the Python bindings library does not remain as a runtime dependency. A more detailed explanation from my agent is:
After investigating this, I ended up with two possible fixes. The larger fix is the one currently proposed in this PR. It makes the generated conversion code part of the Python extension directly and performs cross-package converter lookup at runtime through the Python message classes. This also means everything uses Python3::Module. A smaller alternative would be: diff --git a/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake b/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake
index 2810984..f815eab 100644
--- a/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake
+++ b/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake
@@ -38,6 +38,11 @@ if(NOT TARGET Python3::Module OR NOT TARGET Python3::NumPy)
find_package(Python3 REQUIRED COMPONENTS Interpreter Development NumPy)
endif()
+# The Python C bindings library of an interface package is exported in
+# <pkg>_TARGETS__rosidl_generator_py instead of <pkg>_TARGETS, so that C/C++
+# consumers of the interface package don't link it.
+set(rosidl_generator_py_suffix "__rosidl_generator_py")
+
# Get a list of typesupport implementations from valid rmw implementations.
rosidl_generator_py_get_typesupports(_typesupport_impls)
@@ -165,10 +170,18 @@ add_dependencies(
${rosidl_generate_interfaces_TARGET}__rosidl_typesupport_c
)
+# On macOS, link Python3::Module (-undefined dynamic_lookup): linking libpython
+# would load a second interpreter into a python executable that links it
+# statically (e.g. conda-forge), which crashes.
+if(APPLE)
+ set(_python_target Python3::Module)
+else()
+ set(_python_target Python3::Python)
+endif()
target_link_libraries(
${_target_name_lib} PRIVATE
Python3::NumPy
- Python3::Python
+ ${_python_target}
)
target_include_directories(${_target_name_lib}
PRIVATE
@@ -260,9 +273,20 @@ if(NOT rosidl_generate_interfaces_SKIP_INSTALL)
LIBRARY DESTINATION lib
RUNTIME DESTINATION bin)
- # Export this target so downstream interface packages can depend on it
- rosidl_export_typesupport_targets("${rosidl_generator_py_suffix}" "${_target_name_lib}")
- ament_export_targets(export_${_target_name_lib})
+ # Export this target so downstream interface packages can depend on it.
+ # Not with ament_export_targets(), which would add it to <pkg>_TARGETS.
+ install(
+ EXPORT export_${_target_name_lib}
+ DESTINATION share/${PROJECT_NAME}/cmake
+ NAMESPACE "${PROJECT_NAME}::"
+ FILE "export_${_target_name_lib}Export.cmake")
+ set(_py_extras_file
+ "${CMAKE_CURRENT_BINARY_DIR}/rosidl_generator_py/${_target_name_lib}-extras.cmake")
+ file(WRITE "${_py_extras_file}"
+ "include(\"\${${PROJECT_NAME}_DIR}/export_${_target_name_lib}Export.cmake\")\n"
+ "list(APPEND ${PROJECT_NAME}_TARGETS${rosidl_generator_py_suffix}\n"
+ " \"${PROJECT_NAME}::${_target_name_lib}\")\n")
+ list(APPEND ${PROJECT_NAME}_CONFIG_EXTRAS "${_py_extras_file}")
endif()
if(BUILD_TESTING AND rosidl_generate_interfaces_ADD_LINTER_TESTS)
I tested this smaller version as well, and it appears to work. The main difference is:
Since I do not have much experience with Empy and the code-generation side of rosidl_generator_py, I would appreciate it if you could take a look before I spend more time validating them. In particular, I would be interested to know which of these two directions you would prefer |
|
Thanks a lot for the clear tests and PR description, finally I fully understand the problem behind ros2/rosidl_python#253 . It is a bit late now here in Europe, I will post my thought on this tomorrow. Interestingly, I think the problem is quite similar to PixarAnimationStudios/OpenUSD#3577 . |
…on bindings
Every interface package exports its Python C bindings library
(lib<pkg>__rosidl_generator_py) in <pkg>_TARGETS, so plain C++ consumers
link it. That library has unresolved CPython symbols, so the consumer
aborts at startup on macOS ("symbol not found in flat namespace
'_PyExc_RuntimeError'") and fails to link or load on Linux when the
library links Python3::Module (RoboStack/ros-kilted#76).
The test builds a C++ executable against ${std_msgs_TARGETS} and runs it
(unix only), and checks that the Python bindings still work from Python.
std_msgs is bumped to build 26 so CI rebuilds it and runs the test.
This commit is expected to fail the new test on macOS. On Linux the
library currently links libpython (Python3::Python), which hides the
problem: the consumer runs, but loads libpython.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Maurice <mauricepurnawan@gmail.com>
(cherry picked from commit 2ed71d4 of #51, without the pkg_additional_info.yaml build-number bump)
…odules Replace the macOS-only patch with a cross-platform one that removes the shared lib<pkg>__rosidl_generator_py library: - The generated C conversion code is compiled (as an OBJECT library) into each <pkg>_s__rosidl_typesupport_* Python extension module, which links Python3::Module. Nothing is installed or exported, so <pkg>_TARGETS only contains C/C++ libraries and no library has unresolved CPython symbols. - Conversion functions of nested types from other packages are looked up (and cached) from those packages' Python message classes (_CONVERT_FROM_PY / _CONVERT_TO_PY capsules, already used by rclpy) instead of linking the other package's library. rosidl_generator_py is bumped to build 26 so CI rebuilds it. So are rosidl_core_generators and rosidl_default_generators: std_msgs only depends on the generator through them, and without rebuilding them rattler-build does not know to build the generator before std_msgs. The regression test added in the previous commit now passes. Message packages built by the old generator link their dependencies' lib<dep>__rosidl_generator_py, so all interface packages must be rebuilt together (e.g. in the next full rebuild). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Maurice <mauricepurnawan@gmail.com> (cherry picked from commit 7d891c3 of #51, without the pkg_additional_info.yaml build-number bump)
The earlier CI runs of this PR cached a std_msgs build 26 built with the old generator; with --skip-existing it would be reused instead of being rebuilt with the patched generator. Drop this commit before merging. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Maurice <mauricepurnawan@gmail.com> (cherry picked from commit 82d0021 of #51)
…odules Port of RoboStack/ros-rolling#51 (without its pkg_additional_info.yaml build-number bumps): stop exporting the lib<pkg>__rosidl_generator_py library, which has unresolved CPython symbols, to C++ consumers of interface packages. The generated conversion code is compiled as an OBJECT library into each Python extension module, and conversion functions of nested types from other packages are looked up from those packages' Python message classes (_CONVERT_FROM_PY/_CONVERT_TO_PY). Adds the std_msgs C++-consumer regression test and a CI cache eviction for std_msgs so it is rebuilt and tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…odules Port of RoboStack/ros-rolling#51 (without its pkg_additional_info.yaml build-number bumps): stop exporting the lib<pkg>__rosidl_generator_py library, which has unresolved CPython symbols, to C++ consumers of interface packages. The generated conversion code is compiled as an OBJECT library into each Python extension module, and conversion functions of nested types from other packages are looked up from those packages' Python message classes (_CONVERT_FROM_PY/_CONVERT_TO_PY). Adds the std_msgs C++-consumer regression test and a CI cache eviction for std_msgs so it is rebuilt and tested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
First of all, thanks a lot for working on this and writing so clearly about the problem. I really like the solution in this PR, but that is quite a departure (and an ABI break) w.r.t. to upstream, so it is something that I think it make sense to discuss upstream and just for ROS Rolling (to avoid the long term divergence of upstream and RoboStack), and once there is an indication that upstream is interesting in merging it, we can adopt it in RoboStack. To have something that can is not so impactful and we can use in earlier distro without diverging too much w.r.t. to upstream, I like your proposal of having a separate An alternative I also thought of but I now think it is worse then your options, is to add a: in this way, any executable that link However, this fails when add_library(libA)
target_link_libraries(libA PRIVATE ${<something>_msgs_TARGETS})
add_executable(execA)
target_link_libraries(execA PRIVATE libA)in thise case, the |
|
TL;DR (this is just my opinion):
|
| - python -c "from rclpy.serialization import deserialize_message, serialize_message; from std_msgs.msg import Header; m = deserialize_message(serialize_message(Header(frame_id='map')), Header); assert m.frame_id == 'map'" | ||
| requirements: | ||
| run: | ||
| - ros-rolling-rclpy |
There was a problem hiding this comment.
| - ros-rolling-rclpy | |
| - ros2-rclpy |
| - ./build/std_msgs_cpp_consumer | ||
| requirements: | ||
| build: | ||
| - ${{ compiler('cxx') }} |
There was a problem hiding this comment.
If the problem emerges with minimally activated compilers, to easily reproduce it without the need for env -u LDFLAGS, you can just use cxx-compiler here directly. The ${{ compiler('cxx') }} macro is important for cross-compiling, but in the case of tests and specificaly for this I guess using cxx-compiler make sense.
| class_module = '%s.%s' % ('.'.join(message.structure.namespaced_type.namespaces), module_name) | ||
| namespaced_type = message.structure.namespaced_type.name | ||
| }@ | ||
| -ROSIDL_GENERATOR_C_EXPORT |
There was a problem hiding this comment.
I think you will need this for Windows.
There was a problem hiding this comment.
I don't have a Windows machine to test this, but claude says:
In the refactor, the conversion functions are only called from inside the same extension DLL, and other packages reach them through capsules, not exported symbols.
Also it looks like it is working fine in the CI (?) I am fine to bring it back though
| set(_target_name_lib "${rosidl_generate_interfaces_TARGET}__rosidl_generator_py") | ||
| -add_library(${_target_name_lib} SHARED ${_generated_c_files}) | ||
| +add_library(${_target_name_lib} OBJECT ${_generated_c_files}) | ||
| +set_target_properties(${_target_name_lib} PROPERTIES POSITION_INDEPENDENT_CODE ON) |
There was a problem hiding this comment.
Why do you need this POSITION_INDEPENDENT_CODE ?
There was a problem hiding this comment.
Related to #51 (comment), if I drop the object library, I can also drop the PIC here. But if I go with the object library, I think we need PIC, otherwise I think I get something like: relocation R_X86_64_PC32 … can not be used when making a shared object on my machine
| +# runtime, so no library needs to be exported or linked across packages. | ||
| set(_target_name_lib "${rosidl_generate_interfaces_TARGET}__rosidl_generator_py") | ||
| -add_library(${_target_name_lib} SHARED ${_generated_c_files}) | ||
| +add_library(${_target_name_lib} OBJECT ${_generated_c_files}) |
There was a problem hiding this comment.
If we are not going to install this, can't we just include the generated source files in the Python extension, instead of having an intermediate OBJECT library?
There was a problem hiding this comment.
I think there is a tradeoff here, if I drop the Object library, then we will end up compiling each _s.c once per module
There was a problem hiding this comment.
As mentioned in #51 (comment), I think we should go in another direction (i.e. removing the ${rosidl_generate_interfaces_TARGET}__rosidl_generator_py from ${<pkgname>_TARGETS} and just leave it in ${<pkgname>_TARGETS__rosidl_generator_py}), so this discussion would be stale. Anyhow, just for completeness, as after the changes the _s.c are only compiled in one module, what is the problem? It would still be compiled only once in both cases, unless I am missing something.
Oh boy, trying to answer this question, I found that |
That is indeed the case. As GPT5.6 summarizes better then me:
AI;DR: The separate list for the generator_py was introduced in ros2/rosidl_python#149, but the definition of rosidl_generator_py_suffix was accidentally removed in ros2/rosidl_python#140 . At this point, the separate target lists seems indeed a much more promising path, as it follow more closely what upstream has been doing. I think the upstream issue ros2/rosidl_python#213 is basically about this. |
|
Thanks for the detailed reply and review.
Given that, what if we go with the minimal patch in the short term, including for Rolling? To be honest, the two fixes plus the test are something I started iterating on last month. I couldn't get a promising result initially, but eventually got to something a few days ago that, IMO, is complete enough to put up for discussion. That said, I can't say I understand every part of the code 100%; quite a bit of it was assisted by Claude, while I mainly provided the direction based on what I learned from my previous iterations. If we go with the minimal patch for all distros, I think I can keep this PR open so we can continue iterating on the patch here. That would also give me some more time to review it myself and run a few additional tests when I have time. Once we are more confident about the approach, I can open the upstream PR and then apply it to ros-rolling. In the meantime, I can create another PR with just the minimal patch targeting the full-rebuild branch, and then "backport" it to the other branches that are also doing rebuilds. That feels less disruptive and should still solve the issue I originally reported. Looking through your comments, it seems like you have already responded yourself to most of the items raised in Stop linking the Python bindings library into C++ consumers of interface packages (fixes dyld _PyExc_RuntimeError / ros-kilted#76) #51 (comment). Let me know if there is anything there that you would still like me to respond to :) |
@mini-1235 to be honest this was my opinion before going more in deep in the history of the problem, as reported #51 (comment) (sorry for the AI slop quote, but you can probably just ignore that and read the |
Ah ok. Just to confirm, you agree to link to Python3::Module on MacOS, but Python3::Python on other platform, correct? |
After reading the history of commits, I think |
I vaguely remember running into a build-time error when I tried that before, but maybe I did something wrong. Let me give it another try, and if it works, I will update the PR |
In that case, I would be curious to see the build-time error you are getting in that case! |
4f02e5d to
8ab3c6a
Compare
I can't reproduce that error anymore. I think it was something I ran into last month, and I have updated my dependencies several times since then. My guess now is that it may have been related to ros2-rust/rosidl_rust#22, which could have caused the build error(?) Although I haven't rebuilt the older setup to verify that. As far as I remember, I didn't change anything other than the dependency versions, so I am also convinced that we should link to |
8f7f9a4 to
e3e61ef
Compare
|
Thanks a lot! I really like this version much more. What I think it is confusing now and risky of upstream rejection, is the fact that we do not use anymore
|
|
fyi @eholum @sea-bass , I think here we finally figured out the proper solution to RoboStack/ros-kilted#76 (comment) . |
…RGETS Replace the patch that compiled the Python bindings into the extension modules with a smaller one that is closer to upstream's original design (ros2/rosidl_python#149): - Restore rosidl_generator_py_suffix ("__rosidl_generator_py"), which ros2/rosidl_python#140 accidentally dropped. The existing dependency loop then links other interface packages' Python bindings through the dedicated <pkg>_TARGETS__rosidl_generator_py list again. - Export the library without ament_export_targets(), which always adds it to <pkg>_TARGETS: install the export set and include it from a config extras file, and keep rosidl_export_typesupport_targets(), which adds the imported target to <pkg>_TARGETS__rosidl_generator_py only. C/C++ consumers of an interface package no longer link it. - Link Python3::Module instead of Python3::Python on all platforms: the library is only linked by Python extension modules, which get the CPython symbols from the interpreter that loads them. Also update the std_msgs regression test as suggested in review: build the C++ consumer with cxx-compiler (no LDFLAGS, like a user environment) instead of compiler('cxx') + clearing LDFLAGS, and depend on ros2-rclpy. No build number bumps: this goes into the full rebuild, which rebuilds all interface packages with the new generator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Maurice <mauricepurnawan@gmail.com>
e3e61ef to
be116c6
Compare
Updated in be116c6
I will tag you once I am open the PR |
|
I open two PRs: ament/ament_cmake#640 and ros2/rosidl_python#269 |
|
Thanks @mini-1235 ! As this PR targets the branch of #41, let's at least wait for Linux CI to complete here before merging this PR, so we can then merge #41 safely. |
|
CI is happy and on Windows/macOS we reached the maximum time, I think we can merge. |
3e79362
into
RoboStack:codex/cross-distro-sync
…RGETS Mirror the simplified fix from RoboStack/ros-rolling#51 (be116c63), replacing the earlier approach that compiled the Python bindings into the extension modules. The library is again a shared library linked through <pkg>_TARGETS__rosidl_generator_py, but it is exported via an installed export set included from a config extras file instead of ament_export_targets(), so it no longer ends up in <pkg>_TARGETS and C/C++ consumers of interface packages don't link it. It links Python::Module rather than libpython, since only Python extension modules link it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…RGETS Mirror the simplified fix from RoboStack/ros-rolling#51 (be116c63), replacing the earlier approach that compiled the Python bindings into the extension modules. The library is again a shared library linked through <pkg>_TARGETS__rosidl_generator_py, but it is exported via an installed export set included from a config extras file instead of ament_export_targets(), so it no longer ends up in <pkg>_TARGETS and C/C++ consumers of interface packages don't link it. It links Python::Module rather than libpython, since only Python extension modules link it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 participants
Problem
rosidl_generator_pyexports each interface package's Python C bindings library,lib<pkg>__rosidl_generator_py, in<pkg>_TARGETS(throughament_export_targets). So a plain C++ node doingtarget_link_libraries(node ${std_msgs_TARGETS})links it. That library uses CPython symbols and is only meant to be loaded by the Python extension modules:-Wl,-dead_strip_dylibsfrom theclang_osx-*compiler activation. Environments without it (e.g.compilers/cxx-compiler2.x, which set noLDFLAGS) hit it.Python3::Module: undefined references at link time, orsymbol lookup errorat load time (Static linking issue with _rosidl_generator_py.so in build _17 ros-kilted#76). LinkingPython3::Pythonhides it, but then every C++ node loads libpython. Ubuntu's GCC passes--as-neededby default, which also hides it.Root cause
As @traversaro found, upstream intended a separate
<pkg>_TARGETS__rosidl_generator_pylist for exactly this: ros2/rosidl_python#149 introducedrosidl_generator_py_suffixfor it. ros2/rosidl_python#140 accidentally removed theset(rosidl_generator_py_suffix ...)while keeping its uses, so they silently fell back to<pkg>_TARGETS. In addition,ament_export_targets()always appends exported targets to<pkg>_TARGETS, so the library leaked there even with the suffix set.Fix
This PR targets the full rebuild in #41, which already contains an earlier version of this work (the patch that compiled the Python bindings into the extension modules). It replaces that
ros-rolling-rosidl-generator-py.patchwith a smaller, CMake-only one (+24/−3 against upstream), closer to upstream's original design:set(rosidl_generator_py_suffix "__rosidl_generator_py"). The existing dependency loop then links other interface packages' Python bindings through<pkg>_TARGETS__rosidl_generator_pyagain.ament_export_targets(), which always appends exported targets to<pkg>_TARGETS: install the export set and include it from a small config extras file (creating the imported target), and keep the existingrosidl_export_typesupport_targets()call, which then adds it to<pkg>_TARGETS__rosidl_generator_pyonly. (Upstream, a cleaner option would be anEXCLUDE_FROM_PACKAGE_TARGETSoption forament_export_targets(), as suggested in review; this PR avoids requiring anament_cmakechange.)Python3::Moduleinstead ofPython3::Pythonon all platforms, since the library is only linked by Python extension modules. (rosidl_generator_py_generate_interfaces: link Python3::Module instead of Python3::Python ros2/rosidl_python#253's Linux CI failure came from the global--no-undefinedthatrosidl_generator_rsused to set, removed in fix(rosidl_generator_rs_generate_interfaces): Remove poisoning of global CMAKE_SHARED_LINKER_FLAGS variable ros2-rust/rosidl_rust#22.)Changes (one commit on top of #41)
patch/ros-rolling-rosidl-generator-py.patch: the minimal patch described above, replacing the extension-module version.tests/ros-rolling-std-msgs.yaml: the regression test updated as suggested in review: the C++ consumer is built withcxx-compiler(noLDFLAGS, like a user environment) instead of${{ compiler('cxx') }}+env -u LDFLAGS, and the Python test depends onros2-rclpy.Note for #41: its commit
80acc94("[DO NOT MERGE] CI: drop cached std_msgs build 26 …") came from the earlier version of this PR and should be dropped before merging.Results
main(earlier revision of this PR): the regression test failed on macOS withdyld: symbol not found in flat namespace '_PyExc_RuntimeError'before the patch (run), and the patch was validated locally with rattler-build on osx-arm64 and linux-64 (both tests pass;std_msgs_TARGETSno longer lists the Python library).ros:rollingcontainer (Ubuntu 26.04, apt, GCC 15) with the same change on upstreamrosidl_python: 8 interface packages rebuilt, the C++ consumer doesn't link the Python library, 134/134 Python round-trips pass.Notes
<pkg>_TARGETS__rosidl_generator_py, so all interface packages need to be rebuilt together, which the full rebuild in Full rebuild September 2026 : bump ros2-distro-mutex to 0.21.0 and build number to 27 and switch to new pinning mechanism (take 2) #41 does.ros2/rosidl_python.This PR description was AI-generated with Claude Opus 5.5 (
claude-opus-5-5).🤖 Generated with Claude Code