-
Notifications
You must be signed in to change notification settings - Fork 248
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
Update GenericSubscription's handle_message signature #373
Update GenericSubscription's handle_message signature #373
Conversation
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Unblocks #372 |
Just stumbled upon this myself here: ros2/rmw_iceoryx@52a4bb3 Do you think we can actually shift this generic subscription to |
Maybe generally valuable, we could port it over after the distro release. |
We're still trying to figure out why the test is failing in the Action CI... And that Linux failure on ROS CI is very confusing, some symbol thing in Connext. Bit hard to keep on top of all the changes happening in the core |
do you have an idea why the GitHub Actions fails?
|
What you're pointing at there is actually an error message in shutdown after a successful test. It doesn't mark that test as failed. The failing test is timing out after 60 seconds -
It's passing locally so I'm having a hard time telling what's going on. It will be nice once we switch over to containers, then I could theoretically pull the CI container locally to run the exact environment. |
I'm looking at the test that is timing out and it doesn't look like it's doing what it says it does. I have to dig in further. Probably won't get to it until tomorrow morning, though. |
My guess is that the playback portion of the test is publishing some/all of its messages before our subscription connects to it, then waiting until the timeout for messages which do not arrive. It needs a rework to be timing-insensitive, I can do this first thing tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, sorry folks, I had this change locally, forgot to pr it in the chaos. My fault.
CI lgtm, you can wait on the CI in ros2/rclcpp#1047 (comment) or merge this now, either is fine I believe. |
Fixes the build since ros2/rclcpp#1047
Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com