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

update ros idl generate-py patch #168

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Apr 4, 2024

The initial problem for me was that when performing a full rebuild, it complains later about not finding Python.h (when generating or depending on some messages, I think). Some CMake-export file does not properly expose the Python dependency in a later stage.

The following patch worked a little bit better when doing a local rebuild, but the Python demos actually crash with a segfault on macOS... I haven't investigated why but I remember that such a crash was previously a problem when linking Python, right?

Just for reference I copied a bit of CMake from the current main branch: https://github.com/ros2/rosidl_python/blob/rolling/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake

I can try to find some more minimal way of debugging this over the next days.

@wolfv
Copy link
Member Author

wolfv commented Apr 4, 2024

I can try to use the entirety of this commit: ros2/rosidl_python#140

Although I am not sure how applicable that is to ros-humble ...

@Tobias-Fischer
Copy link
Contributor

I think this causes the issues on OSX:
https://github.com/RoboStack/ros-humble/blob/2ac1e702ac4c96b3c62faafb4f59baad611467f9/patch/ros-humble-rosidl-generator-py.patch#L76C2-L80C83

We should only link to Python3::Python on non-osx platforms (not sure about numpy)

@wolfv
Copy link
Member Author

wolfv commented Apr 4, 2024

I simplified the changes to just use

target_include_directories(${_target_name_lib} PUBLIC ${Python3_INCLUDE_DIRS} ${Python3_NumPy_INCLUDE_DIRS})

on osx ... I think that helped! At least it seems to work :)

@Tobias-Fischer
Copy link
Contributor

Thanks a lot @wolfv. I'm happy with the change - could you please take a quick look too, @traversaro?

@wolfv
Copy link
Member Author

wolfv commented Apr 5, 2024

I'll try this on Linux today, too, just to be sure.

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Ok for me. The resulting package will not be relocatable, but conda relocation features will handle this.

@Tobias-Fischer Tobias-Fischer merged commit 403da1b into RoboStack:main Apr 5, 2024
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.

None yet

3 participants