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

Add test dependency on rosidl_typesupport_c_packages #110

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

jacobperron
Copy link
Member

Since we are generating interfaces for testing.

This is aiming to fix build failures on build.ros2.org, for example: http://build.ros2.org/job/Fbin_uF64__rosidl_generator_py__ubuntu_focal_amd64__binary/7

Since we are generating interfaces for testing.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron changed the title Add test dependency on rosidl_default_generators Add test dependency on rosidl_typesupport_c_packages Apr 30, 2020
<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 🤞

@jacobperron jacobperron merged commit 777f51d into master Apr 30, 2020
@delete-merged-branch delete-merged-branch bot deleted the jacob/default_generators_test_depend branch April 30, 2020 05:52
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