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 urdf/model.h -> urdf/model.hpp #3003

Merged
merged 5 commits into from
Oct 11, 2024
Merged

Conversation

rhaschke
Copy link
Contributor

The old header is marked obsolete.

The old header is marked obsolete.
sea-bass
sea-bass previously approved these changes Sep 17, 2024
@sea-bass sea-bass dismissed their stale review September 17, 2024 00:18

Not all addressed yet

sea-bass
sea-bass previously approved these changes Oct 1, 2024
Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

This changelist is actually fine.

HOWEVER, CI is still failing because srdfdom requires the same update, and a release since it's installed from binaries.

So I think for now we'll need this merged, and brought in via vcstool until there is a new release of srdfdom bloomed: moveit/srdfdom#127

@sea-bass
Copy link
Contributor

sea-bass commented Oct 1, 2024

BTW for reference, seems the deprecation warning came in with a linting PR: ros2/urdf#39

Since this URDF deprecation warning change was only added to Rolling via the latest urdf repo changes, we will need to handle this separately for Humble/Iron since those branches do not have model.hpp.

@sea-bass sea-bass dismissed their stale review October 2, 2024 12:34

needs more changes after all

@sjahr sjahr mentioned this pull request Oct 10, 2024
6 tasks
@sjahr
Copy link
Contributor

sjahr commented Oct 10, 2024

@rhaschke I took your PR and added SebC's proposed changes. 🤞 this fixes CI

@rhaschke
Copy link
Contributor Author

@sjahr: I added srdfdom to the upstream workspace, which should fix CI.

@rhaschke
Copy link
Contributor Author

Looks like workspace overlay doesn't work correctly: Although srdfdom is part of the upstream_ws, MoveIt still finds srdfdom headers in /opt/ros... Same issue in #3026.

@mikeferguson
Copy link
Contributor

I think there isn't anything we can do at the moment to get CI working - however, for the future, we can fix srdfdom to install it's headers slightly differently (into include/${PACKAGE_NAME} as noted in ros2/ros2#1150) so that overlays could work - this is what most of the core ROS packages do to work around the underlay/overlay issue

@sea-bass
Copy link
Contributor

Worst case @JafarAbdi and I already put in a new release of srdfdom for the next sync, so... yeah.

@sjahr
Copy link
Contributor

sjahr commented Oct 11, 2024

#3026 (review)

Do we need the selective include here in the moveit2 repo? As far as I understood, that was only necessary for srdfdom, which only has a single branch for ros2. Here, we have different branches for rolling and older releases.

The main branch is supposed to support multiple versions of ROS 2 because it acts as a "dev" branch that contains the latest (breaking) features and the distribution branches contain a stable version that is branched off around the distribution release and only receives bug fixes (@tylerjw wrote a nice article about this in the moveit blog a while ago). Without the selective include we'd break support for Humble and Iron and personally, I'd be in favor of adding the selective for now. Feel free to take what you need from #3026, if you agree

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

Let's get this in until srdfdom is released.

Can we have an issue or TODO reminder to re-enable this compiler warning though?


also, the fix to the tutorials build is (hopefully) in moveit/moveit2_tutorials#977

@sjahr sjahr merged commit 9f00fd0 into moveit:main Oct 11, 2024
10 of 13 checks passed
@rhaschke rhaschke deleted the fix-urdf-include branch October 13, 2024 10:54
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