urdfdom version 2 updates by clalancette · Pull Request #157 · ros/urdfdom

clalancette · 2021-07-06T22:24:01Z

Ever since we merged the ROS 2 version of urdfdom into this repository, there has been something of a split-brain here. This repository is really meant to be ROS-agnostic, but the stuff on the ros2 branch here clearly was not. This is a problem for downstream packagers because the latest tag on the master branch is 1.0.4, but the latest tag overall on this repository is on the ros2 branch (2.3.3). One bug that mentions this is #156 , but there have been others.

This PR is an attempt to solve these problems. What this PR does is to rebase the ros2 branch onto master, making a few changes along the way. By getting the changes from the ros2 branch onto master, it resolves the above ambiguity and also brings some of the latest development onto the master branch. The changes are necessary so that the code from the ros2 branch is not ROS 2 specific.

The changes on this branch can be summarized in the following:

The only change to the API is the addition of the parseCamera, parseRay, parseSensor, and parseModelState to the public API. Since those functions are already implemented internally, here is no ABI change here at all. In some sense this change gets us further away from our goal of removing TinyXML, but I didn't want to do too many things in this PR, so I left it as-is.
Use modern CMake wherever possible. This includes getting rid of global include directives (include_directories), global link directives (link_directories), custom setting of the CXX_STANDARD, etc. It also includes creating modern exported interfaces for the libraries in this package. One place where we could not use modern CMake was finding the include directories for urdfdom_headers. That's because the modern interfaces for urdfdom_headers is not available in the version released in Ubuntu 20.04, so we can't rely on it.
Make finding the "vendor" packages QUIET, which essentially makes them optional. That means that when building standalone (for Debian packaging) or in ROS 1, it will just use the system versions of the packages. On ROS 2 (and more specifically ROS 2 Windows), it will find the vendor packages.
Add a package.xml into the source repository. For ROS 1 we typically relied on overlaying the package.xml at bloom time, but there is no real reason to do that. Systems that do understand package.xml can take easier advantage of it in the source, and systems that don't understand it will just ignore it.

I've tested this PR by building it standalone (mkdir build ; cd build ; cmake .. ; make -j10), building it with catkin_make_isolated in a ROS 1 overlay, and by building it with colcon in a ROS 2 workspace (on Linux and Windows), and it seems to work in all of those circumstances.

If we merge this, this will obsolete #146 .

This reverts commit 2884900.

* find debug libraries correctly * cleanup cmake file. remove messages * set optimized/debug keywords correctly in place * style

We're shadowing this key for r2b2 since upstream is out of date but we should use the rosdep key so we can eventually target upstream. ros2/robot_model#1

This reverts commit 17f886f.

…ol. (#6)

* Change debug / optimized for generator expressions Signed-off-by: Luca Della Vedova <luca@openrobotics.org> * Fixed brackets and variable name Signed-off-by: Luca Della Vedova <luca@openrobotics.org>

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

* Use CMake targets instead of variables Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Use TinyXML_INCLUDE_DIRS :( Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> * Put back urdf_world building everyting for Windows Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

This is so we can use older urdfdom_headers 1.0.4 that doesn't export the modern CMake targets (i.e. on ROS 1). Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

sloretz

Looks pretty reasonable to me. I have nitpicks that probably fit better as follow up PRs since they've been on the ros2 branch for a whlie. What should the next version number for the version released from master be?

sloretz · 2021-07-07T00:33:08Z

cmake/urdfdom-config.cmake.in


 foreach(lib @PKG_LIBRARIES@)
  set(onelib "${lib}-NOTFOUND")
+  set(onelibd "${lib}-NOTFOUND")


nit, I think find_library() will set this to ...-NOTFOUND already, no need to initialize it.

sloretz · 2021-07-07T00:36:29Z

CMakeLists.txt

+
+# Default to C++14
+if(NOT CMAKE_CXX_STANDARD)
+  set(CMAKE_CXX_STANDARD 14)


If the CMake version is bumped a bit (Bionic supports 3.10 which is good enough) then the targets in this package can use

target_compile_features(target_name PUBLIC cxx_std_14)

This has the advantage that if C++ 14 is used in the headers then downstream targets will also get the C++ 14 flags.

sloretz · 2021-07-07T00:39:10Z

urdf_parser/CMakeLists.txt


 include(CTest)
 if(BUILD_TESTING)
  add_subdirectory(test)


nit, there's a use of include_directories() in test/CMakeLists.txt

urdfdom/urdf_parser/test/CMakeLists.txt

Line 14 in e24f47e

include_directories(${GTEST_INCLUDE_DIRS})

sloretz · 2021-07-07T00:43:48Z

CMakeLists.txt

 if(MSVC)
 set(CMAKE_DEBUG_POSTFIX "d")
+# Export symbols on Windows by default
+add_definitions(-DURDFDOM_EXPORTS)


nit

target_compile_definitions(target_name PRIVATE URDFDOM_EXPORTS)

and no need to make it conditional on MSVC, the visibility header looks like defining it would be fine on other platforms too.

Observation that may be ignored for now, if this PR is just to bring ros2 changes on master and not to fix all possible problems in ros2 branch: as URDFDOM_EXPORTS should not be defined if the library is compiled as static, this definition is tipically added automatically by CMake via the DEFINE_SYMBOL property. As in this case the default naming of the DEFINE_SYMBOL is not the correct one, it is possible to simply redefine it as necessary, i.e. add after the add_library calls:

set_target_properties(urdfdom_world PROPERTIES DEFINE_SYMBOL URDFDOM_EXPORTS) set_target_properties(urdfdom_model PROPERTIES DEFINE_SYMBOL URDFDOM_EXPORTS)

Note that the need for setting DEFINE_SYMBOL two times is a consequence of the fact that urdfdom contains two libraries (urdf_model and urdf_world) that contains exactly the same public symbols (as exportWorld and parseWorld are not defined in public headers).

(especially because we have the peculiar case of two libraries,

traversaro · 2021-07-08T07:17:03Z

CMakeLists.txt

+  set(CMAKE_CONFIG_INSTALL_DIR ${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}/cmake)
 endif()
+string(REGEX REPLACE "[^/]+" ".." RELATIVE_PATH_CMAKE_DIR_TO_PREFIX "${CMAKE_CONFIG_INSTALL_DIR}")
+string(REGEX REPLACE "[^/]+" ".." RELATIVE_PATH_LIBDIR_TO_PREFIX "${CMAKE_INSTALL_LIBDIR}")


As for https://github.com/ros/urdfdom/pull/157/files#r665928610, this comment can be safely ignored if the goal of this PR is not to fix all the problems of ros2 branch, but rather to align master and ros2 as fast as possible.

Note that if you need to compute the relative path between two directories, you can use the file(RELATIVE_PATH [..] [..]) CMake command, see for example https://github.com/dartsim/dart/blob/31fb4ea3897cf26788aa16facda4e90f3fbab473/CMakeLists.txt#L409 .

rhaschke · 2021-07-08T10:04:47Z

Thanks @clalancette for this attempt to cleanup history and sync ROS1 and ROS2 branches.
But, I don't understand yet, why you prefer rebasing over merging? Do you also plan to rebase all release tags on the ROS2 branch then? If not, this means that we keep both branches, the old/current ROS2 branch and the rebased one.
I suggest to merge and apply required modifications afterwards. This would cleanly resemble the history of both branches.

What about the release strategy? As far as I remember, ROS1 relies on the system package released via the Debian release process (which was cumbersome). However, ROS2 releases the package via the ROS release process, right?
Do you plan to change the release process for ROS1 as well?

clalancette · 2021-09-21T18:18:23Z

Superseded by #158, where we use the merge strategy as suggested by @rhaschke .

Karsten1987 and others added 30 commits July 6, 2021 15:20

use tinyxml_vendor

28a43be

disable tinyxml_vendor

3f05121

Revert "use tinyxml_vendor"

5fc1902

This reverts commit 2884900.

export symbols on windows by default

a578104

export more symbols

4d4721f

correctly export more symbols

b2f6ead

add package.xml

fc246dd

fix install routine

85e6ead

find debug libraries correctly (#2)

b421719

* find debug libraries correctly * cleanup cmake file. remove messages * set optimized/debug keywords correctly in place * style

use CMAKE_X_STANDARD

25b0e34

Use standard urdfdom-headers rosdep key. (#3)

01bbe2a

We're shadowing this key for r2b2 since upstream is out of date but we should use the rosdep key so we can eventually target upstream. ros2/robot_model#1

Revert "Use standard urdfdom-headers rosdep key. (#3)" (#4)

37ac954

This reverts commit 17f886f.

Packages with the cmake build type should depend on the cmake buildto…

49f7573

…ol. (#6)

fix CMake policy 42 warning

254bb95

define M_PI if not defined (#8)

53529da

Update maintainer (#12)

32f28c5

2.0.0

656f05a

use console_bridge_vendor (#14)

f846ddf

2.1.0

4832f30

Install the package.xml for use by overlay workspaces (#15)

1483bb2

2.2.0

995c4f5

Change debug / optimized for generator expressions (#16)

3bdf0e2

* Change debug / optimized for generator expressions Signed-off-by: Luca Della Vedova <luca@openrobotics.org> * Fixed brackets and variable name Signed-off-by: Luca Della Vedova <luca@openrobotics.org>

2.3.0

d0de0e2

avoid exporting absolute paths (#18)

898ea2f

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

export modern CMake interface target (#19)

8508ff2

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

2.3.1

9123ddc

avoid embedding the absolute install prefix in the artifacts (#20)

27b8fa5

Signed-off-by: Dirk Thomas <dirk-thomas@users.noreply.github.com>

2.3.2

8a84a54

2.3.3

87681bc

Switch back to urdfdom_headers_INCLUDE_DIR.

e24f47e

This is so we can use older urdfdom_headers 1.0.4 that doesn't export the modern CMake targets (i.e. on ROS 1). Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette requested review from scpeters and sloretz July 6, 2021 22:24

sloretz reviewed Jul 7, 2021

View reviewed changes

traversaro reviewed Jul 8, 2021

View reviewed changes

j-rivero mentioned this pull request Sep 15, 2021

Unify master and ros2 branches (merge ros2 into master) #158

Merged

clalancette closed this Sep 21, 2021

clalancette deleted the ros2-rebase branch February 14, 2022 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

urdfdom version 2 updates#157

urdfdom version 2 updates#157
clalancette wants to merge 31 commits intomasterfrom
ros2-rebase

clalancette commented Jul 6, 2021 •

edited

Loading

Uh oh!

sloretz left a comment

Uh oh!

sloretz Jul 7, 2021

Uh oh!

sloretz Jul 7, 2021

Uh oh!

sloretz Jul 7, 2021

Uh oh!

sloretz Jul 7, 2021

Uh oh!

traversaro Jul 8, 2021

Uh oh!

traversaro Jul 8, 2021

Uh oh!

rhaschke commented Jul 8, 2021

Uh oh!

clalancette commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Conversation

clalancette commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sloretz left a comment

Choose a reason for hiding this comment

Uh oh!

sloretz Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

sloretz Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

traversaro Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

traversaro Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

rhaschke commented Jul 8, 2021

Uh oh!

clalancette commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

clalancette commented Jul 6, 2021 •

edited

Loading