Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

tobiasneumann
Copy link

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.

  • FIX: [urdf] package.xml: add missing exec_depend to urdf_parser_plugin

fixup for #13

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I don't totally see how this is possible.

urdf_parser_plugin is a header-only library; it only installs a parser.h file plus some CMake glue.

Further, this package (urdf) only uses urdf_parser_plugin in internal C++ files (in src/model.cpp and src/urdf_plugin.cpp). So I don't see how this can require an exec_depend.

Can you give us some more information about what failed and why you think this is the fix?

@tobiasneumann
Copy link
Author

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
Copy link

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
Copy link
Author

tobiasneumann commented Oct 26, 2022

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

  • Clone and build https://github.com/ros/urdf_tutorial.git (branch ros2)
  • Use the launch file from the tutorial with any urdf ros2 launch urdf_tutorial display.launch.py model:=<any-urdf> to check that this works.

  • move or delete urdf_parser_plugin from wherever you have it installed.
  • try to restart the launch file again, this will result in the exception been thrown.

@clalancette
Copy link

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 tobiasneumann force-pushed the add-missing-exec-dependency-to-urdf branch from 3935cd7 to 53b4080 Compare October 27, 2022 07:27
@tobiasneumann
Copy link
Author

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

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small grammatical fixes. Once those are in, I'm happy to approve and run CI on it. Thanks.

urdf/package.xml Outdated Show resolved Hide resolved
@clalancette
Copy link

CI:

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

@clalancette clalancette merged commit 4f38dd9 into ros2:rolling Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants