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 2 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 libtinyxml2-dev
cmake --version
gcc --version

Expand Down
17 changes: 8 additions & 9 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,14 @@ 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
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)
include_directories(SYSTEM ${urdfdom_headers_INCLUDE_DIRS})
if (NOT MSVC)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")
endif()
find_package(urdfdom_headers 1.0 REQUIRED)
find_package(console_bridge_vendor QUIET) # Provides console_bridge 0.4.0 on platforms without it.
Expand Down
74 changes: 0 additions & 74 deletions cmake/FindTinyXML.cmake
View file
Open in desktop

This file was deleted.

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()
8 changes: 4 additions & 4 deletions package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@

<build_depend>console_bridge_vendor</build_depend>
<build_depend>libconsole-bridge-dev</build_depend>
<build_depend>tinyxml</build_depend>
<build_depend>tinyxml_vendor</build_depend>
<build_depend>tinyxml2</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>tinyxml_vendor</exec_depend>
<exec_depend>tinyxml2</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
18 changes: 9 additions & 9 deletions 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,8 @@
#include <stdexcept>
#include <string>
#include <vector>

#include <tinyxml.h>
#include <map>
#include <tinyxml2.h>
#include <urdf_model/model.h>
#include <urdf_model/color.h>
#include <urdf_model/utils.h>
Expand Down Expand Up @@ -143,13 +143,13 @@ namespace urdf{

URDFDOM_DLLAPI ModelInterfaceSharedPtr parseURDF(const std::string &xml_string);
URDFDOM_DLLAPI ModelInterfaceSharedPtr parseURDFFile(const std::string &path);
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*);
URDFDOM_DLLAPI tinyxml2::XMLElement* exportURDF(ModelInterfaceSharedPtr &model);
URDFDOM_DLLAPI tinyxml2::XMLElement* exportURDF(const ModelInterface &model);
URDFDOM_DLLAPI bool parsePose(Pose&, tinyxml2::XMLElement*);
URDFDOM_DLLAPI bool parseCamera(Camera&, tinyxml2::XMLElement*);
URDFDOM_DLLAPI bool parseRay(Ray&, tinyxml2::XMLElement*);
URDFDOM_DLLAPI bool parseSensor(Sensor&, tinyxml2::XMLElement*);
URDFDOM_DLLAPI bool parseModelState(ModelState&, tinyxml2::XMLElement*);
Copy link
Contributor

@traversaro traversaro Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could it make sense to take the occasion and remove these functions that refer to tinyxml2 symbols from the public interface, to avoid in the future all the problems that we have now in the tinyxml --> tinyxml2 transition?

We could add bool exportURDFToString(const ModelInterface &model,, std::string xml_string) and bool exportURDFToFile(const ModelInterface &model,const std::string& path) to mirror the existing parseURDF and parseURDFFile . If downstream users need the tinyxml2::XMLDocument, they can always re-parse the string.

Copy link
Author

Choose a reason for hiding this comment

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

I'd like to suggest the community picks this up since that seems like a decision that needs more knowledge about the tool than I as a mere packager of the tool possess.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @SMillerDev . My comment is indeed more directed towards @scpeters and @clalancette .

Copy link
Contributor

Choose a reason for hiding this comment

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

@traversaro yes, I think we could get rid of the APIs that write to a tinyxml2::XMLDocument. We'd still have the parse* methods that read from tinyxml2::Element* pointers. Do you have any suggestion for those? I guess since it's a raw pointer, we could use forward declarations and remove the include of tinyxml2.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either fwd declare, or simply keep them only as private functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely better!

Copy link
Contributor

Choose a reason for hiding this comment

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

@clalancette and @sloretz what do you think of this API approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

@scpeters I'm a little confused; it looks like 092b57c isn't on this branch. Is that meant to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not on the branch at the moment as this PR is from @SMillerDev's branch. I'm just getting some feedback before pushing it

Copy link
Contributor

Choose a reason for hiding this comment

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

@clalancette I just it to this branch

}

#endif
Loading