search

ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
97 stars 131 forks source link

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

Closed j-rivero closed 2 years ago

j-rivero commented 2 years ago

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 https://github.com/ros/urdfdom/pull/157#issuecomment-876308696.

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:

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.

j-rivero commented 2 years ago

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. https://github.com/ros/urdfdom/pull/158/commits/bab00d9081eba321db0399069b9e53b1dfec8e30

On top of that:

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 https://github.com/ros/urdfdom/pull/158/commits/d6d03030d20560477024a454bcdc25152ed5ad92

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.

j-rivero commented 2 years ago

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

clalancette commented 2 years ago

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 commented 2 years ago

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: Build Status

CI for --packages-above urdfdom

clalancette commented 2 years ago

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!