Upgrade from tinyxml to tinyxml2

[felixf4xu]

my attempt to address the issue https://github.com/ros/urdfdom/issues/185

[ros-discourse]

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

[traversaro]

I think we also need to modify the following line to change the reference to @TinyXML_INCLUDE_DIRS@: https://github.com/ros/urdfdom/blob/6d3c5de088359523857befbe730dbdd5543e35ba/cmake/urdfdom-config.cmake.in#L6 . 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.

[clalancette]

@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 https://github.com/ros/urdfdom/pull/186#issuecomment-1832219702 ; if you'd just like to post a diff of what you are thinking about here, I'm happy to merge it in.

[traversaro]

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 https://github.com/felixf4xu/urdfdom/pull/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]

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 https://github.com/ros/urdfdom/pull/186/commits/2383a0a429ee9ff3540d4cba8ab6100c26ea692b . 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.

[clalancette]

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 https://github.com/felixf4xu/urdfdom/pull/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]

@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]

@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]

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.

[clalancette]

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]

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

[traversaro]

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]

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.

[clalancette]

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]

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]

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.

[clalancette]

Here's preliminary ROS 2 CI on this change:

• Linux
• Linux-aarch64
• Windows
• Windows Debug
• RHEL
• Clang

[clalancette]

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!