urdfdom icon indicating copy to clipboard operation
urdfdom copied to clipboard

CMake improvements for packaging

Open valgur opened this issue 1 year ago • 5 comments

A few minor fixes to CMake. Mostly to aid packaging for the Conan recipe and Vcpkg port.

  • Add explicit BUILD_APPS and BUILD_TESTING options.
  • Add _USE_MATH_DEFINES for MSVC.
  • Replace deprecated ADDITIONAL_MAKE_CLEAN_FILES property with ADDITIONAL_CLEAN_FILES.
  • Prefer find_dependency() in CMake config files.
  • Don't need to enable the C language since the project is pure C++.
  • Add USE_VENDORED_DEPS option and update the config.cmake.in file accordingly. Also modified the exported urdfdom_LIBRARIES to point to the exported CMake targets instead of library files.

valgur avatar Feb 28 '24 09:02 valgur

I applied all the suggestions. Thanks!

I also noticed that only the include/urdfdom path was being exported by urdfdom_INCLUDE_DIRS and added include to it as well.

Here's what the resulting urdfdom-config.cmake looks like after installation:


####### Expanded from @PACKAGE_INIT@ by configure_package_config_file() #######
####### Any changes to this file will be overwritten by the next CMake run ####
####### The input file was urdfdom-config.cmake.in                            ########

get_filename_component(PACKAGE_PREFIX_DIR "${CMAKE_CURRENT_LIST_DIR}/../../" ABSOLUTE)

macro(set_and_check _var _file)
  set(${_var} "${_file}")
  if(NOT EXISTS "${_file}")
    message(FATAL_ERROR "File or directory ${_file} referenced by variable ${_var} does not exist !")
  endif()
endmacro()

macro(check_required_components _NAME)
  foreach(comp ${${_NAME}_FIND_COMPONENTS})
    if(NOT ${_NAME}_${comp}_FOUND)
      if(${_NAME}_FIND_REQUIRED_${comp})
        set(${_NAME}_FOUND FALSE)
      endif()
    endif()
  endforeach()
endmacro()

####################################################################################

if (urdfdom_CONFIG_INCLUDED)
  return()
endif()
set(urdfdom_CONFIG_INCLUDED TRUE)

set(CMAKE_MODULE_PATH_BACKUP_URDFDOM ${CMAKE_MODULE_PATH})
list(APPEND CMAKE_MODULE_PATH "${urdfdom_DIR}")

set(urdfdom_INCLUDE_DIRS "${PACKAGE_PREFIX_DIR}/include/urdfdom")
if(ON)
  list(APPEND urdfdom_INCLUDE_DIRS "${PACKAGE_PREFIX_DIR}/include/urdfdom/..")
endif()

foreach(lib urdfdom_sensor;urdfdom_model_state;urdfdom_model;urdfdom_world)
  set(onelib "${lib}-NOTFOUND")
  set(onelibd "${lib}-NOTFOUND")
  find_library(onelib ${lib}
    PATHS "${PACKAGE_PREFIX_DIR}/lib"
    NO_DEFAULT_PATH)
  find_library(onelibd ${lib}d
    PATHS "${PACKAGE_PREFIX_DIR}/lib"
    NO_DEFAULT_PATH)
  if(onelib-NOTFOUND AND onelibd-NOTFOUND)
    message(FATAL_ERROR "Library '${lib}' in package urdfdom is not installed properly")
  endif()
  if(onelib AND onelibd)
    list(APPEND urdfdom_LIBRARIES $<$<NOT:$<CONFIG:Debug>>:${onelib}>)
    list(APPEND urdfdom_LIBRARIES $<$<CONFIG:Debug>:${onelibd}>)
  else()
    if(onelib)
      list(APPEND urdfdom_LIBRARIES ${onelib})
    else()
      list(APPEND urdfdom_LIBRARIES ${onelibd})
    endif()
  endif()
  list(APPEND urdfdom_TARGETS urdfdom::${lib})
endforeach()

include(CMakeFindDependencyMacro)
if(OFF)
  find_dependency(tinyxml2_vendor QUIET)
  find_dependency(console_bridge_vendor QUIET)
else()
  find_dependency(TinyXML2 REQUIRED)
  find_dependency(console_bridge REQUIRED)
endif()

find_dependency(urdfdom_headers REQUIRED)
list(APPEND urdfdom_INCLUDE_DIRS "${urdfdom_headers_INCLUDE_DIRS}")

foreach(exp urdfdom)
  include(${urdfdom_DIR}/${exp}Export.cmake)
endforeach()

set(urdfdom_LIBRARIES ${urdfdom_TARGETS})

set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH_BACKUP_URDFDOM})

valgur avatar Feb 29 '24 08:02 valgur

If now urdfdom_LIBRARIES does not contain absolute path to libraries anymore but just targets, can we remove all the logic from the for except for list(APPEND urdfdom_TARGETS urdfdom::${lib})?

traversaro avatar Feb 29 '24 08:02 traversaro

@traversaro urdfdom_TARGETS is indeed not needed anymore, but I would keep it for backwards compatibility.

valgur avatar Feb 29 '24 09:02 valgur

@traversaro urdfdom_TARGETS is indeed not needed anymore, but I would keep it for backwards compatibility.

Yes, that is why I suggested to keep just the line related to it in the for, and remove everything else (see https://github.com/ros/urdfdom/pull/197/files#diff-d7dc97268b73a1c3d45d0b2ef6e480309066f17cc38ab85b02121ccc7729ecb3R17-R37).

traversaro avatar Feb 29 '24 09:02 traversaro

@sloretz A friendly ping for a review. The PR should be ready to be merged.

valgur avatar May 27 '24 14:05 valgur