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.

Sign up for GitHub

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

Jump to bottom

Add test dependency on rosidl_typesupport_c_packages #110

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Add test dependency on rosidl_typesupport_c_packages #110

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eba756b
Add test dependency on rosidl_default_generators
jacobperron Apr 30, 2020
7ff5222
Explicitly depend on rosidl_typesupport_c_packages
jacobperron Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions rosidl_generator_py/package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@
<test_depend>python_cmake_module</test_depend>
<test_depend>rmw</test_depend>
<test_depend>rosidl_cmake</test_depend>

<!--
Bloom does not support group_depend so entries below duplicate the group rosidl_typesupport_c_packages.
-->
<test_depend>rosidl_typesupport_connext_c</test_depend>
<test_depend>rosidl_typesupport_introspection_c</test_depend>
<test_depend>rosidl_typesupport_fastrtps_c</test_depend>
<!-- end of group dependencies added for bloom -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Q: should we also add <group_depend>rosidl_typesupport_c_packages</group_depend> ?

Copy link
Member

Choose a reason for hiding this comment

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

What for? Above we already buildtool_export depend on rosidl_typesupport_c which has such a group dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it seems like it is necessary for this package to build (specifically the tests). It seems more correct than relying on it as a transitive dependency.

Copy link
Member

Choose a reason for hiding this comment

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

But a group dependency is not limited to tests but acts as a build as well as run dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, so maybe it's best to leave it at the separate test_depend tags.

Copy link
Member

Choose a reason for hiding this comment

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

Should instead https://github.com/ros2/rosidl_typesupport/blob/3c15da21132af47d73a3fea00f2d99352e647f11/rosidl_typesupport_c/package.xml#L19-L20 be modified to include rosidl_typesupport_fastrtps_c and this package woulld only test_depend on rosidl_typesupport_c (as it also does buildtool_export_depend on)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I'm wrong, but I don't think the issue is that rosidl_typesupport_fastrtps_c is missing. I think the issue is that no C typesupports are depended on here (I presume the build depends of rosidl_typesupport_c don't get installed when building rosidl_generator_py).

I guess adding one or more <build_export_depend> tags for C typesupport packages in rosidl_typesupport_c would fix the build though.

Copy link
Member

Choose a reason for hiding this comment

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

I guess adding one or more <build_export_depend> tags for C typesupport packages in rosidl_typesupport_c would fix the build though.

That doesn't seem right from the perspective of that package.

How about we keep the patch as it is right now and see if it fixes the build as desired?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Considering that the PR job passes (when past PR jobs failed) I'm fairly confident it will work 🤞

<test_depend>rosidl_generator_c</test_depend>
<test_depend>rosidl_generator_cpp</test_depend>
<test_depend>rosidl_parser</test_depend>
Expand Down