CMakeLists.txt: Some fixes for Relocatable package

[DasRoteSkelett]

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:

• CMAKE_MODULE_PATH append instead of replacement
• Do not expose TinyXML Path (private instead of public)
• INTERFACE use relative path instead of ${CMAKE_INSTALL_INCLUDEDIR}

See also

https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages

for reference.

Signed-off-by: Matthias Schoepfer [email protected]

[traversaro]

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:

• Either install a FindTinyXML.cmake or depend on a package that installs it to switch to use imported targets
• Or switch to forward declare the used tinyxml classes

[DasRoteSkelett]

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 :-(

[traversaro]

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 #178 for a related change.

[clalancette]

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

[ghost]

Done

[clalancette]

Done

Thank you! Here is CI on this change:

• Linux
• Linux-aarch64
• Windows