Skip to content

Add missing dependency on urdf#234

Merged
wjwwood merged 1 commit intoros2from
missing_urdf_dep
Mar 21, 2018
Merged

Add missing dependency on urdf#234
wjwwood merged 1 commit intoros2from
missing_urdf_dep

Conversation

@mikaelarguedas
Copy link
Copy Markdown
Member

@mikaelarguedas mikaelarguedas commented Mar 21, 2018

Follow-up of #210.

RViz doesn't build on my machine as it finds the system urdfdom headers (that uses boost::shared_ptr, located in /usr/include/) instead of the one built in ROS 2.

In file included from /home/mikael/work/ros2/current_ws/src/ros2/urdf/urdf/include/urdf/model.h:45:0,
                 from /home/mikael/work/ros2/current_ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/robot/robot.hpp:41,
                 from /home/mikael/work/ros2/current_ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/robot/robot.cpp:31:
/home/mikael/work/ros2/current_ws/build_debug_isolated/urdf/include/urdf/urdfdom_compatibility.h:97:41: error: conflicting declaration ‘typedef class std::shared_ptr<urdf::ModelInterface> urdf::ModelInterfaceSharedPtr’
 typedef std::shared_ptr<ModelInterface> ModelInterfaceSharedPtr;
                                         ^
In file included from /home/mikael/work/ros2/current_ws/build_debug_isolated/urdf/include/urdf/urdfdom_compatibility.h:93:0,
                 from /home/mikael/work/ros2/current_ws/src/ros2/urdf/urdf/include/urdf/model.h:45,
                 from /home/mikael/work/ros2/current_ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/robot/robot.hpp:41,
                 from /home/mikael/work/ros2/current_ws/src/ros2/rviz/rviz_default_plugins/src/rviz_default_plugins/robot/robot.cpp:31:
/usr/include/urdf_world/types.h:48:43: note: previous declaration as ‘typedef class boost::shared_ptr<urdf::ModelInterface> urdf::ModelInterfaceSharedPtr’
 typedef boost::shared_ptr<ModelInterface> ModelInterfaceSharedPtr;

Adding the dependency on urdf in the package including urdf headers fixed the issue for me. Though I didnt look into it in details and maybe more is needed (the urdf dependency should be exported? the dependency should come from another package ? ...)

@mikaelarguedas mikaelarguedas added the in progress Actively being worked on (Kanban column) label Mar 21, 2018
@mikaelarguedas mikaelarguedas changed the title robot_model display uses urdf Add missing dependency on urdf Mar 21, 2018
@mikaelarguedas mikaelarguedas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 21, 2018
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

That's interesting, I would have expected this to fail on our CI job since it uses isolated.

This is definitely a bug though.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@anhosi
Copy link
Copy Markdown
Contributor

anhosi commented Mar 21, 2018

I have seen similar cases where missing dependencies did not make an isolated build fail.

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Mar 21, 2018

When debugging I saw a was other surprises like tf2_ros headers being included in code but not declared as a dependency, but I decided to keep this PR to the minimal set of changes needed to get it to compile on my system.
Not sure why the builds didn't fail

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

Well, tf2_ros is a transitive dependency of rviz_common, which might be the case for urdf too.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

It is, so maybe tf2_ros should be added here following the "depend on what you use" principle, and urdf should be removed from rviz_common if it no longer uses it (not 100% certain of this).

@mikaelarguedas
Copy link
Copy Markdown
Member Author

mikaelarguedas commented Mar 21, 2018

should be added here following the "depend on what you use" principle

Yeah I'm in favor of declaring what a package directly uses rather than assuming what the dependencies provide.
Then if one of it's dependencies has symbols from another package in its' public API it should export that dependency but that's somehow orthogonal.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

Well this is an incremental improvement at least and CI was ok, so I'll merge this and make an issue to audit the dependencies and make sure they're up-to-date.

@wjwwood wjwwood merged commit b19aceb into ros2 Mar 21, 2018
@wjwwood wjwwood deleted the missing_urdf_dep branch March 21, 2018 22:03
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Mar 21, 2018
@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Mar 21, 2018

Thanks @mikaelarguedas for the patch.

@dirk-thomas dirk-thomas mentioned this pull request Apr 11, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mikaelarguedas @wjwwood @anhosi