search

ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
97 stars 131 forks source link

Switch from TinyXML to TinyXML2 - rebase #178

Closed SMillerDev closed 9 months ago

SMillerDev commented 1 year ago

Continues the work in #99

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

SMillerDev commented 1 year ago

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

scpeters commented 1 year ago

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

clalancette commented 1 year ago

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.

SMillerDev commented 1 year ago

At least CI seems to like my last changes

rserban commented 1 year ago

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

felixf4xu commented 1 year ago

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 commented 1 year ago

In this issue (https://github.com/ros2/ros2/issues/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 commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

I'm just happy it's moving really.