ros2 / urdf

Repository for URDF parsing code
BSD 3-Clause "New" or "Revised" License
9 stars 6 forks source link

[urdf] package.xml: add missing exec_depend to urdf_parser_plugin #34

Closed tobiasneumann closed 2 years ago

tobiasneumann commented 2 years ago

Because its been a build dependency that was probably not a visible problem in a normal ROS workspace. However when used in context of openembedded (where the build space and runtime space are separated) this results in urdf_parser_plugin not been part of the image.

fixup for ros2/urdf#13

tobiasneumann commented 2 years ago

Without it the exception package 'urdf_parser_plugin' not found, searching: [/usr] is thrown. I don't know the origin, but i would presume that this occurs at the instantiation of loader_.

clalancette commented 2 years ago

I don't know the origin, but i would presume that this occurs at the instantiation of loader_.

But the thing is that it is a templated class, so that code is all inlined at build time. urdf_parser_plugin shouldn't be needed at runtime at all. That's why I'm confused.

It smells to me like the problem is somewhere else. But I'll defer to @sloretz here, who did the work to add in urdf_parser_plugin.

tobiasneumann commented 2 years ago

I created a little bit unconventional way to reproduces this with a normal ROS workspace.


clalancette commented 2 years ago

I created a little bit unconventional way to reproduces this with a normal ROS workspace.

Ah, thank you! That helps a lot.

And now I see what is going on. During instantiation of the urdf::Model class, it instantiates a urdf::ModelImplementation, which in turn instantiates a pluginlib::ClassLoader here. Note that the package name passed in there is urdf_parser_plugin. Down in the pluginlib implementation, you see that we call ament_index_cpp::get_package_prefix on that package name. So if it doesn't exist, then we get an exception.

So you are totally correct; there is a runtime dependency here, it is just a somewhat unconventional one. In that case, your patch is totally correct, but I'll request that you add a comment above the new exec_depend explaining the situation. Once that is in, I'm happy to approve and run CI on it.

Thanks again.

tobiasneumann commented 2 years ago

I tried to summarize the main reason why its needed from your findings. And hope that this is as you've expected it.

clalancette commented 2 years ago

CI: