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 include statements to use new pluginlib and class_loader headers #827

Conversation

mikaelarguedas
Copy link
Contributor

Disclaimer: This in an automated PR, if it is not relevant on this branch, apologies for the inconveniance, feel free close it.


pluginlib and class_loader headers have been refactored and renamed. The previous headers .h have been deprecated in favor of their .hpp equivalent. The new headers are available on all active ROS distributions and the deprecated ones will be removed in a future version of ROS (likely ROS-N).

This PR migrates the include statements to use the non-deprecated ones and should compile for any active ROS distribution starting with Indigo
The migration was done by running the scripts pluginlib_headers_migration.py and class_loader_headers_update.py on this repository.
Note: this will not work for Jade

@v4hn
Copy link
Contributor

v4hn commented Apr 7, 2018

CI tells us that this is not yet rolled out in indigo and kinetic. So we should not merge it yet.

@mikaelarguedas
Copy link
Contributor Author

These changes have been rolled out in indigo and kinetic during the last sync. CI fails fails because the docker images it uses have not been rebuilt since the last sync. Running an apt upgrade in your CI before performing the build should fix it.
More details on the sister discussion here

@rhaschke
Copy link
Contributor

I retriggered Travis. The docker containers should have been rebuild 5 days ago.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

I will rebase and resolve conflicts and then squash-merge.

@rhaschke
Copy link
Contributor

rhaschke commented May 16, 2018

Squash-merged with ab0f706.
Cherry-picked into melodic-devel: ee42c7c

@rhaschke rhaschke closed this May 16, 2018
@mikaelarguedas
Copy link
Contributor Author

Thanks @rhaschke.
In case it's useful: you can now Squash and Merge on the GitHub UI directly by using the drop down next to the "Merge pull request" button

@mikaelarguedas mikaelarguedas deleted the mikael/pluginlib_class_loader_header_migration branch May 16, 2018 15:30
@rhaschke
Copy link
Contributor

@mikaelarguedas I know about the github merge features. However, in the meantime this PR had a minor merge conflict with the main branch. In this case, you can only go for a fixing merge...

@mikaelarguedas
Copy link
Contributor Author

👍

@knorth55
Copy link
Contributor

In Kinetic, pluginlibchange is not released yet :( .
https://github.com/ros/pluginlib/commits/kinetic-devel
ros/pluginlib@84b0c46
This causes build error with moveit 0.9.12.
I will request to release new version of pluginlib in kinetic.

@mikaelarguedas
Copy link
Contributor Author

This change should not impact downstream packages as both versions of the headers are installed and released.

Moveit 0.9.12 builds successfully on the buildfarm on both Kinetic and Lunar. Can you provide more details about the build failure you are referring to ?

@rhaschke
Copy link
Contributor

Our CI server as well as ROS build farm can perfectly build this. Maybe you need to update your pluginlib installation?

@v4hn
Copy link
Contributor

v4hn commented May 31, 2018

@knorth55 This commit only adjusts to the new headers, the old headers are still there.
this should not trigger errors.

@v4hn
Copy link
Contributor

v4hn commented May 31, 2018

@mikaelarguedas I actually hit problems with the header changes though: pal-robotics maintains their own mirror of packages for TIAGo with a more cautious policy than OSRF.
They did not sync pluginlib (yet), so MoveIt devel (and everything else that updated the headers in kinetic) does not build there at the moment.
And no, it's not their fault for not updating right away...

@rhaschke
Copy link
Contributor

If you rely on a different package repo, you shouldn't update the MoveIt package as well...

@knorth55
Copy link
Contributor

Oh, sorry. I misunderstood the problem.
I will check again the build error correctly.

@mikaelarguedas
Copy link
Contributor Author

pal-robotics maintains their own mirror of packages for TIAGo with a more cautious policy than OSRF.

Good to know, has the problem been reported to them ? If so can you please provide a link here ?

And no, it's not their fault for not updating right away...

Maybe there's history I'm missing here, but I don't recall suggesting that.

I think the part that was missing in this PR is a version constraint on the pluginlib dependency in the package.xml's of the modified packages.
Please see #927 for a follow-up PR

@knorth55
Copy link
Contributor

I realized in our jenkins, and the docker image in the jenkins was not updated.
This causes old version pluginlib error, I thank you all!

dg-shadow pushed a commit to shadow-robot/moveit that referenced this pull request Jul 30, 2018
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

5 participants