Skip to content

Replace deprecated spin_until_future_complete with spin_until_complete#350

Open
hliberacki wants to merge 2 commits intoros2:masterfrom
hliberacki:hliberacki/spin_until
Open

Replace deprecated spin_until_future_complete with spin_until_complete#350
hliberacki wants to merge 2 commits intoros2:masterfrom
hliberacki:hliberacki/spin_until

Conversation

@hliberacki
Copy link
Copy Markdown

Signed-off-by: Hubert Liberacki hliberacki@gmail.com

Signed-off-by: Hubert Liberacki <hliberacki@gmail.com>
@hliberacki
Copy link
Copy Markdown
Author

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

@gbiggs
Copy link
Copy Markdown
Member

gbiggs commented Mar 31, 2022

Running CI: Build Status

@gbiggs
Copy link
Copy Markdown
Member

gbiggs commented Apr 1, 2022

It looks like you missed one.

@hliberacki
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

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

@methylDragon
Copy link
Copy Markdown
Contributor

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

@gbiggs
Copy link
Copy Markdown
Member

gbiggs commented Jun 29, 2022

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

Build Status

@hliberacki
Copy link
Copy Markdown
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
Copy Markdown
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.

5 participants