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

Remove unused IntraProcessMessage #89

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

brawner
Copy link
Contributor

@brawner brawner commented Mar 6, 2020

IntraProcessMessage is no longer used except as a dummy message in rclcpp tests. This removes the message from rcl_interfaces and creates an empty message in test_msgs for those rclcpp tests. rclcpp doesn't depend on common_interfaces, but it already depended on test_msgs so this doesn't change its dependency count.

If reusing common_interfaces/empty.msg is better than creating an empty message I can switch to that instead.

@brawner
Copy link
Contributor Author

brawner commented Mar 6, 2020

See also ros2/rclcpp#1017

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the remove_intraprocessmessage branch from 68fee97 to 4891d55 Compare March 6, 2020 02:13
@brawner
Copy link
Contributor Author

brawner commented Mar 6, 2020

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

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Can you move the Dummy msg here https://github.com/ros2/test_interface_files/tree/master/msg?
Thanks!

@dirk-thomas
Copy link
Member

Creating a Dummy.msg for this case seems overkill. If the tests can use an existing message definition instead that would be preferred. If they can't it would be good to state the rationale why.

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner brawner force-pushed the remove_intraprocessmessage branch from 53da0cb to f9b5432 Compare March 6, 2020 20:02
@brawner
Copy link
Contributor Author

brawner commented Mar 6, 2020

Thanks @ivanpauno for the suggestion. It looks like there is already an Empty message there, so I'm going to use that instead for rclcpp. Latest commit removes Dummy.

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

@brawner
Copy link
Contributor Author

brawner commented Mar 19, 2020

Retesting:

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

@brawner
Copy link
Contributor Author

brawner commented Mar 20, 2020

@ivanpauno @wjwwood The tests now pass. If someone could merge this for me, I would appreciate it.

@ivanpauno
Copy link
Member

@ivanpauno @wjwwood The tests now pass. If someone could merge this for me, I would appreciate it.

Merged both this PR and the one in rclcpp.

@brawner
Copy link
Contributor Author

brawner commented Mar 20, 2020 via email

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.

None yet

4 participants