urdfdom
urdfdom copied to clipboard
ros
Switch from TinyXML to TinyXML2
The library TinyXML is considered to be unmaintained and since all future development is focused on TinyXML2 this patch updates urdfdom to use TinyXML2.
depends on https://github.com/ros/urdfdom_headers/pull/35
The version of TinyXML2 in Ubuntu 14.04-LTS used by TravisCI is from 2012. That's too old.
Looks like we need to change the CI approach to using Docker containers. @dirk-thomas et. al. have a method using bloom we could likely use.
would these apply to improving this repo's CI?
Sorry, I don't understand the question. Can you please clarify what you are asking.
I was asking you to verify that the wiki tutorial I linked to would be the best approach to applying better CI to this repo. The current CI approach is using too old a version of Ubuntu, so we need a Docker container approach.
I was asking you to verify that the wiki tutorial I linked to would be the best approach to applying better CI to this repo.
This repo doesn't contain a ROS package. It doesn't have a ROS manifest and isn't released into a ROS distro. Therefore ros_buildfarm won't be able to build it.
Alternatively, we could consider installing tinyxml2 from source on Travis CI: https://github.com/rojkov/urdfdom/pull/1
Thanks for merging https://github.com/rojkov/urdfdom/pull/1. It seems the build error can be resolved once https://github.com/ros/urdfdom_headers/pull/39 is merged.
@scpeters Could you retrigger the Travis CI tests? I believe this build error is resolved by ros/urdfdom_headers#39. :tada:
Thanks for fixing the CI build. This changes the API since there are tinyxml symbols in urdf_parser.h. I think this change will require a major version bump.
A question for a subsequent pull request, if we are bumping the major version anyway, is it possible to remove the tinyxml2 symbols from the header file entirely? The answer may be negative, but I just wanted to check while we are making big changes to API.
What is the status of this PR? I don't have a particularly strong opinion on tinyxml 1 vs 2, but if a major version bump is to happen to urdfdom, it would make sense to do this prior to the Melodic release. This decision will affect things like the srdfdom API for Melodic
What is the status of this PR? I don't have a particularly strong opinion on tinyxml 1 vs 2, but if a major version bump is to happen to urdfdom, it would make sense to do this prior to the Melodic release. This decision will affect things like the srdfdom API for Melodic
I ended up doing a somewhat detailed evaluation of the situation in https://github.com/ros/urdf/pull/4#discussion_r169813112 . The short of it is that I think it would be too painful to the whole ecosystem to just remove the tinyxml APIs. Thus, in urdf, we ended up closing https://github.com/ros/urdf/pull/4 and instead merging https://github.com/ros/urdf/pull/9, which adds the tinyxml2 APIs and puts a deprecation warning on the tinyxml APIs. I'd suggest that we do the same thing here, for the same reasons.
As pointed out in https://github.com/ros/urdf/issues/24, the ecosystem actually doesn't use the public TinXML API (heavily). The only publicly exported TinyXML-related API here are exportURDF() and parsePose(). Actual parsing happens from string and all corresponding TinyXML-based methods are private.
Hence, one might think about dropping the public TinyXML API completely. exportURDF() should export to string to be congruent to parseURDF.
In any case, transition to TinyXML2 should be tackled for the next Debian / Ubuntu release, please.
In any case, transition to TinyXML2 should be tackled for the next Debian / Ubuntu release, please.
I am trying to get a feeling on the very rough timeline for the migration to TinyXML2 for urdfdom. Are we talking an order of a few months? I saw that Debian 10 "buster" was released in July 2019 and that the Ubuntu 19.10 feature freeze was on August 22 2019 in https://wiki.ubuntu.com/EoanErmine/ReleaseSchedule so I imagine the releases after these ones (e.g. Ubuntu 20.04) will be targeted instead.
Context : I am looking at packaging urdfdom in conda-forge (see https://github.com/ros/urdfdom/issues/130 for more details), conda-forge has tinyxml2 but not tinyxml and I would avoid packaging tinyxml if possible, since it is a deprecated project.
Is this still actively considered? Urdfdom is there last holdout using tinyxml in Homebrew so this PR would help us a lot.
I believe it's not. There's code conflict now and I haven't been touching ROS for quite some time. It'd be better to close this PR and to resubmit by someone else.