Normalize dependency key names. by nuclearsandwich · Pull Request #1 · ros2/robot_model

nuclearsandwich · 2017-06-17T21:23:41Z

TinyXML is provided by the rosdep key tinyxml and urdfdom-headers should
just be urdfdom_headers.

TinyXML is provided by the rosdep key tinyxml and urdfdom-headers should just be urdfdom_headers.

mikaelarguedas · 2017-06-19T16:47:05Z

urdf/package.xml

  <exec_depend>urdfdom</exec_depend>
-  <exec_depend>urdfdom-headers</exec_depend>
-  <exec_depend>TinyXML</exec_depend>
+  <exec_depend>urdfdom_headers</exec_depend>


not sure what we want to do here, usually we use upstream debian package, the rosdep key for it is liburdfdom-headers-dev. I think we can use urdfdom_headers for now but keep in mind that this should be packaged and be a thirdparty dependency that we install rather than a ros2 package by itself.

Also I'm not sure if we need to depend on both tinyxml and tinyxml_vendor now that we have a chocolatey package for it. But @nuclearsandwich knows much more about this so I'll leave it to you

wjwwood

lgtm other than the urdfdom_header comment (for which perhaps nothing should be done)

wjwwood · 2017-06-19T16:53:51Z

urdf/package.xml

  <exec_depend>urdfdom</exec_depend>
-  <exec_depend>urdfdom-headers</exec_depend>
-  <exec_depend>TinyXML</exec_depend>
+  <exec_depend>urdfdom_headers</exec_depend>


I'm not sure about this one. What's the situation, urdfdom-header points to an upstream deb via rosdep which we cannot use in ROS 2? So this change allows us to use a fork of urdfdom_headers?

If that is the case. I guess that's the right thing to do, but it's an unfortunately subtle change, which I hope we remember to change back at some point.

I guess my unease could be put to rest if we have a comment on it, saying it needs to be resolved at some point.

Here's some related stuff:

Math h revisited urdfdom_headers#1 (review)

always define M_PI ros/urdfdom_headers#34

We recently updated the REP to allow ROS packages to have names like urdfdom-headers. That makes it unnecessary to change the rosdep keys in downstream packages to e.g. urdfdom_headers. We should make use of that and not require these changes to change.

The same rational should also apply to TinyXML.

The same rationale should also apply to TinyXML

Can you elaborate on this. I see no reason to create a rosdistro specific "TinyXML" package when the rosdep key named tinyxml exists and is suitable.

Looks like the tinyxml dependency has been introduced when porting because it was before provided by another package that has been striped. I think that tinyxml is fine as it and that using TinyXML was an oversight by using the "cmake package name" rather than the rosdep key

nuclearsandwich · 2017-06-19T17:25:45Z

Also I'm not sure if we need to depend on both tinyxml and tinyxml_vendor now that we have a chocolatey package for it. But @nuclearsandwich knows much more about this so I'll leave it to you

When I last asked about removing the vendor packages the response from @wjwwood was

We can, but on the other hand they don't cost us much if they don't actually download and build anything because they find the system dependency already installed.

The vendor packages were low effort to produce and are in now, so I'm inclined to just leave them be for beta2. Handling the use of either system or vendor packages is a case we should examine when we discuss build/dependency groups after beta2.

nuclearsandwich · 2017-06-19T17:33:11Z

I'm not sure about this one. What's the situation, urdfdom-header points to an upstream deb via rosdep which we cannot use in ROS 2? So this change allows us to use a fork of urdfdom_headers?

Upstream urdfdom-headers on Xenial is version 0.4.1 whereas the ros2 fork is based on 1.0.0 with one header change. Which means that even if we land ros/urdfdom_headers#34 for this release we still can't use the upstream xenial package.

ros/urdfdom_headers@0.4...ros2:ros2

Because of the restrictions on ament package names. I can't directly shadow urdfdom-headers with a rosdistro package of urdfdom-headers. I'm open to suggestions about how to resolve this. It was not an issue with my vendored console_bridge because the rosdep key is also underscored rather than dashed.

nuclearsandwich · 2017-06-19T17:33:59Z

We recently updated the REP to allow ROS packages to have names like urdfdom-headers.

Oh, this is news to me. I'll just change my urdfdom-headers package name then.

Per the discussion here: ros2/robot_model#1 This allows us to directly shadow the rosdep key for upstream urdfdom-headers.

nuclearsandwich · 2017-06-19T18:23:34Z

A fun added cost of renaming the package.

Other ros2 packages depend on the underscored name. I'll be going through and normalizing those to use the dashed name instead.

steven@steven-desktop:~/o/beta2-release➤ bloomer fpd urdfdom_headers
/home/steven/osrf/ros2_ws/src
ros2/cartographer_ros/cartographer_ros/package.xml
38:  <build_depend>urdfdom_headers</build_depend>

ros2/robot_model/urdf/package.xml
26:  <build_depend>urdfdom_headers</build_depend>

ros2/urdfdom/package.xml
17:  <build_depend version_gte="0.2.3">urdfdom_headers</build_depend>
22:  <exec_depend version_gte="0.2.3">urdfdom_headers</exec_depend>

ros2/robot_model/kdl_parser/package.xml
27:  <build_depend>urdfdom_headers</build_depend>

ros2/robot_state_publisher/package.xml
19:  <build_depend>urdfdom_headers</build_depend>
28:  <exec_depend>urdfdom_headers</exec_depend>

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

nuclearsandwich · 2017-06-19T19:45:45Z

Would love a second round of review on this and the three other PRs settling on the dependency key urdfdom-headers rather than urdfdom_headers so we can shadow upstream for this release (And future releases targeting xenial).

mikaelarguedas · 2017-06-20T15:13:54Z

still looks good to me along with referenced PRs. As long as the decision has been to create a package shadowing the rosdep key (I didnt follow the entire urdfdom discussion so I can't give an explicit greenlight)

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

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 is the second attempt to do so after the first failed due to the urdfdom_headers package not being renamed for source builds. That is being resolved by placing a package.xml with the updated name in the source tree rather than in a release patch.

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. #1 This is the second attempt to do so after the first failed due to the urdfdom_headers package not being renamed for source builds. That is being resolved by placing a package.xml with the updated name in the source tree rather than in a release patch.

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 is the second attempt to do so after the first failed due to the urdfdom_headers package not being renamed for source builds. That is being resolved by placing a package.xml with the updated name in the source tree rather than in a release patch.

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. #1 This is the second attempt to do so after the first failed due to the urdfdom_headers package not being renamed for source builds. That is being resolved by placing a package.xml with the updated name in the source tree rather than in a release patch.

Normalize dependency key names.

b17a403

TinyXML is provided by the rosdep key tinyxml and urdfdom-headers should just be urdfdom_headers.

nuclearsandwich self-assigned this Jun 17, 2017

nuclearsandwich added in progress in review and removed in progress labels Jun 17, 2017

nuclearsandwich mentioned this pull request Jun 17, 2017

ROS 2 Beta 2 package list ros2/rosdistro#2

Closed

mikaelarguedas reviewed Jun 19, 2017

View reviewed changes

wjwwood approved these changes Jun 19, 2017

View reviewed changes

nuclearsandwich added 3 commits June 19, 2017 13:34

urdfdom-headers you're fine just the way you are.

972ba0f

Fix alphabetization

3a89f20

web editors

9fc244b

nuclearsandwich added a commit to ros2-gbp/urdfdom_headers-release that referenced this pull request Jun 19, 2017

Rename package urdfdom-headers.

46ca637

Per the discussion here: ros2/robot_model#1 This allows us to directly shadow the rosdep key for upstream urdfdom-headers.

This was referenced Jun 19, 2017

Use standard urdfdom-headers rosdep key. ros2/urdfdom#3

Merged

Use standard urdfdom-headers rosdep key. ros2/robot_state_publisher#4

Merged

Use standard urdfdom-headers rosdep key. ros2/cartographer_ros#6

Merged

dirk-thomas approved these changes Jun 20, 2017

View reviewed changes

nuclearsandwich merged commit c177e39 into ros2:ros2 Jun 20, 2017

nuclearsandwich deleted the rosdep-keynames branch June 20, 2017 16:03

nuclearsandwich removed the in review label Jun 20, 2017

nuclearsandwich mentioned this pull request Jun 20, 2017

Package naming issues with urdfdom-headers. ros2/rosdistro#6

Closed

This was referenced Jun 22, 2017

Rename dependency key for urdfdom-headers. ros2/cartographer_ros#8

Closed

Rename dependency key for urdfdom-headers. #5

Closed

Rename dependency key for urdfdom-headers. ros2/robot_state_publisher#7

Closed

Conversation

nuclearsandwich commented Jun 17, 2017

Uh oh!

mikaelarguedas Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

wjwwood Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

dirk-thomas Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

dirk-thomas Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaelarguedas Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

nuclearsandwich commented Jun 19, 2017

Uh oh!

nuclearsandwich commented Jun 19, 2017

Uh oh!

nuclearsandwich commented Jun 19, 2017

Uh oh!

nuclearsandwich commented Jun 19, 2017

Uh oh!

nuclearsandwich commented Jun 19, 2017

Uh oh!

mikaelarguedas commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nuclearsandwich Jun 20, 2017 •

edited

Loading