Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove tinyxml2 from public dependencies. #190

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

clalancette
Copy link
Contributor

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 SMillerDev@092b57c

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.

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

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

ROS 2 CI:

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

@traversaro
Copy link
Contributor

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
Copy link
Contributor

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

add_library(${add_urdfdom_library_LIBNAME} SHARED
, 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
Copy link
Contributor Author

, 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.

@clalancette clalancette merged commit fb7f8cb into master Dec 8, 2023
12 checks passed
@clalancette clalancette deleted the clalancette/tinyxml2-private branch December 8, 2023 13:42
harleylara pushed a commit to traversaro/urdfdom that referenced this pull request Feb 15, 2024
That way, we don't have to export the tinyxml2 dependencies
to downstream consumers.  It is just a private dependency
at that point.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: harleylara <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tinyxml2 from public API
3 participants