ros2_rust
ros2_rust copied to clipboard
Timers
This PR splits the timer.rs module out of #446 so it can be reviewed separately after #446 is merged.
@esteve This is embarrassing since I was vocally supportive of committing the cargo lockfiles, but now I'm thinking that might have been a mistake. I'm noticing two issues in practice:
- Every run of
colcon buildseems to update the lockfiles. In fact a bunch of different tools that I use on a regular basis are triggering automatic updates of the lockfiles. It's creating a lot of unnecessary churn in the git history. - More crucially, I'm seeing this error happen in the minimum supported Rust compiler version CI. Essentially any new lockfiles that get generated and committed will be incompatible with the "Minimal" CI.
I've had to resort to manually deleting the Cargo.lock for the worker_demo, logging_demo, and now timer_demo examples each time I commit in order to get the Minimal CI to pass.
I'd suggest we remove the lockfiles from the git history or else this is going to cause a great deal of ongoing annoyance where everyone is constantly reverting lockfiles before committing :disappointed:
@knmcguire Sorry to summon you into this random PR but it seems a new CI failure has popped up for Windows.
I have a feeling this is due to this recent PR in colcon-cargo, and we'll see this failure show up for the main branch on Tuesday during its weekly CI run.
Thanks for letting me know ! I don't get messages of the CI failures on windows so I was unaware of this. I'll dig into it and make a PR
The cargo lock did solve some things for the windows CI though so if that is removed we might need to deal with those failures, but let's see when it comes.
See this PR: https://github.com/ros2-rust/ros2_rust/pull/495
@jhdcs thanks for noticing the absence of safety comments. I've added them in via a589997
CI is currently failing because of an upstream issue which is fixed by https://github.com/ros2-rust/rosidl_runtime_rs/pull/13
When I did a squash merge to resolve merge conflicts from https://github.com/ros2-rust/ros2_rust/pull/446, I lost the commit history from https://github.com/ros2-rust/ros2_rust/pull/440 which this PR was based off of.
I've added 775b170 which should prompt GitHub to credit @agalbachicar and @JesusSilvaUtrera when this PR gets squash-merged, but I would appreciate if whichever maintainer merges this PR can double-check that the final commit message includes
Co-authored-by: Agustin Alba Chicar <[email protected]>
Co-authored-by: Jesús Silva <[email protected]>
@mxgrey sorry it took so long, can you fix the conflicts? Now that rclrs 0.5.0 is out, we can merge this soon. Thanks.
@esteve no worries, merge conflicts are fixed!
@jhdcs I hope it's ok with you, I'll merge this PR. Given the US government shutdown, that @jhdcs can't work on ros2-rust during the shutdown, and that @mxgrey has addressed @jhdcs 's feedback, I'll go ahead and merge this PR.
Thanks @mxgrey for the great work, and @jhdcs @knmcguire @luca-della-vedova for their reviews.