Switch from TinyXML to TinyXML2 - rebase #178

SMillerDev · 2023-01-17T09:41:17Z

Continues the work in #99

I know nothing about urdfdom, but with these changes the work in that PR compiles again

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 <[email protected]>

clalancette

In short, we can't do things exactly like this. There are too many downstream projects that might depend on the TinyXML APIs.

Instead, what we should do is to add TinyXML2 APIs alongside the TinyXML ones, and make the TinyXML ones as deprecated. Once that is done, and has been released, we can then consider removing the TinyXML APIs.

Does that make sense?

SMillerDev · 2023-01-17T22:26:24Z

How do you feel about leaving it up to the packager. Essentially cmake would decide what to enable

scpeters · 2023-01-18T00:58:10Z

How do you feel about leaving it up to the packager. Essentially cmake would decide what to enable

Relevant context here is that homebrew-core would like to remove the tinyxml formula. I'm guessing that @clalancette would be open to adding support for tinyxml2 and a cmake option that disables support for tinyxml

urdf_parser/src/world.cpp

clalancette · 2023-01-18T15:17:55Z

Relevant context here is that homebrew-core would like to remove the tinyxml formula. I'm guessing that @clalancette would be open to adding support for tinyxml2 and a cmake option that disables support for tinyxml

Yes, that would be OK with me. Though I would say that we should have the default be that both TinyXML and TinyXML2 APIs are available, with a CMake option that explicitly disables the TinyXML ones. That is most consistent with how we generally tick-tock APIs.

That said, @scpeters is the maintainer here so I'll defer final approval to his judgement.

Require tinyxml2 and allow tinyxml support to be disabled. Signed-off-by: Steve Peters <[email protected]>

traversaro · 2023-02-14T10:32:35Z

urdf_parser/include/urdf_parser/urdf_parser.h

+  URDFDOM_DLLAPI bool parseCamera(Camera&, tinyxml2::XMLElement*);
+  URDFDOM_DLLAPI bool parseRay(Ray&, tinyxml2::XMLElement*);
+  URDFDOM_DLLAPI bool parseSensor(Sensor&, tinyxml2::XMLElement*);
+  URDFDOM_DLLAPI bool parseModelState(ModelState&, tinyxml2::XMLElement*);


Could it make sense to take the occasion and remove these functions that refer to tinyxml2 symbols from the public interface, to avoid in the future all the problems that we have now in the tinyxml --> tinyxml2 transition?

We could add bool exportURDFToString(const ModelInterface &model,, std::string xml_string) and bool exportURDFToFile(const ModelInterface &model,const std::string& path) to mirror the existing parseURDF and parseURDFFile . If downstream users need the tinyxml2::XMLDocument, they can always re-parse the string.

I'd like to suggest the community picks this up since that seems like a decision that needs more knowledge about the tool than I as a mere packager of the tool possess.

Sure @SMillerDev . My comment is indeed more directed towards @scpeters and @clalancette .

@traversaro yes, I think we could get rid of the APIs that write to a tinyxml2::XMLDocument. We'd still have the parse* methods that read from tinyxml2::Element* pointers. Do you have any suggestion for those? I guess since it's a raw pointer, we could use forward declarations and remove the include of tinyxml2.h?

Either fwd declare, or simply keep them only as private functions?

Definitely better!

@clalancette and @sloretz what do you think of this API approach?

@scpeters I'm a little confused; it looks like 092b57c isn't on this branch. Is that meant to be?

it's not on the branch at the moment as this PR is from @SMillerDev's branch. I'm just getting some feedback before pushing it

@clalancette I just it to this branch

SMillerDev · 2023-02-23T15:36:24Z

At least CI seems to like my last changes

Signed-off-by: Steve Peters <[email protected]>

clalancette

In short, I like the direction here. I've left a few pieces of feedback of things to fix.

All of that said, I'd prefer if we held off on merging this until after Iron is released (May of this year). I can forsee some possible packaging problems with this setup, and I want to make sure we aren't introducing them while we are trying to stabilize Iron.

cmake/FindTinyXML.cmake

CMakeLists.txt

clalancette · 2023-03-30T19:19:39Z

cmake/FindTinyXML2.cmake

Do we need this file at all? I think that TinyXML2 typically ships with a valid, working CMake file, so this seems redundant. But I could be missing something here.

I can see if we can do without it, this was taken from earlier PRs

I'm guessing it was copied from the approach used for TinyXML but may not be necessary for TinyXML2

I think it is related to the distro(s) we are targeting, for example looking at Ubuntu LTS, 18.04, 20.04 and 22.04 do not have the tinyxml2-config.cmake (so find_package(tinyxml2) does not work out of the box):

https://packages.ubuntu.com/bionic/amd64/libtinyxml2-dev/filelist

https://packages.ubuntu.com/focal/amd64/libtinyxml2-dev/filelist

https://packages.ubuntu.com/jammy/amd64/libtinyxml2-dev/filelist
while 22.10 has tinyxml2-config.cmake (probably because they switched from using the autotools build system to the cmake one to compile tinyxml2 itself):

https://packages.ubuntu.com/kinetic/amd64/libtinyxml2-dev/filelist

A problem with FindTinyXML2.cmake as provided is that it does not properly deal with multi-config generators. I want to link urdfdom and dependencies to another project and be able to build that in both Release and Debug (Visual Studio on Windows). I was able to do that only by using tinyxml2-config.cmake.

Platforms that need to support multi-config generators I guess that for sure will ship tinyxml2-config.cmake. Probably the best way to support both platforms with tinyxml2-config.cmake and platforms without is to add a find_package(tinyxml2 NO_MODULE) at the top of the FindTinyXML2.cmake , and in case the package is found, just put tinyxml2::tinyxml2 in TinyXML2_LIBRARIES.

Not entirely sure what this entails, could you make a code suggestion?

CMakeLists.txt

urdf_parser/src/joint.cpp

urdf_parser/src/model.cpp

Co-authored-by: Chris Lalancette <[email protected]>

rserban · 2023-04-11T12:55:02Z

I suggest marking variables related to tinyxml2 as non-advanced when DISABLE_TINYXML_SUPPORT is set to 'ON'

felixf4xu · 2023-05-25T03:15:42Z

In short, we can't do things exactly like this. There are too many downstream projects that might depend on the TinyXML APIs.

I don't quite understand this.

If a downstream project depends on TinyXML, they can still use tinyxml directly, providing linkage to tinyxml is not urdfdom's responsibility.

BTW, if you check the code of urdf (https://github.com/ros2/urdf/blob/humble/urdf/CMakeLists.txt#L9), it depends on tinyxml2 only, never worried about downstream project that needs tinyxml.

felixf4xu · 2023-05-25T03:22:54Z

In this issue (ros2/ros2#987), urdfdom is the only remaining project that still use tinyxml, the concern that "many downstream projects that might depend on the TinyXML" is not very valid.

if urdfrom upgraded to tinyxml2, tinyxml might be removed out from rosdsitro completely, I guess.

Oh, I'm sorry and I realized urdfdom is a project both of ros and ros2. So I edited this comment just for anyone interested.
ros will not be maintained ros2 is the only target for new code so the comment above still makes some sense.

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

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

clalancette · 2023-11-29T15:50:29Z

@SMillerDev After talking about this with @scpeters and @sloretz , and getting some feedback from the community, I think we are going to go with the simple solution of just breaking the API. That is, we are just going to convert TinyXML APIs to TinyXML2, and do a new major version release.

I apologize for having set you on the path in this PR, which just ended up being too complicated for not enough gain. But I also want to thank you for your perseverance in getting this done. I'm going to close this in favor of #186, and continue development there.

SMillerDev · 2023-11-29T20:56:35Z

I'm just happy it's moving really.

Dmitry Rozhkov and others added 2 commits January 17, 2023 10:15

Switch from TinyXML to TinyXML2

9c68be5

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 <[email protected]>

chore: rebase

5ba4be4

SMillerDev mentioned this pull request Jan 17, 2023

tinyxml: deprecate Homebrew/homebrew-core#119829

Closed

6 tasks

clalancette requested changes Jan 17, 2023

View reviewed changes

clalancette mentioned this pull request Jan 17, 2023

Switch from TinyXML to TinyXML2 #99

Closed

convert world.cpp to conditional tinyxml2

b9aebf1

SMillerDev commented Jan 18, 2023

View reviewed changes

urdf_parser/src/world.cpp Outdated Show resolved Hide resolved

Add DISABLE_TINYXML_SUPPORT cmake option

a65ae22

Require tinyxml2 and allow tinyxml support to be disabled. Signed-off-by: Steve Peters <[email protected]>

SMillerDev requested a review from scpeters February 8, 2023 19:05

traversaro mentioned this pull request Feb 14, 2023

CMakeLists.txt: Some fixes for Relocatable package #179

Merged

traversaro reviewed Feb 14, 2023

View reviewed changes

SMillerDev force-pushed the tinyxml2 branch from 5fff92f to 1458ccd Compare February 14, 2023 13:22

urdf_parser: convert remaining files to HAVE_TINYXML

0bca496

SMillerDev force-pushed the tinyxml2 branch from 1458ccd to 0bca496 Compare February 14, 2023 13:42

Remove tinyxml2 header from public API

092b57c

Signed-off-by: Steve Peters <[email protected]>

clalancette requested changes Mar 30, 2023

View reviewed changes

Apply suggestions from code review

3e215fc

Co-authored-by: Chris Lalancette <[email protected]>

Fix comments, don't use tinyxml2 namespace

aca8250

SMillerDev requested a review from clalancette May 7, 2023 11:57

clalancette closed this Nov 29, 2023

This was referenced Dec 6, 2023

Upgrade from tinyxml to tinyxml2 #186

Merged

Remove tinyxml2 from public API #189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from TinyXML to TinyXML2 - rebase #178

Switch from TinyXML to TinyXML2 - rebase #178

SMillerDev commented Jan 17, 2023

clalancette left a comment

SMillerDev commented Jan 17, 2023

scpeters commented Jan 18, 2023

clalancette commented Jan 18, 2023

traversaro Feb 14, 2023 •

edited

Loading

SMillerDev Feb 14, 2023

traversaro Feb 14, 2023

scpeters Feb 24, 2023

traversaro Feb 24, 2023

traversaro Mar 20, 2023

scpeters Mar 20, 2023

clalancette Mar 20, 2023

scpeters Mar 20, 2023

scpeters Mar 30, 2023

SMillerDev commented Feb 23, 2023

clalancette left a comment

clalancette Mar 30, 2023

SMillerDev Apr 4, 2023

scpeters Apr 4, 2023

traversaro Apr 6, 2023

rserban Apr 11, 2023

traversaro Apr 11, 2023

SMillerDev Apr 18, 2023

rserban commented Apr 11, 2023

felixf4xu commented May 25, 2023

felixf4xu commented May 25, 2023 •

edited

Loading

ros-discourse commented Nov 20, 2023

clalancette commented Nov 29, 2023

SMillerDev commented Nov 29, 2023

Switch from TinyXML to TinyXML2 - rebase #178

Switch from TinyXML to TinyXML2 - rebase #178

Conversation

SMillerDev commented Jan 17, 2023

clalancette left a comment

Choose a reason for hiding this comment

SMillerDev commented Jan 17, 2023

scpeters commented Jan 18, 2023

clalancette commented Jan 18, 2023

traversaro Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SMillerDev commented Feb 23, 2023

clalancette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rserban commented Apr 11, 2023

felixf4xu commented May 25, 2023

felixf4xu commented May 25, 2023 • edited Loading

ros-discourse commented Nov 20, 2023

clalancette commented Nov 29, 2023

SMillerDev commented Nov 29, 2023

traversaro Feb 14, 2023 •

edited

Loading

felixf4xu commented May 25, 2023 •

edited

Loading