Remove direct dependency on TinyXML

[rotu]

No description provided.

[clalancette]

I'm on the fence on this one. In the upstream https://github.com/ros/kdl_parser , we ended up deprecating the TinyXML API and adding a TinyXML2 one. I'm not sure how heavily either is used.

Also, this is another repository where we forked it for ROS 2, but this has fallen behind the melodic-devel and noetic-devel branches on the upstream.

There are three things that we have to decide here. The first is whether to recombine this repository with the upstream. The second is whether to rebase/merge this fork with the upstream repository (so we get the latest fixes/changes from upstream). And the third is whether to deprecate/remove the TinyXML API here. They are all kind of separate, but also heavily related.

My personal opinion is that we should recombine the repositories, rebase/merge the branches, and then deprecate the TinyXML API here for a release cycle. But that is also the most amount of work. @sloretz thoughts?

[sloretz]

My personal opinion is that we should recombine the repositories

Agreed.

but this has fallen behind the melodic-devel and noetic-devel branches on the upstream.

rebase/merge the branches

I'm not sure I understand. Is this ROS 1 or ROS 2 branches?

I would:

1. push the ros2, dashing, and eloquent branches to ros/kdl_parser
2. update Dashing and Eloquent source and doc entries on ros/rosdistro to point to ros/kdl_parser
3. delete every branch here except for ardent, bouncy, and crystal
4. archive this repo

then deprecate the TinyXML API here for a release cycle

Yeah, deprecating is the polite thing to do. I think it would be fair to remove the TinyXml depend from the package.xml, delete any test coverage, and forward-declare TiXmlDocument. Anyone using that API would have had to include TinyXml headers in order to create a TiXmlDocument *, so those that don't need it can avoid installing it.

[rotu]

Since we don’t get any sort of performance boost from the tinyxml API, I think it provides little to no value.

You certainly could pre-empt this PR with one that adds deprecation notices, and delay this PR for a release cycle. It may be the polite thing to do. It also means twice as many PRs to juggle and more opportunity for this PR to get lost.

[rotu]

@clalancette, @sloretz so how should I move forward with this PR? If we're gonna archive this repo and retarget the PR, can we do it now?

None of the dependents use the TinyXML API anyway, so I don't think it's a problem to take it out now.

[clalancette]

It's unlikely I'm going to get to this before Foxy release; there's just too much going on with that.

However, I will commit to some housecleaning after Foxy. In particular:

• Look at recombining ros2/kdl_parser and ros/kdl_parser
• Look at recombining ros2/urdf and ros/urdf
• Look at recombining ros2/urdfdom and ros/urdfdom

[rotu]

That’s fine if the housekeeping has to wait. Can we then move forward with this PR and do the housekeeping later?

[clalancette]

That’s fine if the housekeeping has to wait. Can we then move forward with this PR and do the housekeeping later?

I don't think it is a good idea to just remove the APIs without deprecating them first. The reason is simple: we don't know all of the callers. Most of the time when we deprecate and remove APIs, we hear about uses that we never knew about. Part of the motivation to combine this with the original upstream is that we have already deprecated it there: https://github.com/ros/kdl_parser/blob/24597c21301c53b66a5e0ae22619210261f1a28f/kdl_parser/include/kdl_parser/kdl_parser.hpp#L89

But with that said, I'd be OK doing a PR into this repository now that deprecated the API for the Foxy release. Then for G-Turtle we can remove the API completely.

[rotu]

I think it's really unreasonable to leave this PR open for a whole year.

I thought maybe deprecation was unnecessary since manual inspection of indexed dependent packages shows this function is unused: https://index.ros.org/p/kdl_parser/github-ros2-kdl_parser/#eloquent-deps

Since you feel deprecation is necessary, I'm going to do the deprecation PRs now so they're in for Foxy and this PR can be merged in a reasonable timeframe.

[sloretz]

Closing because this repo is being archived and it's content moved back to https://github.com/ros/kdl_parser . Please reopen the pull request there.