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 messages with key to test_communication #542

Open
wants to merge 6 commits into
base: rolling
Choose a base branch
from

Conversation

MiguelCompany
Copy link

@MiguelCompany MiguelCompany commented Apr 3, 2024

This adds support for the messages added by ros2/rcl_interfaces#162 to the test_communication package

Part of ros2/ros2#1538

Depends on ros2/rosidl#796
Depends on ros2/rosidl_typesupport_fastrtps#116
Depends on ros2/rcl_interfaces#162

@MiguelCompany MiguelCompany force-pushed the feature/rolling/keys branch from 2eaefd2 to ee107f2 Compare April 3, 2024 08:50
Comment on lines +44 to +45
string_ends_with("${interface_file}" ".idl" is_idl)
if(is_idl AND interface_ns STREQUAL "msg")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this bug, that is unrelated to this original fix? .idl files are included in messages.

Copy link
Author

Choose a reason for hiding this comment

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

In a way, yes.
ros2/rcl_interfaces#162 uncovered a bug in this CMakeLists.txt file.
The code here is not considering the case where a message is directly defined in a .idl file.

I suppose I could make a separate PR with the changes in 486acce.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can fix this with another PR, so that we can backport this fix to other distros.

Copy link
Author

Choose a reason for hiding this comment

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

See #549 for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

this can be removed from this PR after #549 is merged

@@ -0,0 +1,53 @@
// Copyright 2015 Open Source Robotics Foundation, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

other new files too.

Suggested change
// Copyright 2015 Open Source Robotics Foundation, Inc.
// Copyright 2024 Open Source Robotics Foundation, Inc.

Copy link
Author

Choose a reason for hiding this comment

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

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