-
Notifications
You must be signed in to change notification settings - Fork 208
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
[eloquent] Use RViz node clock in tools. #520
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@wjwwood any other thing you'd want me to do or add before merging? @mjcarroll since you were (are?) ROS boss for Eloquent, any thoughts? |
LGTM, can you just add a note about it just so that I'll remember to put it in the Patch Release announcement?: ros2/ros2#859 |
You should run Eloquent CI, but other than that no, it's fine to merge. |
@hidmic Based on CI for my other backports (#579 (comment)), I think the cmake/compiler warnings are expected. I don't see the same test failure on Windows though; it seems unrelated to this change, maybe it's a flake and worth re-running 🤷♂️ |
Nope, the test failure is still there. I'll have to dig in. |
Eloquent goes EOL this month. I don't think I'll have time to circle back to this. This is fixed in newer versions, so I'll close this now. |
This pull request is an equivalent of #519, but ABI-compatible so it can be backported.