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

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

Open
sloretz opened this issue Jan 23, 2019 · 1 comment
Open

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

sloretz opened this issue Jan 23, 2019 · 1 comment

Comments

@sloretz
Copy link
Contributor

sloretz commented Jan 23, 2019

urdfdom finds TinyXML when it is being built, but it does not find it again when it is installed.

find_package(TinyXML REQUIRED)

Since tinyxml.h is included in a public header

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.

set(@PKG_NAME@_INCLUDE_DIRS "@CMAKE_INSTALL_PREFIX@/include" "@TinyXML_INCLUDE_DIRS@")

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

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.

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

No branches or pull requests

2 participants