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

Forward port ABI compatibility workaround with OBBs to ros2 branch #252

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

sea-bass
Copy link

Forward porting b00b1e5 since we ran into the ABI incompatibility...

moveit/moveit2#3130

Closes #251

Copy link

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Thanks!

@sjahr sjahr merged commit 3bece34 into ros2 Nov 26, 2024
9 checks passed
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (8f1002e) to head (1fe4313).
Report is 1 commits behind head on ros2.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@     Coverage Diff     @@
##   ros2   #252   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sea-bass sea-bass deleted the fix-abi-with-obbs branch November 26, 2024 11:43
@rhaschke
Copy link
Contributor

Just applying the ABI compatibility hack is a bad idea. As pointed out by @peci1 in its original ROS1 commits, this should only be necessary for compatibility with release binaries.
However, at least in CI you are building everything (geometric_shapes and MoveIt) from source. Thus, there shouldn't be any ABI compat issues. As we see issues anyway, they probably originate from the build system not correctly resolving headers. I suggest to tackle this root cause instead of introducing the ABI compat hack here.

@rhaschke
Copy link
Contributor

I will have a short look this afternoon.

@sea-bass
Copy link
Author

Thank you! This issue was specifically happening with the main branch of moveit2 built from source in a Ubuntu 22.04/Humble environment, if that is any help.

(To verify changes, I was just running the humble-ci job)

@rhaschke
Copy link
Contributor

I strongly believe that this the good old header installation problem. Since ROS Humble, headers should be installed to /opt/ros/<distro>/include/<package>/<package>/:
https://docs.ros.org/en/humble/Releases/Release-Humble-Hawksbill.html#c-headers-are-installed-in-a-subdirectory
https://colcon.readthedocs.io/en/released/user/overriding-packages.html#install-headers-to-a-unique-include-directory

MoveIt2 never realized that change, causing ABI issues with package overlays to persist.
We recently fixed this for srdfdom: moveit/srdfdom#132 and moveit/srdfdom@65ce909.
I think, it's time to go through all MoveIt-releated packages and eventually fix it.
Unfortunately, I don't have the resources to do so.

@rhaschke
Copy link
Contributor

I created moveit/moveit2#3134 as a reminder and will provide a PR here as another example.

@sea-bass
Copy link
Author

sea-bass commented Nov 27, 2024

MoveIt has had some active new contributors lately that may want to/be able to help with this!

Do you mind making a new issue on that repo? I can bring it up at the maintainer meeting alongside the topic of switching from .h to .hpp files.

Edit: hah, way ahead of me :)

rhaschke added a commit to rhaschke/geometric_shapes that referenced this pull request Nov 27, 2024
sea-bass pushed a commit that referenced this pull request Nov 29, 2024
* Revert "Forward port ABI compatibility workaround with OBBs to ros2 branch (#252)"

This reverts commit 3bece34.

* Install headers into subdirectory

... following the standard defined in
https://colcon.readthedocs.io/en/released/user/overriding-packages.html#install-headers-to-a-unique-include-directory

* Declare missing include dependency for a test
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