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

Replace deprecated spin_until_future_complete with spin_until_complete #350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hliberacki
Copy link

Signed-off-by: Hubert Liberacki [email protected]

@hliberacki
Copy link
Author

Due to change in RCLCPP - ros2/rclcpp#1874 Pull request

@gbiggs
Copy link
Member

gbiggs commented Mar 31, 2022

Running CI: Build Status

@gbiggs
Copy link
Member

gbiggs commented Apr 1, 2022

It looks like you missed one.

@hliberacki
Copy link
Author

It looks like you missed one.

Sorry, but which one do you mean? I've checked (hopefully correctly) and there is only a single call to spin_until_future_complete in this repository ;). Unless you meant something else

@gbiggs
Copy link
Member

gbiggs commented Apr 3, 2022

The offending function is at ros1_bridge/test/test_ros2_client.cpp:36. And I made a mistake, it's not one you missed, it appears to be a missing include. Check the bottom of this output log.

@hliberacki
Copy link
Author

hliberacki commented Apr 4, 2022

The offending function is at ros1_bridge/test/test_ros2_client.cpp:36. And I made a mistake, it's not one you missed, it appears to be a missing include. Check the bottom of this output log.

yes, that was because my changes to rclcpp are still not merged yet. Since the old method needs to be deprecated when new is introduced - merging it first would cause a warning which would break the ci.

the main PR has CI with all of my changes :)

@hliberacki
Copy link
Author

ros2/rclcpp#1874 (comment) Passing CI with all related PRs linked and build together.

@methylDragon
Copy link
Contributor

Just noting here that this is still pending the merge of:

@gbiggs
Copy link
Member

gbiggs commented Jun 29, 2022

Since rclcpp#1874 has been merged, let's try a CI run.

Build Status

@hliberacki
Copy link
Author

Since rclcpp#1874 has been merged, let's try a CI run.

Build Status

Sorry for the confusion, unfortunately there were dependencies which should've been merged together, and due to the fact that there are review findings in rclpy - this PR was reverted until all PRs are in correct shape. rclcpp#1874 - has all desprition about it.

@gbiggs
Copy link
Member

gbiggs commented Jul 4, 2022

That explains the failing CI then. Ping this PR when it's ready to go, then, so I don't forget about it.

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.

4 participants