Stop linking the Python bindings library into C++ consumers of interface packages (fixes dyld _PyExc_RuntimeError / ros-kilted#76) #51
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| 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 2fe245b..f46b674 100644 | ||
| --- a/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake | ||
| +++ b/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake | ||
| @@ -152,8 +152,14 @@ set_property( | ||
| ${_generated_extension_files} ${_generated_py_files} ${_generated_c_files} | ||
| PROPERTY GENERATED 1) | ||
|
|
||
| +# The generated C conversion functions are compiled into each Python extension | ||
| +# module below, rather than into a shared library: they use CPython symbols | ||
| +# that only a Python interpreter provides. Conversion functions of types from | ||
| +# other packages are looked up from those packages' Python message classes at | ||
| +# 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}) | ||
| +set_target_properties(${_target_name_lib} PROPERTIES POSITION_INDEPENDENT_CODE ON) | ||
|
Member
There was a problem hiding this comment. Why do you need this |
||
| target_link_libraries(${_target_name_lib} PRIVATE | ||
| ${rosidl_generate_interfaces_TARGET}__rosidl_generator_c) | ||
| add_dependencies( | ||
| @@ -165,7 +171,7 @@ add_dependencies( | ||
| target_link_libraries( | ||
| ${_target_name_lib} PRIVATE | ||
| Python3::NumPy | ||
| - Python3::Python | ||
| + Python3::Module | ||
| ) | ||
| target_include_directories(${_target_name_lib} | ||
| PRIVATE | ||
| @@ -242,26 +248,8 @@ foreach(_typesupport_impl ${_typesupport_impls}) | ||
| endif() | ||
| endforeach() | ||
|
|
||
| - | ||
| -# Depend on rosidl_generator_py generated targets from our dependencies | ||
| -foreach(_pkg_name ${rosidl_generate_interfaces_DEPENDENCY_PACKAGE_NAMES}) | ||
| - target_link_libraries(${_target_name_lib} PRIVATE ${${_pkg_name}_TARGETS${rosidl_generator_py_suffix}}) | ||
| -endforeach() | ||
| - | ||
| set_target_properties(${_target_name_lib} PROPERTIES COMPILE_OPTIONS "${_extension_compile_flags}") | ||
|
|
||
| -if(NOT rosidl_generate_interfaces_SKIP_INSTALL) | ||
| - install(TARGETS ${_target_name_lib} | ||
| - EXPORT export_${_target_name_lib} | ||
| - ARCHIVE DESTINATION lib | ||
| - 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}) | ||
| -endif() | ||
| - | ||
| if(BUILD_TESTING AND rosidl_generate_interfaces_ADD_LINTER_TESTS) | ||
| if( | ||
| NOT _generated_py_files STREQUAL "" OR | ||
| diff --git a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em | ||
| index d234915..6d1bef1 100644 | ||
| --- a/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em | ||
| +++ b/rosidl_generator_py/resource/_msg_pkg_typesupport_entry_point.c.em | ||
| @@ -48,9 +48,7 @@ static void @('__'.join(message.structure.namespaced_type.namespaces + [module_n | ||
| @(msg_typename)__destroy(ros_message); | ||
| } | ||
|
|
||
| -ROSIDL_GENERATOR_C_IMPORT | ||
| bool @('__'.join(message.structure.namespaced_type.namespaces + [module_name]))__convert_from_py(PyObject * _pymsg, void * ros_message); | ||
| -ROSIDL_GENERATOR_C_IMPORT | ||
| PyObject * @('__'.join(message.structure.namespaced_type.namespaces + [module_name]))__convert_to_py(void * raw_ros_message); | ||
|
|
||
|
|
||
| diff --git a/rosidl_generator_py/resource/_msg_support.c.em b/rosidl_generator_py/resource/_msg_support.c.em | ||
| index 62941b2..76c89bd 100644 | ||
| --- a/rosidl_generator_py/resource/_msg_support.c.em | ||
| +++ b/rosidl_generator_py/resource/_msg_support.c.em | ||
| @@ -158,15 +158,80 @@ type_ = member.type | ||
| if isinstance(type_, AbstractNestedType): | ||
| type_ = type_.value_type | ||
| }@ | ||
| -@[ if isinstance(type_, NamespacedType)]@ | ||
| -@[ if type_.namespaces[0] != package_name]@ | ||
| -ROSIDL_GENERATOR_C_IMPORT | ||
| -@[ end if]@ | ||
| +@[ if isinstance(type_, NamespacedType) and type_.namespaces[0] == package_name]@ | ||
| bool @('__'.join(type_.namespaces + [convert_camel_case_to_lower_case_underscore(type_.name)]))__convert_from_py(PyObject * _pymsg, void * _ros_message); | ||
| -@[ if type_.namespaces[0] != package_name]@ | ||
| -ROSIDL_GENERATOR_C_IMPORT | ||
| -@[ end if]@ | ||
| PyObject * @('__'.join(type_.namespaces + [convert_camel_case_to_lower_case_underscore(type_.name)]))__convert_to_py(void * raw_ros_message); | ||
| +@[ elif isinstance(type_, NamespacedType) and ('converter', *type_.namespaced_name()) not in include_directives]@ | ||
| +@{ | ||
| +include_directives.add(('converter', *type_.namespaced_name())) | ||
| +converter_prefix = '__'.join(type_.namespaces + [convert_camel_case_to_lower_case_underscore(type_.name)]) | ||
| +# the Python module is named after the interface file, e.g. _add_two_ints for | ||
| +# the AddTwoInts_Request service message | ||
| +interface_name = type_.name if type_.namespaces[-1] == 'msg' else type_.name.split('_', 1)[0] | ||
| +converter_module = '.'.join(type_.namespaces) + '._' + convert_camel_case_to_lower_case_underscore(interface_name) | ||
| +}@ | ||
| +@[ if 'converter_lookup' not in include_directives]@ | ||
| +@{include_directives.add('converter_lookup')}@ | ||
| +// Get a conversion function of a message type from another package from its | ||
| +// Python class, which that package's extension module provides it to. | ||
| +static void * _rosidl_generator_py__get_converter( | ||
| + const char * module_name, const char * class_name, const char * capsule_name) | ||
| +{ | ||
| + PyObject * module = PyImport_ImportModule(module_name); | ||
| + if (!module) { | ||
| + return NULL; | ||
| + } | ||
| + PyObject * pyclass = PyObject_GetAttrString(module, class_name); | ||
| + Py_DECREF(module); | ||
| + if (!pyclass) { | ||
| + return NULL; | ||
| + } | ||
| + PyObject * capsule = PyObject_GetAttrString(pyclass, capsule_name); | ||
| + if (capsule == Py_None) { | ||
| + Py_DECREF(capsule); | ||
| + PyObject * result = PyObject_CallMethod(pyclass, "__import_type_support__", NULL); | ||
| + if (!result) { | ||
| + Py_DECREF(pyclass); | ||
| + return NULL; | ||
| + } | ||
| + Py_DECREF(result); | ||
| + capsule = PyObject_GetAttrString(pyclass, capsule_name); | ||
| + } | ||
| + Py_DECREF(pyclass); | ||
| + if (!capsule) { | ||
| + return NULL; | ||
| + } | ||
| + void * converter = PyCapsule_GetPointer(capsule, NULL); | ||
| + Py_DECREF(capsule); | ||
| + return converter; | ||
| +} | ||
| + | ||
| +@[ end if]@ | ||
| +static bool @(converter_prefix)__convert_from_py(PyObject * _pymsg, void * _ros_message) | ||
| +{ | ||
| + static bool (* convert)(PyObject *, void *) = NULL; | ||
| + if (!convert) { | ||
| + convert = (bool (*)(PyObject *, void *))_rosidl_generator_py__get_converter( | ||
| + "@(converter_module)", "@(type_.name)", "_CONVERT_FROM_PY"); | ||
| + if (!convert) { | ||
| + return false; | ||
| + } | ||
| + } | ||
| + return convert(_pymsg, _ros_message); | ||
| +} | ||
| + | ||
| +static PyObject * @(converter_prefix)__convert_to_py(void * raw_ros_message) | ||
| +{ | ||
| + static PyObject * (* convert)(void *) = NULL; | ||
| + if (!convert) { | ||
| + convert = (PyObject * (*)(void *))_rosidl_generator_py__get_converter( | ||
| + "@(converter_module)", "@(type_.name)", "_CONVERT_TO_PY"); | ||
| + if (!convert) { | ||
| + return NULL; | ||
| + } | ||
| + } | ||
| + return convert(raw_ros_message); | ||
| +} | ||
| @[ end if]@ | ||
| @[end for]@ | ||
|
|
||
| @@ -175,7 +240,6 @@ module_name = '_' + convert_camel_case_to_lower_case_underscore(interface_path.s | ||
| class_module = '%s.%s' % ('.'.join(message.structure.namespaced_type.namespaces), module_name) | ||
| namespaced_type = message.structure.namespaced_type.name | ||
| }@ | ||
| -ROSIDL_GENERATOR_C_EXPORT | ||
|
Member
There was a problem hiding this comment. I think you will need this for Windows. |
||
| bool @('__'.join(message.structure.namespaced_type.namespaces + [convert_camel_case_to_lower_case_underscore(message.structure.namespaced_type.name)]))__convert_from_py(PyObject * _pymsg, void * _ros_message) | ||
| { | ||
| // check that the passed message is of the expected Python class | ||
| @@ -573,7 +637,6 @@ nested_type = '__'.join(type_.namespaced_name()) | ||
| return true; | ||
| } | ||
|
|
||
| -ROSIDL_GENERATOR_C_EXPORT | ||
| PyObject * @('__'.join(message.structure.namespaced_type.namespaces + [convert_camel_case_to_lower_case_underscore(message.structure.namespaced_type.name)]))__convert_to_py(void * raw_ros_message) | ||
| { | ||
| /* NOTE(esteve): Call constructor of @(message.structure.namespaced_type.name) */ | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,29 @@ | ||||||
| tests: | ||||||
| # Regression test: a plain C++ consumer of std_msgs must build and run. | ||||||
| # If std_msgs_TARGETS exports the Python C bindings library | ||||||
| # (libstd_msgs__rosidl_generator_py), the consumer links it and it has | ||||||
| # unresolved CPython symbols: the executable aborts on macOS ("symbol not | ||||||
| # found in flat namespace '_PyExc_RuntimeError'") and fails to link or load | ||||||
| # on Linux when it links Python3::Module | ||||||
| # (https://github.com/RoboStack/ros-kilted/issues/76). | ||||||
| # LDFLAGS is cleared so -Wl,--as-needed / -Wl,-dead_strip_dylibs can't hide it. | ||||||
| - if: unix | ||||||
| then: | ||||||
| script: | ||||||
| - env -u LDFLAGS cmake -S cpp_consumer -B build -G Ninja -DCMAKE_PREFIX_PATH="$PREFIX" | ||||||
| - cmake --build build | ||||||
| - ./build/std_msgs_cpp_consumer | ||||||
| requirements: | ||||||
| build: | ||||||
| - ${{ compiler('cxx') }} | ||||||
|
Member
There was a problem hiding this comment. If the problem emerges with minimally activated compilers, to easily reproduce it without the need for |
||||||
| - cmake | ||||||
| - ninja | ||||||
| files: | ||||||
| recipe: | ||||||
| - cpp_consumer/ | ||||||
| # The Python C bindings library still works when loaded by Python. | ||||||
| - script: | ||||||
| - 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 | ||||||
|
Member
There was a problem hiding this comment.
Suggested change
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| cmake_minimum_required(VERSION 3.20) | ||
| project(std_msgs_cpp_consumer CXX) | ||
|
|
||
| find_package(std_msgs REQUIRED) | ||
|
|
||
| add_executable(std_msgs_cpp_consumer main.cpp) | ||
| target_link_libraries(std_msgs_cpp_consumer ${std_msgs_TARGETS}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| #include <iostream> | ||
|
|
||
| #include <std_msgs/msg/string.hpp> | ||
|
|
||
| int main() | ||
| { | ||
| std_msgs::msg::String msg; | ||
| msg.data = "hello"; | ||
| std::cout << "std_msgs C++ consumer started fine: " << msg.data << std::endl; | ||
| return 0; | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
OBJECTlibrary?