Upgrade from tinyxml to tinyxml2 #186

felixf4xu · 2023-05-25T03:04:29Z

my attempt to address the issue #185

ros-discourse · 2023-11-20T18:51:08Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/upcoming-api-break-in-urdfdom/34750/1

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

urdf_parser/include/urdf_parser/urdf_parser.h

CMakeLists.txt

traversaro · 2023-11-29T16:13:58Z

I think we also need to modify the following line to change the reference to @TinyXML_INCLUDE_DIRS@:

urdfdom/cmake/urdfdom-config.cmake.in

Line 6 in 6d3c5de

    
           set(@PKG_NAME@_INCLUDE_DIRS "${@PROJECT_NAME@_DIR}/@RELATIVE_PATH_CMAKE_DIR_TO_PREFIX@/@CMAKE_INSTALL_INCLUDEDIR@" "@TinyXML_INCLUDE_DIRS@")

. The urdfdom's CMake machinery is a relic from the past, but for users not using tinyxml-related methods everything works fine only because we manually export the tinyxml include dirs there. I can prepare a quick PR to handle this.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-11-29T16:21:37Z

@traversaro You read my mind with almost all of your feedback; I've addressed most of it in 2f45917 and 06a18de.

As for your comment in #186 (comment) ; if you'd just like to post a diff of what you are thinking about here, I'm happy to merge it in.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

traversaro · 2023-11-29T16:38:18Z

As for your comment in #186 (comment) ; if you'd just like to post a diff of what you are thinking about here, I'm happy to merge it in.

Actually my comment was a bit outdated by your changes. I updated the modifications in felixf4xu#2, basically if tinyxml2::tinyxml2 is not a public linked library, we need to make sure that it is defined in urdfdom-config.cmake before the urdfdom::urdfdom_* libraries imported target are defined.

traversaro · 2023-11-29T16:42:57Z

As for your comment in #186 (comment) ; if you'd just like to post a diff of what you are thinking about here, I'm happy to merge it in.

Actually my comment was a bit outdated by your changes. I updated the modifications in felixf4xu#2, basically if tinyxml2::tinyxml2 is not a public linked library, we need to make sure that it is defined in urdfdom-config.cmake before the urdfdom::urdfdom_* libraries imported target are defined.

Ok, this is partially outdated by 2383a0a . Adding FindTinyXML2.cmake locally does not help in systems without either tinyxml2_vendor or tinyxml2-config.cmake installed, as find_package(urdfdom) will fail if tinyxml2::tinyxml2 is not defined. A possible alternative is to also install FindTinyXML2.cmake, and temporary add it to the CMAKE_MODULE_PATH before the find_package(TinyXml2) call.

The former doesn't exist in older TinyXML2, and is just a convenience function anyway. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-11-29T18:59:55Z

Ok, this is partially outdated by 2383a0a . Adding FindTinyXML2.cmake locally does not help in systems without either tinyxml2_vendor or tinyxml2-config.cmake installed, as find_package(urdfdom) will fail if tinyxml2::tinyxml2 is not defined. A possible alternative is to also install FindTinyXML2.cmake, and temporary add it to the CMAKE_MODULE_PATH before the find_package(TinyXml2) call.

I've merged in felixf4xu#2 to this PR, though I'm not totally convinced by the find_package(tinyxml2_vendor); I think that will only exist in ROS 2 land. I'm more inclined to go with your other idea of installing FindTinyXML2.cmake and calling that, but I'm not 100% sure how to do that. I'll poke at it a bit more here.

clalancette · 2023-11-29T19:10:51Z

@traversaro Relatedly, do you have a link to a non-ROS package that uses urdfdom? I'd like to have something concrete to test against.

traversaro · 2023-11-29T19:13:06Z

@traversaro Relatedly, do you have a link to a non-ROS package that uses urdfdom? I'd like to have something concrete to test against.

Probably a quite relevant example of such package may be sdformat (https://github.com/gazebosim/sdformat).

traversaro · 2023-11-29T19:24:13Z

Ok, this is partially outdated by 2383a0a . Adding FindTinyXML2.cmake locally does not help in systems without either tinyxml2_vendor or tinyxml2-config.cmake installed, as find_package(urdfdom) will fail if tinyxml2::tinyxml2 is not defined. A possible alternative is to also install FindTinyXML2.cmake, and temporary add it to the CMAKE_MODULE_PATH before the find_package(TinyXml2) call.

I've merged in felixf4xu#2 to this PR, though I'm not totally convinced by the find_package(tinyxml2_vendor); I think that will only exist in ROS 2 land. I'm more inclined to go with your other idea of installing FindTinyXML2.cmake and calling that, but I'm not 100% sure how to do that. I'll poke at it a bit more here.

Yes, that is correct. In the corrent form, this works:

If tinyxml2_vendor is installed (case in ROS)
If tinyxml2 is installed via cmake, and so it installs tinyxml2-config.cmake (case of tinyxml2 in conda-forge)

However, it does not work with Ubuntu with apt dependencies, where libtinyxml2-dev package does not contain any tinyxml2-config.cmake. I was not thinking to this case.

Use the provided CMake primitives for this, which should make it work in all situations. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-11-29T21:36:27Z

With an assist from @sloretz , I updated this PR with b592bbd, which generates the necessary config file changes. I can't make this fail in any situation now; it always seems to do the right thing for me. But any additional testing @traversaro is much appreciated.

traversaro · 2023-11-29T22:28:57Z

Thanks! I will check this tomorrow (European) morning.

traversaro · 2023-11-30T08:47:24Z

Ok, I reproduced the problem I was referring to in earlier messages (with the example project https://gist.github.com/traversaro/c79f69e2fe5e0d2b7c1405ee973c2058, tested on Ubuntu 22.04 with apt dependencies):

sudo apt install build-essential libtinyxml2-dev liburdfdom-headers-dev
mkdir -p urdfdom_ws/src
cd urdfdom_ws/src
git clone -b tinyxml2 https://github.com/felixf4xu/urdfdom/
git clone https://gist.github.com/traversaro/c79f69e2fe5e0d2b7c1405ee973c2058 urdfdom_test
cd ..
colcon build

this fails with:

CMake Warning at /home/traversaro/urdfdom_ws/install/urdfdom/lib/urdfdom/cmake/urdfdom-config.cmake:62 (find_package):
  By not providing "FindTinyXML2.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "TinyXML2",
  but CMake did not find one.

  Could not find a package configuration file provided by "TinyXML2" with any
  of the following names:

    TinyXML2Config.cmake
    tinyxml2-config.cmake

  Add the installation prefix of "TinyXML2" to CMAKE_PREFIX_PATH or set
  "TinyXML2_DIR" to a directory containing one of the above files.  If
  "TinyXML2" provides a separate development package or SDK, be sure it has
  been installed.
Call Stack (most recent call first):
  CMakeLists.txt:5 (find_package)


CMake Error at CMakeLists.txt:9 (add_executable):
  Target "dummy" links to target "tinyxml2::tinyxml2" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?


CMake Error at CMakeLists.txt:9 (add_executable):
  Target "dummy" links to target "tinyxml2::tinyxml2" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?


CMake Error at CMakeLists.txt:9 (add_executable):
  Target "dummy" links to target "tinyxml2::tinyxml2" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

It turns out that sdformat is not actually a great example to see this problem, as it also uses tinyxml2 internally, and so in that case tinyxml2::tinyxml2 is already defined before the find_package(urdfdom) call.

clalancette · 2023-11-30T19:40:37Z

Ok, I reproduced the problem I was referring to in earlier messages (with the example project https://gist.github.com/traversaro/c79f69e2fe5e0d2b7c1405ee973c2058, tested on Ubuntu 22.04 with apt dependencies):

Ah, thanks for that. I didn't see this problem because, while I did an extremely similar test yesterday, I did it on Fedora 38. And there, the tinyxml-devel package actually provides the proper .cmake, while the Ubuntu one doesn't.

In any case, we should fix this, so I'll look at installing the FindTinyXML2.cmake file as we discussed earlier.

This is so downstream projects can always find it. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-11-30T19:59:44Z

All right, 0e61d1f should fix that particular problem. I have a few other tests to run on it (like on ROS 2), but another look here is much appreciated.

traversaro · 2023-12-01T08:25:28Z

Ok, I reproduced the problem I was referring to in earlier messages (with the example project https://gist.github.com/traversaro/c79f69e2fe5e0d2b7c1405ee973c2058, tested on Ubuntu 22.04 with apt dependencies):

Ah, thanks for that. I didn't see this problem because, while I did an extremely similar test yesterday, I did it on Fedora 38. And there, the tinyxml-devel package actually provides the proper .cmake, while the Ubuntu one doesn't.

Probably also fedora switched to build tinyxml2 via cmake like done in conda-forge.

traversaro · 2023-12-01T08:26:24Z

All right, 0e61d1f should fix that particular problem. I have a few other tests to run on it (like on ROS 2), but another look here is much appreciated.

It seems fine. I did a small suggestion to reduce the risk of conflict shadowing with other installed FindTinyXML2.cmake files, but even that version should be working fine for now.

cmake/urdfdom-config.cmake.in

So we don't permanently manipulate it. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-12-03T19:07:43Z

Here's preliminary ROS 2 CI on this change:

Linux
Linux-aarch64
Windows
Windows Debug
RHEL
Clang

scpeters · 2023-12-06T18:21:01Z

urdf_parser/include/urdf_parser/urdf_parser.h

-  URDFDOM_DLLAPI bool parseSensor(Sensor&, TiXmlElement*);
-  URDFDOM_DLLAPI bool parseModelState(ModelState&, TiXmlElement*);
+  URDFDOM_DLLAPI tinyxml2::XMLDocument*  exportURDF(ModelInterfaceSharedPtr &model);
+  URDFDOM_DLLAPI tinyxml2::XMLDocument*  exportURDF(const ModelInterface &model);


I just remembered that we had planned some changes to the public API in #178 so that tinyxml2.h wouldn't need to be included from this header file. We can address those in a separate pull request, so that this is a cleaner tinyxml -> tinyxml2 replacement

added an issue for this: #189

clalancette · 2023-12-06T19:56:38Z

All right. With the latest reviews, I'm going to go ahead and merge this one. There is some follow-up work to do here, so I'm not going to do the 4.0.0 release right now, but I'll keep it on my list to do in the next couple of weeks.

@felixf4xu Thanks for getting this started, @traversaro thanks for the feedback, and @scpeters thanks for the review!

* upgrade from tinyxml to tinyxml2 * Remove 'using tinyxml2' * Update github actions. * Add in FindTinyXML2 for systems that don't have it. * Make sure that tinyxml2::tinyxml2 is available in downstream packages * Update urdfdom-config.cmake.in * Replace InsertNewChildElement with GetDocument()->NewElement. The former doesn't exist in older TinyXML2, and is just a convenience function anyway. * Fix generation of CMake config files. Use the provided CMake primitives for this, which should make it work in all situations. * Install the FindTinyXML2.cmake file. This is so downstream projects can always find it. * Backup and restore the CMAKE_MODULE_PATH. So we don't permanently manipulate it. Signed-off-by: Chris Lalancette <clalancette@gmail.com> Co-authored-by: Silvio Traversaro <silvio@traversaro.it> Signed-off-by: harleylara <harley.lara@outlook.com>

clalancette mentioned this pull request Nov 29, 2023

Switch from TinyXML to TinyXML2 - rebase #178

Closed

felixf4xu and others added 2 commits November 29, 2023 15:51

upgrade from tinyxml to tinyxml2

9fd9cf6

More cleanups.

06a18de

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

traversaro reviewed Nov 29, 2023

View reviewed changes

urdf_parser/include/urdf_parser/urdf_parser.h Outdated Show resolved Hide resolved

traversaro reviewed Nov 29, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

traversaro reviewed Nov 29, 2023

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Remove 'using tinyxml2'

2f45917

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette force-pushed the tinyxml2 branch from bcdaba7 to 2f45917 Compare November 29, 2023 16:20

Update github actions.

f2a2ce5

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette force-pushed the tinyxml2 branch from 14fe181 to f2a2ce5 Compare November 29, 2023 16:29

clalancette and others added 2 commits November 29, 2023 16:34

Add in FindTinyXML2 for systems that don't have it.

2383a0a

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Make sure that tinyxml2::tinyxml2 is available in downstream packages

a68d9a8

traversaro mentioned this pull request Nov 29, 2023

Make sure that tinyxml2::tinyxml2 is available in downstream packages felixf4xu/urdfdom#2

Merged

Update urdfdom-config.cmake.in

bef2860

clalancette added 3 commits November 29, 2023 18:50

Replace InsertNewChildElement with GetDocument()->NewElement.

a448c63

The former doesn't exist in older TinyXML2, and is just a convenience function anyway. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Remove another use of using tinyxml2.

713e457

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Merge remote-tracking branch 'traversaro/patch-3' into tinyxml2

2c58e3f

Fix generation of CMake config files.

b592bbd

Use the provided CMake primitives for this, which should make it work in all situations. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Install the FindTinyXML2.cmake file.

0e61d1f

This is so downstream projects can always find it. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

traversaro reviewed Dec 1, 2023

View reviewed changes

cmake/urdfdom-config.cmake.in Show resolved Hide resolved

Backup and restore the CMAKE_MODULE_PATH.

23ea44e

So we don't permanently manipulate it. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

scpeters changed the title ~~upgrade from tinyxml to tinyxml2~~ Upgrade from tinyxml to tinyxml2 Dec 1, 2023

azeey mentioned this pull request Dec 1, 2023

Update the version requirement on urdfdom so it works with version 4.0 gazebosim/sdformat#1350

Open

traversaro approved these changes Dec 4, 2023

View reviewed changes

traversaro mentioned this pull request Dec 4, 2023

Do not install CMake config files in a different directory on Windows #188

Merged

scpeters approved these changes Dec 6, 2023

View reviewed changes

scpeters reviewed Dec 6, 2023

View reviewed changes

This was referenced Dec 6, 2023

Use tinyxml2 instead of tinyxml #185

Closed

Remove tinyxml2 from public API #189

Closed

clalancette linked an issue Dec 6, 2023 that may be closed by this pull request

Use tinyxml2 instead of tinyxml #185

Closed

clalancette merged commit 18325be into ros:master Dec 6, 2023
6 of 7 checks passed

This was referenced Dec 6, 2023

CMakeLists.txt: Some fixes for Relocatable package #179

Merged

Add cmake version export #145

Closed

maltewi mentioned this pull request Dec 16, 2023

use tinyxml2 instead of tinyxml ARC-OPT/wbc#85

Open

jiaxshi mentioned this pull request Jun 28, 2024

{humble}: fix host path is included in cmake files. ros/meta-ros#1192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade from tinyxml to tinyxml2 #186

Upgrade from tinyxml to tinyxml2 #186

felixf4xu commented May 25, 2023

ros-discourse commented Nov 20, 2023

traversaro commented Nov 29, 2023 •

edited

Loading

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 29, 2023

clalancette commented Nov 29, 2023

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 29, 2023

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 30, 2023 •

edited

Loading

clalancette commented Nov 30, 2023

clalancette commented Nov 30, 2023

traversaro commented Dec 1, 2023

traversaro commented Dec 1, 2023

clalancette commented Dec 3, 2023

scpeters Dec 6, 2023

scpeters Dec 6, 2023

clalancette commented Dec 6, 2023

Upgrade from tinyxml to tinyxml2 #186

Upgrade from tinyxml to tinyxml2 #186

Conversation

felixf4xu commented May 25, 2023

ros-discourse commented Nov 20, 2023

traversaro commented Nov 29, 2023 • edited Loading

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 29, 2023

clalancette commented Nov 29, 2023

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 29, 2023

clalancette commented Nov 29, 2023

traversaro commented Nov 29, 2023

traversaro commented Nov 30, 2023 • edited Loading

clalancette commented Nov 30, 2023

clalancette commented Nov 30, 2023

traversaro commented Dec 1, 2023

traversaro commented Dec 1, 2023

clalancette commented Dec 3, 2023

scpeters Dec 6, 2023

Choose a reason for hiding this comment

scpeters Dec 6, 2023

Choose a reason for hiding this comment

clalancette commented Dec 6, 2023

traversaro commented Nov 29, 2023 •

edited

Loading

traversaro commented Nov 30, 2023 •

edited

Loading