search

ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
96 stars 132 forks source link

Remove tinyxml2 from public dependencies. #190

Closed clalancette closed 10 months ago

clalancette commented 10 months ago

That way, we don't have to export the tinyxml2 dependencies to downstream consumers. It is just a private dependency at that point.

Based on the code in https://github.com/SMillerDev/urdfdom/commit/092b57c20bca37d1d81eaa6da6b08a7bebb04662

Closes #189

@scpeters @sloretz Thoughts from both of you appreciated.

@traversaro If you have time, any testing you can do on this is highly appreciated.

clalancette commented 10 months ago

ROS 2 CI:

traversaro commented 10 months ago

Thanks @clalancette ! One problem of using imported target for linking, is that even if the library is linked as PRIVATE, if the library is compiled is STATIC, you still need to make sure that any PRIVATE-linked target is found in the <package>-config.cmake . This is due to the fact that if you have a downstream executable that links a urdfdom library, it also needs to link the tinyxml2 library (that can be either static or shared).

However, looking at the systems for which urdfdom is packaged (https://repology.org/project/urdfdom/versions) this is just a problem of *-static triplets of vcpkg (or perhaps conan, but I do not have an experience with that), so I think we can safely assume that if one wants to build urdfdom as static (i.e. BUILD_SHARED_LIBS=OFF) it is on a system in which tinyxml2 installs a tinyxml2-config.cmake file. In that case, the required modifications are minimal.

traversaro commented 10 months ago

Thanks @clalancette ! One problem of using imported target for linking, is that even if the library is linked as PRIVATE, if the library is compiled is STATIC, you still need to make sure that any PRIVATE-linked target is found in the <package>-config.cmake . This is due to the fact that if you have a downstream executable that links a urdfdom library, it also needs to link the tinyxml2 library (that can be either static or shared).

However, looking at the systems for which urdfdom is packaged (https://repology.org/project/urdfdom/versions) this is just a problem of *-static triplets of vcpkg (or perhaps conan, but I do not have an experience with that), so I think we can safely assume that if one wants to build urdfdom as static (i.e. BUILD_SHARED_LIBS=OFF) it is on a system in which tinyxml2 installs a tinyxml2-config.cmake file. In that case, the required modifications are minimal.

Actually we are currently hardcoding libraries to be SHARED in https://github.com/ros/urdfdom/blob/10093ba12c74e64977ab6620c2da99db823a10cf/urdf_parser/CMakeLists.txt#L6, so clearly we are not supporting the use case of building static libraries, so I think the PR is good to go as it is.

clalancette commented 10 months ago

, so clearly we are not supporting the use case of building static libraries, so I think the PR is good to go as it is.

Thanks Silvio! If we want to support that in the future I'll guess we'll have to do some additional modifications, but I'll go ahead and merge this one as-is for now.