Skip to content

Conversation

mxgrey
Copy link
Contributor

@mxgrey mxgrey commented Jul 17, 2025

Some recent updates to this repo have led to CI failing in rclrs for the minimal Rust version 1.75.

This PR aims to fix it via the following:

  • Use a compatible resolver for the workspace Cargo.toml (2 instead of 3)
  • Add a CI workflow that verifies this repo is working for both 1.75 and the latest stable
  • Remove the Cargo.lock files so they don't create confusion for older versions of Rust

@mxgrey
Copy link
Contributor Author

mxgrey commented Jul 17, 2025

Apparently we were never running the tests for this package with the serde feature enabled, because we had some invalid test cases which are now fixed in this PR.

Copy link
Contributor

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, though I'd feel more comfortable if a second maintainer could have a look since I'm not the most experienced with workflows

@@ -1,3 +1,3 @@
[workspace]
members = ["rosidl_runtime_rs"]
resolver = "3"
resolver = "2"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry for the intrusion. I was taking a look at ros2_rust's examples and following the instructions for setting up the workspace led me to this error (resolver 3 not available for 1.75).
Going in with this PR would be great so as to build the workspace from scratch without issues, especially for newcomers.

Copy link
Contributor

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a maintainer but anything that adds CI and proves to build correctly sounds like a good idea and upstream development on rclrs is effectively blocked until this repo works correctly, so even if there are any issues I'd suggest merging this first and fixing them in followups.

The only possible error surface is really the code in string.rs that looks sensible to me.

@maspe36 maspe36 merged commit 7818b4d into ros2-rust:main Aug 3, 2025
8 checks passed
@maspe36
Copy link
Contributor

maspe36 commented Aug 3, 2025

Agreed, sorry for the delay on this PR everybody. I did screw up during the merge though and didn't squash the commits. Apologies (again) all

I updated the repo settings so that won't happen again

@mxgrey mxgrey deleted the compatibility_with_1.75 branch August 6, 2025 04:01
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