ros / urdfdom

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

urdfdom should find_package(TinyXML) in urdfdom-config.cmake #120

Open sloretz opened 5 years ago

sloretz commented 5 years ago

urdfdom finds TinyXML when it is being built, but it does not find it again when it is installed. https://github.com/ros/urdfdom/blob/1857a55897295c1fa46669bf1483dc78cf2467a8/CMakeLists.txt#L35

Since tinyxml.h is included in a public header https://github.com/ros/urdfdom/blob/1857a55897295c1fa46669bf1483dc78cf2467a8/urdf_parser/include/urdf_parser/urdf_parser.h#L42

it should also be find_package'd in urdfdom-config.cmake and the include directories should be added to urdfdom_INCLUDE_DIRS. Currently it hard codes the path to TinyXML headers when urdfdom was built.

https://github.com/ros/urdfdom/blob/1857a55897295c1fa46669bf1483dc78cf2467a8/cmake/urdfdom-config.cmake.in#L6

This package has uses its own find module for tinyxml, so that will need to be installed so it can be used from urdfdom-config.cmake https://github.com/ros/urdfdom/blob/master/cmake/FindTinyXML.cmake

Bionic urdfdom-config.cmake ```cmake if (urdfdom_CONFIG_INCLUDED) return() endif() set(urdfdom_CONFIG_INCLUDED TRUE) set(urdfdom_INCLUDE_DIRS "/usr/include" "/usr/include") foreach(lib urdfdom_sensor;urdfdom_model_state;urdfdom_model;urdfdom_world) set(onelib "${lib}-NOTFOUND") find_library(onelib ${lib}) if(NOT onelib) message(FATAL_ERROR "Library '${lib}' in package urdfdom is not installed properly") endif() list(APPEND urdfdom_LIBRARIES ${onelib}) endforeach() foreach(dep urdfdom_headers;console_bridge) if(NOT ${dep}_FOUND) find_package(${dep}) endif() list(APPEND urdfdom_INCLUDE_DIRS ${${dep}_INCLUDE_DIRS}) list(APPEND urdfdom_LIBRARIES ${${dep}_LIBRARIES}) endforeach() ```


traversaro commented 5 years ago

If the final goal is a relocatable package that does not hardcode the location of TinyXML nor the install location of urdfdom, during the build, it probably make sense to make sure that the FindTinyXML.cmake installed by urdfdom creates an imported target, and urdfdom creates an imported target as well.

One tricky aspect is that the urdfdom-installed FindTinyXML.cmake may risk to shadow an existing FindTinyXML.cmake already used by the user, so perhaps it could make sense to use urdfdom prefixes and just add FindTinyXML.cmake to the CMAKE_MODULE_PATH only as long as strictly necessary.