-
Notifications
You must be signed in to change notification settings - Fork 65
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
TinyXML(2) might cause problems with the XSD schema now often attached due to the new package format (V2) #77
Comments
From your description it is unclear to me what the actual problem is. Afaik TinyXML simply ignores the schema declaration. Maybe you can clarify. |
Looking at the recommendation from W3C about embedding references to xsd in xml files (see section "A Reference to an XML Schema" here) and based on recent experience working with XML embedding schemas like the DDS ones. It seems that one solution is to update the reference to the schemas in REP140 from:
to:
That will work around the limitation of TinyXML2 and allow rospack to keep working as expected regardless of the version of Tinyxml2 used |
There are multiple ways to embed a schema. While we could change all manifest I would recommend to investigate why the previous working approach now fails with a newer version of TinyXML2 (I assume that is the reason?) and ticket the problem upstream. |
I don't know how the way to include reference to xsd was decided. Do you remember why we chose the xml-model approach and how to use it (don't see any reference of it on the W3C website)? While there are different approaches, it looks like the one recommended by W3C and used by other projects is more widespread and works using our current parser. Based on that and the small number of packages.xml referencing xsd, it looks to me that it is worth updating the handful of packages and the example in the REP to use it. |
Answering my own question : https://www.w3.org/TR/xml-model/#the-xml-model-processing-instruction |
The goal is not to make TinyXML to "support" schemas. It is only about correctly parsing the markup. |
I agree, my point is more that we won't have a fix in Yakkety and likely not have one in Zesty so we still need to touch every package using it or change rospack's parser to have a working rospack for Lunar packages |
Please try the alternative proposal with existing linters. I can't get it to e.g. pass |
It only fails with TinyXML2 version 4.0.1 which is e.g. in Ubuntu Zesty. All other versions (3.0.0 and lower as well as 5.0.0 and higher) don't have the problem and work as expected. |
looks like leethomason/tinyxml2#496 was the fix (or part of the fix need). It has been merged after 4.0.1 was tagged and is part of 5.0.0 (original issue: leethomason/tinyxml2#399) |
As proposed here #606 I sent a request to the maintainers of the libtinyxml2 package of Debian and Ubuntu. I will keep you updated if I receive an answer. Changes took place:
|
Thanks @arne48 for the initiative. |
Every other platforms or distro are shipping versions that dont have the problem so we should be good: |
It seems fixed, thanks. |
Great, thanks for testing! |
Thanks to all of you to fix this problem so fast. |
@arne48 Do you have any links for tickets against the various distributions? That might help future readers like https://answers.ros.org/question/278733/rospack-find-throws-exception-error/ to determine the state. |
@dirk-thomas Debian Stretch: Ubuntu 17.04: Gentoo: The biggest problem at the moment might be armhf on Stretch. I don't know if armhf is a big issue for Ubuntu as I saw a lot distributions use either Stretch or Xenial. Maybe we can also provide a deb-package for armhf on Stretch. Now I contacted Chow Loong Jin again. |
@mikaelarguedas can you suggest some patch for armhf debian stretch for the above error. |
@kamalpandey1993 The patch I applied and released in ROS is available at https://github.com/mikaelarguedas/tinyxml2/tree/4.0.1-patch (see #77 (comment)). |
Stretch Update: To my understanding the last devices that can be effected are armhf SBCs running a Stretch-based distro (aka. RPi) since Ubuntu 17.04 is now out-of-support and @mikaelarguedas provided packages for arm64 and amd64. |
I managed to get everything up and running on Debian Stretch and armhf by adding the Buster mirror to my /etc/apt/sources.list file, running sudo apt-get install libtinyxml2-dev, then doing a clean build of the entire ros workspace. You still have to install every ros package by cloning them from Git repos, but at least it's possible to make everything run. |
I am running melodic ROS on raspberry pi and debian stretch OS. My package.xml file for rospack has states 2.5.2. I had the same issue regarding rospack, it would return the same error as #86. As advised the error was due to libtinyxml2 being version 4.0.1-1. I tried to follow @jamesbellaero advice and included a buster mirror then I'm out of ideas, please help~~ |
I am not sure if you have seen the ROS support guidelines but we kindly ask to raise questions like this on answers.ros.org instead. The rational for asking all kind of questions in a single place is:
If you have asked your question on answers.ros.org please feel free to add a link to the question to this ticket so that future readers can find the related question. |
@dirk-thomas Noted with thanks! |
This is more a general problem caused by the wide use of TinyXML(2).
But to me it became the most apparent in rospack as this package provides the commandline tool and basic library to deal with ROS packages.
The problem is based on the lightweight approach of TinyXML(2) in which they don't support the parsing of schemas. see What it doesn't do
This property combined with the provided schema in REP140 led several developers to add it into their package.xmls.
In my minimal installation I found that this was already done in pluginlib, nodelet_core, bondpy, test_bond, bondcpp, bond_core, smclib, dynamic_reconfigure, bond and class_loader.
The updates containing the schema are quite new and I don't know if their are any changes already planned to address this potential issue.
According particularly to rospack one solution would be to switch to a more complex parser which supports schemas. But this will obviously not affect packages which don't use rospack but using TinyXML(2) themselves to parse package.xmls.
A more global approach would be to avoid the addition of the schema to the package.xml and maybe provide a dedicated library to verify them if needed. Doing so would preserve the wide usage of TinyXML(2).
The text was updated successfully, but these errors were encountered: