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

CMake improvements for packaging #197

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

valgur
Copy link

@valgur valgur commented Feb 28, 2024

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.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! These look like good improvements to me.

I have just a couple requested changes.

cmake/urdfdom-config.cmake.in Outdated Show resolved Hide resolved
cmake/urdfdom-config.cmake.in Outdated Show resolved Hide resolved
@valgur
Copy link
Author

valgur commented Feb 29, 2024

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})

@traversaro
Copy link
Contributor

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})?

@valgur
Copy link
Author

valgur commented Feb 29, 2024

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

@traversaro
Copy link
Contributor

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

There are no .cmake modules installed, so it has no effect.

Signed-off-by: Martin Valgur <[email protected]>
@valgur
Copy link
Author

valgur commented May 27, 2024

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

CMakeLists.txt Outdated
@@ -12,6 +12,9 @@ message (STATUS "${PROJECT_NAME} version ${URDF_VERSION}")

include(GNUInstallDirs)

option(BUILD_APPS "Build applications" ON)
option(BUILD_TESTING "Build tests" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the motivation for making BUILD_TESTING an explicit option? Including CTest will automatically add a BUILD_TESTING option that is ON by default.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just because of plain ignorance on my part. Thanks! I'll drop it.

Not needed as it's added automatically by CTest.

Signed-off-by: Martin Valgur <[email protected]>
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

Successfully merging this pull request may close these issues.

4 participants