Migrate to using TinyXML2 (gazebosim#264)

* Remove vendored TinyXML
* Add FindTinyXML2 CMake Logic
* Add XmlUtils with test
* Update CI dependencies
* Allow trim to remove newlines as well
* Remove trailing quote in example sdf
* Make internal URDF parser use tinyxml2

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>

.github/workflows/linux-ubuntu-bionic.yml

@@ -16,7 +16,7 @@ jobs:
         sudo sh -c 'echo "deb http://packages.osrfoundation.org/gazebo/ubuntu-stable $(lsb_release -cs) main" > /etc/apt/sources.list.d/gazebo-stable.list';
         sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys D2486D2DD83DB69272AFE98867170598AF249743;
         sudo apt-get update;
-        sudo apt -y install cmake build-essential curl g++-8 git mercurial libtinyxml-dev libxml2-utils ruby-dev python-psutil cppcheck;
+        sudo apt -y install cmake build-essential curl g++-8 git mercurial libtinyxml2-dev libxml2-utils ruby-dev python-psutil cppcheck;
         sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-8 800 --slave /usr/bin/g++ g++ /usr/bin/g++-8 --slave /usr/bin/gcov gcov /usr/bin/gcov-8;
         # workaround for https://github.com/rubygems/rubygems/issues/3068
         # suggested in https://github.com/rubygems/rubygems/issues/3068#issuecomment-574775885

CMakeLists.txt

@@ -109,24 +109,8 @@ set (build_warnings "" CACHE INTERNAL "build warnings" FORCE)
 set (sdf_cmake_dir ${PROJECT_SOURCE_DIR}/cmake CACHE PATH
      "Location of CMake scripts")
 
-include (${sdf_cmake_dir}/SDFUtils.cmake)
-
-if (UNIX)
-  option (USE_EXTERNAL_TINYXML "Use external TinyXML" ON)
-elseif(WIN32)
-  option (USE_EXTERNAL_TINYXML "Use external TinyXML" OFF)
-else()
-  message (STATUS "Unknown platform")
-  BUILD_ERROR("Unknown platform")
-endif()
-
 message (STATUS "\n\n====== Finding 3rd Party Packages ======")
- # Use of tinyxml. System installation on UNIX. Internal copy on WIN
-if (USE_EXTERNAL_TINYXML)
-  message (STATUS "Using system tinyxml")
-else()
-  message (STATUS "Using internal tinyxml code")
-endif()
+include (${sdf_cmake_dir}/SDFUtils.cmake)
 include (${sdf_cmake_dir}/SearchForStuff.cmake)
 message (STATUS "----------------------------------------\n")
 

Changelog.md

@@ -16,6 +16,9 @@
 
 ### libsdformat 10.0.0 (202X-XX-XX)
 
+1. Migrate to using TinyXML2.
+    * [Pull request 264](https://github.com/osrf/sdformat/pull/264)
+
 1. Enforce minimum/maximum values specified in SDFormat description files.
     * [Pull request 303](https://github.com/osrf/sdformat/pull/303)
 

Migration.md

@@ -24,6 +24,9 @@ but with improved human-readability..
 
 ### Modifications
 
+1. + Depend on tinyxml2 instead of tinyxml for XML parsing.
+   + [Pull request 264](https://github.com/osrf/sdformat/pull/264)
+
 1. + Minimum/maximum values specified in SDFormat description files are now enforced
    + [Pull request 303](https://github.com/osrf/sdformat/pull/303)
 

bitbucket-pipelines.yml

@@ -10,7 +10,7 @@ pipelines:
           - sh -c 'echo "deb http://packages.osrfoundation.org/gazebo/ubuntu-stable `lsb_release -cs` main" > /etc/apt/sources.list.d/gazebo-stable.list'
           - wget http://packages.osrfoundation.org/gazebo.key -O - | apt-key add -
           - apt-get update
-          - apt -y install cmake build-essential curl git libtinyxml-dev libxml2-utils ruby-dev python-psutil cppcheck
+          - apt -y install cmake build-essential curl git libtinyxml2-dev libxml2-utils ruby-dev python-psutil cppcheck
           - gcc -v
           - g++ -v
           - gcov -v

cmake/Modules/FindTinyXML2.cmake

@@ -0,0 +1,42 @@
+# CMake Logic to find system TinyXML2, sourced from:
+# ros2/tinyxml2_vendor
+# https://github.com/ros2/tinyxml2_vendor/commit/fde8000d31d68ff555431d63af3c324afba9f117#diff-120198e331f1dd3e7806c31af0cfb425
+
+# The CMake Logic here is licensed under Apache License 2.0
+# TinyXML2 itself is licensed under the zlib License
+
+# TinyXML2_FOUND
+# TinyXML2_INCLUDE_DIRS
+# TinyXML2_LIBRARIES
+
+# try to find the CMake config file for TinyXML2 first
+find_package(TinyXML2 CONFIG QUIET)
+if(TinyXML2_FOUND)
+  message(STATUS "Found TinyXML2 via Config file: ${TinyXML2_DIR}")
+  if(NOT TINYXML2_LIBRARY)
+    # in this case, we're probably using TinyXML2 version 5.0.0 or greater
+    # in which case tinyxml2 is an exported target and we should use that
+    if(TARGET tinyxml2)
+      set(TINYXML2_LIBRARY tinyxml2)
+    elseif(TARGET tinyxml2::tinyxml2)
+      set(TINYXML2_LIBRARY tinyxml2::tinyxml2)
+    endif()
+  endif()
+else()
+  find_path(TINYXML2_INCLUDE_DIR NAMES tinyxml2.h)
+
+  find_library(TINYXML2_LIBRARY tinyxml2)
+
+  include(FindPackageHandleStandardArgs)
+  find_package_handle_standard_args(TinyXML2 DEFAULT_MSG TINYXML2_LIBRARY TINYXML2_INCLUDE_DIR)
+
+  mark_as_advanced(TINYXML2_INCLUDE_DIR TINYXML2_LIBRARY)
+endif()
+
+# Set mixed case INCLUDE_DIRS and LIBRARY variables from upper case ones.
+if(NOT TinyXML2_INCLUDE_DIRS)
+  set(TinyXML2_INCLUDE_DIRS ${TINYXML2_INCLUDE_DIR})
+endif()
+if(NOT TinyXML2_LIBRARIES)
+  set(TinyXML2_LIBRARIES ${TINYXML2_LIBRARY})
+endif()

cmake/SDFUtils.cmake

@@ -135,30 +135,17 @@ macro (sdf_build_tests)
     string(REGEX REPLACE ".cc" "" BINARY_NAME ${GTEST_SOURCE_file})
     set(BINARY_NAME ${TEST_TYPE}_${BINARY_NAME})
 
-    if (NOT USE_EXTERNAL_TINYXML)
-      set(tinyxml_SRC
-        ${PROJECT_SOURCE_DIR}/src/win/tinyxml/tinystr.cpp
-        ${PROJECT_SOURCE_DIR}/src/win/tinyxml/tinyxmlerror.cpp
-        ${PROJECT_SOURCE_DIR}/src/win/tinyxml/tinyxml.cpp
-        ${PROJECT_SOURCE_DIR}/src/win/tinyxml/tinyxmlparser.cpp)
-    endif()
-
     add_executable(${BINARY_NAME}
       ${GTEST_SOURCE_file}
       ${SDF_BUILD_TESTS_EXTRA_EXE_SRCS}
-      ${tinyxml_SRC}
     )
 
     add_dependencies(${BINARY_NAME}
       gtest gtest_main ${sdf_target}
       )
 
     link_directories(${IGNITION-MATH_LIBRARY_DIRS})
-
-    if (USE_EXTERNAL_TINYXML)
-      target_link_libraries(${BINARY_NAME} PRIVATE
-        ${tinyxml_LIBRARIES})
-    endif()
+    target_link_libraries(${BINARY_NAME} ${tinyxml2_LIBRARIES})
 
     if (UNIX)
       target_link_libraries(${BINARY_NAME} PRIVATE

cmake/SearchForStuff.cmake

@@ -5,36 +5,10 @@ include (${project_cmake_dir}/TargetArch.cmake)
 target_architecture(ARCH)
 message(STATUS "Building for arch: ${ARCH}")
 
-if (USE_EXTERNAL_TINYXML)
-  #################################################
-  # Find tinyxml. Only debian distributions package tinyxml with a pkg-config
-  # Use pkg_check_modules and fallback to manual detection (needed, at least, for MacOS)
-  pkg_check_modules(tinyxml tinyxml)
-  if (NOT tinyxml_FOUND)
-    find_path (tinyxml_INCLUDE_DIRS tinyxml.h ${tinyxml_INCLUDE_DIRS} ENV CPATH)
-    find_library(tinyxml_LIBRARIES NAMES tinyxml)
-    set (tinyxml_FAIL False)
-    if (NOT tinyxml_INCLUDE_DIRS)
-      message (STATUS "Looking for tinyxml headers - not found")
-      set (tinyxml_FAIL True)
-    endif()
-    if (NOT tinyxml_LIBRARIES)
-      message (STATUS "Looking for tinyxml library - not found")
-      set (tinyxml_FAIL True)
-    endif()
-  endif()
-
-  if (tinyxml_FAIL)
-    message (STATUS "Looking for tinyxml.h - not found")
-    BUILD_ERROR("Missing: tinyxml")
-  endif()
-else()
-  # Needed in WIN32 since in UNIX the flag is added in the code installed
-  add_definitions(-DTIXML_USE_STL)
-  include_directories (${PROJECT_SOURCE_DIR}/src/win/tinyxml)
-  set (tinyxml_LIBRARIES "tinyxml")
-  set (tinyxml_LIBRARY_DIRS "")
-endif()
+#################################################
+# Find tinyxml2.
+list(INSERT CMAKE_MODULE_PATH 0 "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules")
+find_package(TinyXML2 REQUIRED)
 
 ################################################
 # Find urdfdom parser. Logic:

examples/simple.sdf

@@ -3,4 +3,4 @@
 <model name='my_model'>
   <link name='link'/>
 </model>
-</sdf>"
+</sdf>

src/CMakeLists.txt

@@ -8,10 +8,6 @@ if (NOT USE_INTERNAL_URDF)
   link_directories(${URDF_LIBRARY_DIRS})
 endif()
 
-if (USE_EXTERNAL_TINYXML)
-  link_directories(${tinyxml_LIBRARY_DIRS})
-endif()
-
 set (sources
   Actor.cc
   AirPressure.cc
@@ -62,22 +58,10 @@ set (sources
   Utils.cc
   Visual.cc
   World.cc
+  XmlUtils.cc
 )
 include_directories(${CMAKE_CURRENT_SOURCE_DIR})
 
-if (USE_EXTERNAL_TINYXML)
-  include_directories(${tinyxml_INCLUDE_DIRS})
-else()
-  set(sources ${sources}
-    win/tinyxml/tinystr.cpp
-    win/tinyxml/tinyxmlerror.cpp
-    win/tinyxml/tinyxml.cpp
-    win/tinyxml/tinyxmlparser.cpp)
-
-    install (FILES win/tinyxml/tinyxml.h
-	    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/sdformat-${SDF_VERSION})
-endif()
-
 if (USE_INTERNAL_URDF)
   include_directories(${CMAKE_CURRENT_SOURCE_DIR}/urdf)
   set(sources ${sources}
@@ -153,18 +137,23 @@ if (BUILD_SDF_TEST)
     sdf_build_tests(Utils_TEST.cc)
   endif()
 
+  if (NOT WIN32)
+    set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS XmlUtils.cc)
+    sdf_build_tests(XmlUtils_TEST.cc)
+  endif()
+
   if (NOT WIN32)
     set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS FrameSemantics.cc)
     sdf_build_tests(FrameSemantics_TEST.cc)
   endif()
 
   if (NOT WIN32)
-    set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS Converter.cc EmbeddedSdf.cc)
+    set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS Converter.cc EmbeddedSdf.cc XmlUtils.cc)
     sdf_build_tests(Converter_TEST.cc)
   endif()
 
   if (NOT WIN32)
-    set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS SDFExtension.cc parser_urdf.cc)
+    set(SDF_BUILD_TESTS_EXTRA_EXE_SRCS SDFExtension.cc parser_urdf.cc XmlUtils.cc)
     sdf_build_tests(parser_urdf_TEST.cc)
     if (NOT USE_INTERNAL_URDF)
       target_compile_options(UNIT_parser_urdf_TEST PRIVATE ${URDF_CFLAGS})
@@ -178,7 +167,13 @@ endif()
 
 sdf_add_library(${sdf_target} ${sources})
 target_compile_features(${sdf_target} PUBLIC cxx_std_17)
-target_link_libraries(${sdf_target} PUBLIC ${IGNITION-MATH_LIBRARIES})
+target_link_libraries(${sdf_target} PUBLIC
+  ${IGNITION-MATH_LIBRARIES}
+  ${TinyXML2_LIBRARIES})
+
+if (WIN32)
+  target_compile_definitions(${sdf_target} PRIVATE URDFDOM_STATIC)
+endif()
 
 target_include_directories(${sdf_target}
   PUBLIC
@@ -187,14 +182,6 @@ target_include_directories(${sdf_target}
     $<INSTALL_INTERFACE:${INCLUDE_INSTALL_DIR}/..>
 )
 
-if (USE_EXTERNAL_TINYXML)
-  target_link_libraries(${sdf_target} PRIVATE ${tinyxml_LIBRARIES})
-else()
-  # Ignore the warnings from the internal library
-  set_target_properties(sdformat${SDF_MAJOR_VERSION} PROPERTIES
-      LINK_FLAGS "/IGNORE:4049 /IGNORE:4217")
-endif()
-
 message (STATUS "URDF_LIBRARY_DIRS=${URDF_LIBRARY_DIRS}")
 message (STATUS "URDF_LIBRARIES=${URDF_LIBRARIES}")