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

add additional patches for gcc 13 and fix rosidl py generator patch on linux #169

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Apr 5, 2024

I had to make the linkage PRIVATE to work on Linux (and not complain later on).

And I had to add 2x cstdint header because gcc 13 requires it.

@Tobias-Fischer Tobias-Fischer merged commit e675bab into RoboStack:main Apr 5, 2024
@tony-p
Copy link

tony-p commented Apr 5, 2024

making the linkage public was needed to solve #119

I have a fix for the later packages in this branch https://github.com/tony-p/ros-humble/tree/fix/rebuild-regressions

Haven't opened a PR yet as a gazebo plugins package build was failing and hadn't yet had the chance to look into it/solve it

@wolfv
Copy link
Member Author

wolfv commented Apr 5, 2024

I had the issue that with PUBLIC linkage downstream repos complained because of a missing find_package(Python3 ...). Do you know how to solve that, perhaps?

I took the changes (inspiration) from upstream, btw: https://github.com/ros2/rosidl_python/blob/rolling/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake

They recently (2 months ago) modernized this CMake.

@wolfv
Copy link
Member Author

wolfv commented Apr 5, 2024

Any help and insights appreciated btw!

@traversaro
Copy link
Member

making the linkage public was needed to solve #119

I have a fix for the later packages in this branch https://github.com/tony-p/ros-humble/tree/fix/rebuild-regressions

Haven't opened a PR yet as a gazebo plugins package build was failing and hadn't yet had the chance to look into it/solve it

It may be helpful also if you have a MRE (such as a small self-contained CMake project showing the problem) of the problem tracked in #119, so that we can easily check if the problem is solved or not.

@wolfv
Copy link
Member Author

wolfv commented Apr 5, 2024

Upstream also uses this function: https://github.com/ros2/rosidl_python/blob/0497ce85b9df224085dd578eb0454aa56d1713b2/rosidl_generator_py/cmake/rosidl_generator_py_generate_interfaces.cmake#L178-L180

Not sure if that changes much...

@tony-p
Copy link

tony-p commented Apr 5, 2024

The fix to find the python packages was in cmake/rosidl_generator_py_generate_interfaces.cmake add

find_package(rosidl_generator_py REQUIRED)
ament_export_dependencies(rosidl_generator_py)

as rosidl_generator_py includes a cmake extras file which includes the necessary find_package(Python .... and in this way it gets passed to all the downstream dependencies

https://github.com/tony-p/ros-humble/blob/fix/rebuild-regressions/patch/ros-humble-rosidl-generator-py.patch

@tony-p
Copy link

tony-p commented Apr 5, 2024

making the linkage public was needed to solve #119
I have a fix for the later packages in this branch https://github.com/tony-p/ros-humble/tree/fix/rebuild-regressions
Haven't opened a PR yet as a gazebo plugins package build was failing and hadn't yet had the chance to look into it/solve it

It may be helpful also if you have a MRE (such as a small self-contained CMake project showing the problem) of the problem tracked in #119, so that we can easily check if the problem is solved or not.

The public version did solve (the building) for me and no longer need to manually link in the python files. Next step was to actually run nodes to ensure it still ran happily

I can look into making an MRE .... just juggling a few things at the moment

@tony-p
Copy link

tony-p commented Apr 5, 2024

The work around is a bit dirty, but the ament dependency at least makes sure it is applied recursively. I'm no cmake expert so tried to do the same using more native CMake for only the python libraries but quickly gave up.

@wolfv
Copy link
Member Author

wolfv commented Apr 5, 2024

btw. related I also thought about adding tests in this repository, that would be added to the individual recipes (similar to patches). That way we could do some simple checks (e.g. starting the demos, importing rclpy etc.).

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.

4 participants