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

Switch from TinyXML to TinyXML2 #99

Closed
wants to merge 3 commits into from
Closed

Conversation

rojkov
Copy link

@rojkov rojkov commented Jun 14, 2017

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 ros/urdfdom_headers#35

The library TinyXML is considered to be unmaintained and
since all future development is focused on TinyXML2 this
patch updates urdfdom to use TinyXML2.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@linux.intel.com>
@rojkov
Copy link
Author

rojkov commented Jun 15, 2017

The version of TinyXML2 in Ubuntu 14.04-LTS used by TravisCI is from 2012. That's too old.

@davetcoleman
Copy link
Contributor

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.

@davetcoleman
Copy link
Contributor

@dirk-thomas would these apply to improving this repo's CI?

@dirk-thomas
Copy link
Member

would these apply to improving this repo's CI?

Sorry, I don't understand the question. Can you please clarify what you are asking.

@davetcoleman
Copy link
Contributor

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.

@dirk-thomas
Copy link
Member

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.

@jslee02
Copy link
Contributor

jslee02 commented Sep 9, 2017

Alternatively, we could consider installing tinyxml2 from source on Travis CI: rojkov#1

@jslee02
Copy link
Contributor

jslee02 commented Sep 18, 2017

Thanks for merging rojkov#1. It seems the build error can be resolved once ros/urdfdom_headers#39 is merged.

@jslee02
Copy link
Contributor

jslee02 commented Oct 12, 2017

@scpeters Could you retrigger the Travis CI tests? I believe this build error is resolved by ros/urdfdom_headers#39. 🎉

@sloretz
Copy link
Contributor

sloretz commented Oct 12, 2017

@jslee02 started build number 54

@scpeters
Copy link
Contributor

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.

@IanTheEngineer
Copy link

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

@clalancette
Copy link
Contributor

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 ros/urdf#4 (comment) . 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 ros/urdf#4 and instead merging ros/urdf#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.

@rhaschke
Copy link
Contributor

rhaschke commented Feb 8, 2019

As pointed out in ros/urdf#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.

@lesteve
Copy link

lesteve commented Jul 19, 2019

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 #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.

@SMillerDev
Copy link

Is this still actively considered? Urdfdom is there last holdout using tinyxml in Homebrew so this PR would help us a lot.

@rojkov
Copy link
Author

rojkov commented Jan 10, 2023

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.

@clalancette
Copy link
Contributor

Closing in favor of #178

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet