Unify master and ros2 branches (merge ros2 into master) by j-rivero · Pull Request #158 · ros/urdfdom

j-rivero · 2021-09-15T18:48:01Z

This PR is a continuation of the work done by @clalancette in #157 (please read all details in the description), after the proposal of modifying the way of merging (merge and not rebase) both branches #157 (comment).

I've made a merge in the master branch of ros2 branch in ae6b61d and solve/import some missing pieces in 5a729c1.

Some details I changed on top of Chris work:

A bit of Cmake trickery to keep old platforms (mainly Bionic) working using old CMake style. See 203a1b2 for TinyXML and
e577e81 for the lack of components in CMake.
Add the CI in github actions for the different options of building mentioned by Chris:
- System (mainly cmake, make) on Bionic and Focal: 4213a41
- ROS1 using catkin_make for Melodic 682ddd7
- ROS2 using colcon for Galactic 557b007

Testing is not enabled yet in this repo, can be checked out in my fork https://github.com/j-rivero/urdfdom/actions

CI can be easily extended to all the existing platforms and with a bit more of effort to Mac and Windows. The option of including more or change ROS1/ROS2 distributions could go into this PR, for new operative system I would prefer to do it in different PRs.

This reverts commit 2884900.

fix install routine

* 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. (ros#6)

fix CMake policy 42 warning

* skip test directory when building tests is not requested * include CTest

Port upstream PRs 111 and 112 addressing linker warning

* 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>

j-rivero · 2021-09-17T16:22:54Z

Thanks for taking this over Jose. Really good work so far on this. Here's what I think needs to happen for us to complete this:
1. Finish updating the CMakeLists.txt in response to the TODO comments that are in there.

Before doing that, I did a refactor since all libraries were repeating a good bunch of code. bab00d9

On top of that:

@traversaro suggestion for DEFINE_SYMBOL f070fba
@sloretz suggestion for target_compile_features 7fa83d8

2. The removal of the tests in urdf_unit_test.cpp is unfortunate.  What I really think we should do there is to change `urdf_unit_test.cpp` back to using gtest, and reinstate the tests that were there.

Ouch, my bad. Reverted d6d0303

3. Fix the issues I have inline.

I think all them are fixed. CI seems happy https://github.com/j-rivero/urdfdom/actions

Thanks Chris for the review.

clalancette

I've left a few more comments. This is getting pretty close now.

urdf_parser/CMakeLists.txt

CMakeLists.txt

urdf_parser/CMakeLists.txt

clalancette · 2021-09-17T17:38:57Z

urdf_parser/CMakeLists.txt

+    target_compile_features(${add_urdfdom_library_LIBNAME} PUBLIC cxx_std_14)
+  endif()
+  set_target_properties(${add_urdfdom_library_LIBNAME} PROPERTIES
+    DEFINE_SYMBOL URDFDOM_EXPORTS


I'm OK with making this change, but we need to make sure that this still compiles on Windows in both Release and Debug mode, and that downstream packages can still call it.

Umm. I was playing with Windows and indeed found some problems. Lack of M_PI in e90a6e3, missing API functions f556790 and missing a link c81dd7d. Long CI test run is ongoing in ROS2

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

.github/workflows/ros2.yaml

… jrivero/ros2-merge

clalancette

I left one nit inline, and it looks like @traversaro did as well. Other than those two things, this looks good to me, so I'm going to approve. Once the nits are fixed, we should definitely run CI on this full change in the context of ROS 2.

urdf_parser/include/urdf_parser/urdf_parser.h

Co-authored-by: Silvio Traversaro <silvio.traversaro@iit.it>

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

j-rivero · 2021-09-21T13:11:26Z

A full CI test on ROS2 (only running test on urdfdom):

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-09-21T14:06:50Z

A full CI test on ROS2 (only running test on urdfdom):

This is fine for a first pass, but before we merge I'll suggest we run another CI run with testing --packages-above urdfdom, just to make sure everything downstream is happy. Also we should run Windows Debug as well, as it can be more picky.

j-rivero · 2021-09-21T18:54:14Z

This is fine for a first pass, but before we merge I'll suggest we run another CI run with testing --packages-above urdfdom, just to make sure everything downstream is happy. Also we should run Windows Debug as well, as it can be more picky.

We have green light on Windows/Debug:

CI for --packages-above urdfdom

Linux
Linux-aarch64
macOS
Windows

clalancette

CI looks good here, and Windows Debug was happy as well, so this looks good to me.

The failures on macOS are because the vendored version of gtest in urdfdom is way out of date (1.7.0 vs 1.10.0 that we use in ament). In order to keep the size of this PR down, I discussed with @j-rivero offline about opening a separate PR that just updates gtest to 1.10.0. I'll suggest we hold off on merging this PR until that one is ready to go; once that one is ready, we can merge this, then quickly merge that one, and keep CI green.

clalancette · 2021-09-24T19:12:26Z

OK, I'm going to go ahead and merge this one, then rebase #161 on top to get CI going green. Thanks @j-rivero for all of the work here!

In ros/urdfdom#158 and ros/urdfdom#161, we merged the ros2 branch onto the master branch so there would be single, unified release branch for all of urdfdom. Now that that is done, we should switch to using the master branch of urdfdom.

Karsten1987 and others added 30 commits March 7, 2017 08:52

use tinyxml_vendor

2884900

disable tinyxml_vendor

a1c3180

Revert "use tinyxml_vendor"

a4eeec5

This reverts commit 2884900.

export symbols on windows by default

4ea5a70

export more symbols

429f59d

correctly export more symbols

a35e8ef

add package.xml

639ef31

fix install routine

4622f4f

Merge pull request ros#1 from ros2/fix_install_dir

3f558e2

fix install routine

find debug libraries correctly (ros#2)

32eb4d8

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

use CMAKE_X_STANDARD

cbd8781

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

17f886f

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. (ros#3)" (ros#4)

d2e5e37

This reverts commit 17f886f.

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

d09e117

…ol. (ros#6)

fix CMake policy 42 warning

5b98ef3

Merge pull request ros#7 from ros2/fix_cmake_policy_42_warning

0282a87

fix CMake policy 42 warning

define M_PI if not defined (ros#8)

7763c64

gtest: new version to fix warnings (ros#9)

70a48ec

skip test directory when building tests is not requested (ros#111)

e4fb481

* skip test directory when building tests is not requested * include CTest

remove invalid link dir (ros#112)

106f7fb

Merge pull request ros#11 from ros2/port_111_112

1ac9141

Port upstream PRs 111 and 112 addressing linker warning

Update maintainer (ros#12)

67bbbf2

2.0.0

15ff0c4

use console_bridge_vendor (ros#14)

e42b87d

2.1.0

59eebca

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

ecd9eca

2.2.0

c756477

Change debug / optimized for generator expressions (ros#16)

776eb65

* 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

b77e51c

avoid exporting absolute paths (ros#18)

6ed0517

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

j-rivero requested a review from clalancette September 17, 2021 16:23

clalancette requested changes Sep 17, 2021

View reviewed changes

Jose Luis Rivero and others added 6 commits September 17, 2021 20:29

Install graphiz executable

0386d49

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Remove include_directories for GTEST_INCLUDE_DIR

7b5fe9a

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Missing linking for sensor library

c81dd7d

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Missing M_PI definition

e90a6e3

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Missing API functions

f556790

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

Fix compiler warnings.

a714bcf

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

traversaro reviewed Sep 20, 2021

View reviewed changes

.github/workflows/ros2.yaml Outdated Show resolved Hide resolved

Merge branch 'jrivero/ros2-merge' of github.com:j-rivero/urdfdom into…

d7b09ee

… jrivero/ros2-merge

clalancette approved these changes Sep 20, 2021

View reviewed changes

urdf_parser/include/urdf_parser/urdf_parser.h Outdated Show resolved Hide resolved

Jose Luis Rivero and others added 3 commits September 20, 2021 20:01

Update .github/workflows/ros2.yaml

4827d07

Co-authored-by: Silvio Traversaro <silvio.traversaro@iit.it>

remove duplicate header

4dc8cf6

Fix cast warnings for MSVC in test

85fc714

Signed-off-by: Jose Luis Rivero <jrivero@osrfoundation.org>

This was referenced Sep 21, 2021

urdfdom version 2 updates #157

Closed

Merge the master branch onto the ros2 branch #146

Closed

clalancette approved these changes Sep 21, 2021

View reviewed changes

This was referenced Sep 21, 2021

Update gtest to 1.11.0 version #160

Closed

Update gtest to 1.10.0 version #161

Merged

scpeters approved these changes Sep 24, 2021

View reviewed changes

clalancette merged commit 36be7f8 into ros:master Sep 24, 2021

clalancette mentioned this pull request Sep 24, 2021

Switch to the master branch for urdfdom ros2/ros2#1190

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Unify master and ros2 branches (merge ros2 into master)#158
clalancette merged 64 commits intoros:masterfrom
j-rivero:jrivero/ros2-merge

j-rivero commented Sep 15, 2021 •

edited

Loading

Uh oh!

j-rivero commented Sep 17, 2021

Uh oh!

clalancette left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clalancette Sep 17, 2021

Uh oh!

j-rivero Sep 20, 2021

Uh oh!

Uh oh!

clalancette left a comment

Uh oh!

Uh oh!

j-rivero commented Sep 21, 2021

Uh oh!

clalancette commented Sep 21, 2021 •

edited

Loading

Uh oh!

j-rivero commented Sep 21, 2021

Uh oh!

clalancette left a comment

Uh oh!

clalancette commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Conversation

j-rivero commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j-rivero commented Sep 17, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clalancette Sep 17, 2021

Choose a reason for hiding this comment

Uh oh!

j-rivero Sep 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j-rivero commented Sep 21, 2021

Uh oh!

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

Uh oh!

Uh oh!

j-rivero commented Sep 21, 2021

Uh oh!

clalancette left a comment

Choose a reason for hiding this comment

Uh oh!

clalancette commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

j-rivero commented Sep 15, 2021 •

edited

Loading

clalancette commented Sep 21, 2021 •

edited

Loading