Remove rviz_default_plugins dependency on TinyXML #531
Conversation
This clears the way for urdf to switch to TinyXML2 Note that internally, urdf was converting the passed XML to a string and reparsing it in the implementation of `urdf::model::initXml`
|
Note also that there is already my ancient PR to get rid of TinyXML also in |
This clears the way for urdfdom to switch to TinyXML2 Depends on ros2/rviz#531 and ros2/kdl_parser#7
|
@Martin-Idel it's sad to see a PR like that wither on the vine. @cottsay, @wjwwood, any idea why #418 never got pulled in and how to prevent such backlogs? |
We don't have time to review all the pull requests and that one was considered to be cleanup, and so lower priority than bug fixes or feature development. Preventing them? More maintainers to share the load is the only thing I can think of. We have a triage process, but it's imperfect, we don't get to all issues, simply because we do not have the resources to do so. I don't know why this one was not triaged to be honest. |
|
Even if low priority, it probably should have come up at some time in the last 10 months. I can't find public details on the current triage process, but there should be some action authors can take or automatic re-triage for PRs that have been idle for a few weeks. |
Resolve merge conflicts from ros2#533
|
Ready for review |
|
To me, this looks good. @wjwwood I know that your time is limited, that's why I never proposed changes that are even more cleanup (getting rid of an old dependencies is more important than cleaning up the cmake code which I always wanted to do because it's so messy...), I just wanted to point out that maybe if you look at this, it's time to get rid of tinyxml once and for all - that'd be one less package to worry about ;-). I also have only so much time, but if I can help you somehow let me know (for instance, do reviews help? I don't know how much that helps you or whether Open Robotics wants a full internal review of any PR). |
|
@Martin-Idel We do appreciate any reviews you can give, especially given that you did a lot of this port initially. So that is always valuable. Discussing this PR vs #418, I would like to eventually get all of #418 in, but I think getting just this in for now is a good step forward. That way we can deprecate the URDF @rotu With all of that said, this needs a rebase to get to a green CI. Once this is rebased I'll fire up a CI job for this along with ros2/kdl_parser#8 and ros2/urdf#12 |
|
@clalancette backmerged. |
Fantastic, thanks. I'll kick off CI on this along with the other PRs. |
|
|
All right, we have a fix for the deprecation warnings over in ros2/kdl_parser#8 (thanks @rotu). Here's another CI go:
|
|
macOS warning is known problem, so we can ignore that one. I think we are good to go here, but I want to get approval from the Foxy ROS Boss before we go ahead and merge these three. |
This clears the way for urdf to switch to TinyXML2
Note that internally, urdf was converting the passed XML to a string and reparsing it in the implementation of
urdf::model::initXml