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.

Sign up for GitHub

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

Switch from TinyXML to TinyXML2 - rebase #178

Closed
wants to merge 8 commits into from
Closed

Switch from TinyXML to TinyXML2 - rebase #178

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c68be5
Switch from TinyXML to TinyXML2
Jan 26, 2017
5ba4be4
chore: rebase
SMillerDev Jan 17, 2023
b9aebf1
convert world.cpp to conditional tinyxml2
SMillerDev Jan 18, 2023
a65ae22
Add DISABLE_TINYXML_SUPPORT cmake option
scpeters Feb 6, 2023
0bca496
urdf_parser: convert remaining files to HAVE_TINYXML
SMillerDev Feb 8, 2023
092b57c
Remove tinyxml2 header from public API
scpeters Mar 20, 2023
3e215fc
Apply suggestions from code review
SMillerDev Apr 4, 2023
aca8250
Fix comments, don't use tinyxml2 namespace
SMillerDev Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/system.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ jobs:
run: |
sudo apt-get -qq update
sudo apt-get -qq upgrade -y
sudo apt-get -qq install -y build-essential cmake libconsole-bridge-dev liburdfdom-headers-dev libtinyxml-dev
sudo apt-get -qq install -y build-essential cmake libconsole-bridge-dev liburdfdom-headers-dev libtinyxml-dev libtinyxml2-dev
cmake --version
gcc --version

Expand Down
29 changes: 20 additions & 9 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ if(APPEND_PROJECT_NAME_TO_INCLUDEDIR)
set(CMAKE_INSTALL_INCLUDEDIR "${CMAKE_INSTALL_INCLUDEDIR}/${PROJECT_NAME}")
endif()

option(DISABLE_TINYXML_SUPPORT
"When ON, only tinyxml2 will be found and linked against. \
When OFF, both tinyxml and tinyxml2 will be found and linked against." OFF)

# set the default build type
if (NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release)
Expand All @@ -45,16 +49,23 @@ endif (MSVC OR MSVC90 OR MSVC10)

set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

find_package(tinyxml_vendor QUIET)
find_package(TinyXML)
# bionic has not cmake module, workaround
if (NOT TinyXML_FOUND AND UNIX)
include(FindPkgConfig)
pkg_check_modules (TinyXML tinyxml)
else()
# Make it fail in platforms without pkgconfig
find_package(TinyXML REQUIRED) # bionic has not cmake module
if(NOT DISABLE_TINYXML_SUPPORT)
find_package(tinyxml_vendor QUIET)
find_package(TinyXML)
# bionic has not cmake module, workaround
SMillerDev marked this conversation as resolved.
Show resolved Hide resolved
if (NOT TinyXML_FOUND AND UNIX)
include(FindPkgConfig)
pkg_check_modules (TinyXML tinyxml)
else()
# Make it fail in platforms without pkgconfig
find_package(TinyXML REQUIRED) # bionic has not cmake module
SMillerDev marked this conversation as resolved.
Show resolved Hide resolved
endif()
endif()

find_package(TinyXML2 REQUIRED)
include_directories(SYSTEM ${TinyXML2_INCLUDE_DIRS})
link_libraries(${TinyXML2_LIBRARIES})
SMillerDev marked this conversation as resolved.
Show resolved Hide resolved

find_package(urdfdom_headers 1.0 REQUIRED)
find_package(console_bridge_vendor QUIET) # Provides console_bridge 0.4.0 on platforms without it.
find_package(console_bridge REQUIRED)
Expand Down
2 changes: 1 addition & 1 deletion cmake/FindTinyXML.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,4 @@ unset(TINYXML_FOUND) # ...and unset uppercase variable generated by find_package
if(TinyXML_FOUND)
set(TinyXML_INCLUDE_DIRS ${TinyXML_INCLUDE_DIR})
set(TinyXML_LIBRARIES ${TinyXML_LIBRARY})
endif()
endif()
SMillerDev marked this conversation as resolved.
Show resolved Hide resolved
74 changes: 74 additions & 0 deletions cmake/FindTinyXML2.cmake
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file at all? I think that TinyXML2 typically ships with a valid, working CMake file, so this seems redundant. But I could be missing something here.

Copy link
Author

Choose a reason for hiding this comment

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

I can see if we can do without it, this was taken from earlier PRs

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing it was copied from the approach used for TinyXML but may not be necessary for TinyXML2

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is related to the distro(s) we are targeting, for example looking at Ubuntu LTS, 18.04, 20.04 and 22.04 do not have the tinyxml2-config.cmake (so find_package(tinyxml2) does not work out of the box):

Copy link

Choose a reason for hiding this comment

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

A problem with FindTinyXML2.cmake as provided is that it does not properly deal with multi-config generators. I want to link urdfdom and dependencies to another project and be able to build that in both Release and Debug (Visual Studio on Windows). I was able to do that only by using tinyxml2-config.cmake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Platforms that need to support multi-config generators I guess that for sure will ship tinyxml2-config.cmake. Probably the best way to support both platforms with tinyxml2-config.cmake and platforms without is to add a find_package(tinyxml2 NO_MODULE) at the top of the FindTinyXML2.cmake , and in case the package is found, just put tinyxml2::tinyxml2 in TinyXML2_LIBRARIES.

Copy link
Author

Choose a reason for hiding this comment

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

Not entirely sure what this entails, could you make a code suggestion?

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
##################################################################################################
#
# CMake script for finding TinyXML2.
#
# Input variables:
#
# - TinyXML2_ROOT_DIR (optional): When specified, header files and libraries will be searched for in
# ${TinyXML2_ROOT_DIR}/include
# ${TinyXML2_ROOT_DIR}/libs
# respectively, and the default CMake search order will be ignored. When unspecified, the default
# CMake search order is used.
# This variable can be specified either as a CMake or environment variable. If both are set,
# preference is given to the CMake variable.
# Use this variable for finding packages installed in a nonstandard location, or for enforcing
# that one of multiple package installations is picked up.
#
#
# Cache variables (not intended to be used in CMakeLists.txt files)
#
# - TinyXML2_INCLUDE_DIR: Absolute path to package headers.
# - TinyXML2_LIBRARY: Absolute path to library.
#
#
# Output variables:
#
# - TinyXML2_FOUND: Boolean that indicates if the package was found
# - TinyXML2_INCLUDE_DIRS: Paths to the necessary header files
# - TinyXML2_LIBRARIES: Package libraries
#
#
# Example usage:
#
# find_package(TinyXML2)
# if(NOT TinyXML2_FOUND)
# # Error handling
# endif()
# ...
# include_directories(${TinyXML2_INCLUDE_DIRS} ...)
# ...
# target_link_libraries(my_target ${TinyXML2_LIBRARIES})
#
##################################################################################################

# Get package location hint from environment variable (if any)
if(NOT TinyXML2_ROOT_DIR AND DEFINED ENV{TinyXML2_ROOT_DIR})
set(TinyXML2_ROOT_DIR "$ENV{TinyXML2_ROOT_DIR}" CACHE PATH
"TinyXML2 base directory location (optional, used for nonstandard installation paths)")
endif()

# Search path for nonstandard package locations
if(TinyXML2_ROOT_DIR)
set(TinyXML2_INCLUDE_PATH PATHS "${TinyXML2_ROOT_DIR}/include" NO_DEFAULT_PATH)
set(TinyXML2_LIBRARY_PATH PATHS "${TinyXML2_ROOT_DIR}/lib" NO_DEFAULT_PATH)
endif()

# Find headers and libraries
find_path(TinyXML2_INCLUDE_DIR NAMES tinyxml2.h PATH_SUFFIXES "tinyxml2" ${TinyXML2_INCLUDE_PATH})
find_library(TinyXML2_LIBRARY NAMES tinyxml2 PATH_SUFFIXES "tinyxml2" ${TinyXML2_LIBRARY_PATH})

mark_as_advanced(TinyXML2_INCLUDE_DIR
TinyXML2_LIBRARY)

# Output variables generation
include(FindPackageHandleStandardArgs)
find_package_handle_standard_args(TinyXML2 DEFAULT_MSG TinyXML2_LIBRARY
TinyXML2_INCLUDE_DIR)

set(TinyXML2_FOUND ${TINYXML2_FOUND}) # Enforce case-correctness: Set appropriately cased variable...
unset(TINYXML2_FOUND) # ...and unset uppercase variable generated by find_package_handle_standard_args

if(TinyXML2_FOUND)
set(TinyXML2_INCLUDE_DIRS ${TinyXML2_INCLUDE_DIR})
set(TinyXML2_LIBRARIES ${TinyXML2_LIBRARY})
endif()
4 changes: 4 additions & 0 deletions package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@
<build_depend>console_bridge_vendor</build_depend>
<build_depend>libconsole-bridge-dev</build_depend>
<build_depend>tinyxml</build_depend>
<build_depend>tinyxml2</build_depend>
<build_depend>tinyxml_vendor</build_depend>
<build_depend>tinyxml2_vendor</build_depend>
<build_depend version_gte="0.2.3">urdfdom_headers</build_depend>

<buildtool_depend>cmake</buildtool_depend>

<exec_depend>console_bridge_vendor</exec_depend>
<exec_depend>libconsole-bridge-dev</exec_depend>
<exec_depend>tinyxml</exec_depend>
<exec_depend>tinyxml2</exec_depend>
<exec_depend>tinyxml_vendor</exec_depend>
<exec_depend>tinyxml2_vendor</exec_depend>
<exec_depend version_gte="0.2.3">urdfdom_headers</exec_depend>

<test_depend>python3</test_depend>
Expand Down
15 changes: 11 additions & 4 deletions urdf_parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,22 @@ macro(add_urdfdom_library)

add_library(${add_urdfdom_library_LIBNAME} SHARED
${add_urdfdom_library_SOURCES})
target_include_directories(${add_urdfdom_library_LIBNAME} SYSTEM PUBLIC
${TinyXML_INCLUDE_DIRS})
target_include_directories(${add_urdfdom_library_LIBNAME} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>")
"$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
${TinyXML2_INCLUDE_DIR})
target_link_libraries(${add_urdfdom_library_LIBNAME} PUBLIC
${add_urdfdom_library_LINK}
${console_bridge_link_libs}
${urdfdom_headers_link_libs}
${TinyXML_LIBRARIES})
${TinyXML2_LIBRARY})
if(NOT DISABLE_TINYXML_SUPPORT)
target_include_directories(${add_urdfdom_library_LIBNAME} SYSTEM PUBLIC
${TinyXML_INCLUDE_DIRS})
target_link_libraries(${add_urdfdom_library_LIBNAME} PUBLIC
${TinyXML_LIBRARIES})
target_compile_definitions(${add_urdfdom_library_LIBNAME} PUBLIC HAVE_TINYXML)
endif()
if(NOT CMAKE_CXX_STANDARD)
target_compile_features(${add_urdfdom_library_LIBNAME} PUBLIC cxx_std_14)
endif()
Expand Down Expand Up @@ -58,6 +64,7 @@ add_urdfdom_library(
LIBNAME
urdfdom_sensor
SOURCES
src/pose.cpp
src/urdf_sensor.cpp
LINK
urdfdom_model)
Expand Down
17 changes: 16 additions & 1 deletion urdf_parser/include/urdf_parser/urdf_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,9 @@
#include <stdexcept>
#include <string>
#include <vector>

#ifdef HAVE_TINYXML
#include <tinyxml.h>
#endif
#include <urdf_model/model.h>
#include <urdf_model/color.h>
#include <urdf_model/utils.h>
Expand Down Expand Up @@ -139,17 +140,31 @@ class URDFVersion final

}

namespace tinyxml2{
// Forward declaration for private APIs.
class XMLElement;
}

namespace urdf{

URDFDOM_DLLAPI ModelInterfaceSharedPtr parseURDF(const std::string &xml_string);
URDFDOM_DLLAPI ModelInterfaceSharedPtr parseURDFFile(const std::string &path);
#ifdef HAVE_TINYXML
URDFDOM_DLLAPI TiXmlDocument* exportURDF(ModelInterfaceSharedPtr &model);
URDFDOM_DLLAPI TiXmlDocument* exportURDF(const ModelInterface &model);
URDFDOM_DLLAPI bool parsePose(Pose&, TiXmlElement*);
URDFDOM_DLLAPI bool parseCamera(Camera&, TiXmlElement*);
URDFDOM_DLLAPI bool parseRay(Ray&, TiXmlElement*);
URDFDOM_DLLAPI bool parseSensor(Sensor&, TiXmlElement*);
URDFDOM_DLLAPI bool parseModelState(ModelState&, TiXmlElement*);
#endif
URDFDOM_DLLAPI void exportURDF(ModelInterfaceSharedPtr &model, std::string &xml_string);
URDFDOM_DLLAPI void exportURDF(const ModelInterface &model, std::string &xml_string);
bool parsePose(Pose&, tinyxml2::XMLElement*);
bool parseCamera(Camera&, tinyxml2::XMLElement*);
bool parseRay(Ray&, tinyxml2::XMLElement*);
bool parseSensor(Sensor&, tinyxml2::XMLElement*);
bool parseModelState(ModelState&, tinyxml2::XMLElement*);
}

#endif
Loading