Closed DasRoteSkelett closed 10 months ago
The problem is that tinyxml.h is actually included in public header, see https://github.com/ros/urdfdom/blob/3.1.0/urdf_parser/include/urdf_parser/urdf_parser.h#L44 . This change would break compilation if tinyxml is not installed in a system prefix. Possible fixes are:
So, one could REQUIRE
tinyxml_vendor, as it installs a FindTinyXML.cmake?
Generally, I find it a bit awkward that tinyxml types are exposed in the public api (especially, because exchanging the xml parser is now difficult). But replacing this would be a breaking change :-(
So, one could REQUIRE tinyxml_vendor, as it installs a FindTinyXML.cmake?
In theory, yes. In practice, it is a bit awkward as urdfdom
is a library that is tipically packaged outside of the usual ros packaging (see for example https://repology.org/project/urdfdom/versions), while tinyxml_vendor
is a ROS2-only package, that has a "strange" name that has a specific meaning as part of ROS2, but it would be a bit strange to package for general distributions.
Generally, I find it a bit awkward that tinyxml types are exposed in the public api (especially, because exchanging the xml parser is now difficult). But replacing this would be a breaking change :-(
Yes, I totally agree. See https://github.com/ros/urdfdom/pull/178 for a related change.
@DasRoteSkelett With #186 done, some of this is no longer necessary, but some of it is. If you are willing to rebase this to remove the bits that are conflicting, we can consider getting this in.
Done
The overall rationale is, that the current behavior is not well suited for cross-compiling or relocation of the package. This commit shall address some of the issues found.
There are some absolute path in the generation of the .cmake files which ruin the creation of relocatable packages. Fixing:
See also
https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages
for reference.
Signed-off-by: Matthias Schoepfer m.schoepfer@rethinkrobotics.com