ros2_rust icon indicating copy to clipboard operation
ros2_rust copied to clipboard

Timers

Open mxgrey opened this issue 6 months ago • 4 comments

This PR splits the timer.rs module out of #446 so it can be reviewed separately after #446 is merged.

mxgrey avatar May 14 '25 15:05 mxgrey

@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:

  1. Every run of colcon build seems 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.
  2. 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:

mxgrey avatar Jun 29 '25 15:06 mxgrey

@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.

mxgrey avatar Jun 29 '25 15:06 mxgrey

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.

knmcguire avatar Jun 30 '25 07:06 knmcguire

See this PR: https://github.com/ros2-rust/ros2_rust/pull/495

knmcguire avatar Jun 30 '25 07:06 knmcguire

@jhdcs thanks for noticing the absence of safety comments. I've added them in via a589997

mxgrey avatar Jul 17 '25 12:07 mxgrey

CI is currently failing because of an upstream issue which is fixed by https://github.com/ros2-rust/rosidl_runtime_rs/pull/13

mxgrey avatar Jul 17 '25 15:07 mxgrey

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 avatar Jul 18 '25 08:07 mxgrey

@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 avatar Aug 21 '25 15:08 esteve

@esteve no worries, merge conflicts are fixed!

mxgrey avatar Aug 22 '25 16:08 mxgrey

@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.

esteve avatar Oct 12 '25 09:10 esteve