-
Notifications
You must be signed in to change notification settings - Fork 127
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
Use latest stable Rust CI + Fix Test Errors #420
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
…guage features Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Even after #419 was merged, the CI for this PR still showed some clippy errors. I've updated and repurposed this PR to get the CI green for the latest stable version of Rust. @esteve I just need to check on how you'd like to resolve this comment
versus this comment
I've noticed that this line already has the CI run every day, which is probably more frequently than what's really needed, but the daily runs don't cost us anything so 🤷 . Are you okay with using For what its worth we do express a minimum supported Rust version in the Cargo.toml of rclrs (which needed to be updated for this PR to pass since we've been using a language feature that stabilized in 1.70). Maybe that's sufficient to express what version we're supporting? |
@@ -6,7 +6,7 @@ authors = ["Esteve Fernandez <esteve@apache.org>", "Nikolai Morin <nnmmgit@gmail | |||
edition = "2021" | |||
license = "Apache-2.0" | |||
description = "A ROS 2 client library for developing robotics applications in Rust" | |||
rust-version = "1.63" | |||
rust-version = "1.70" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually depend on u128 FFI functions (and suppress warnings currently), so it'd be nice if our minimum was actually 1.78 so we could remove that.
@mxgrey thanks for the changes.
I had forgotten that we already run CI periodically and not only per PR. I meant running the periodic workflow with |
It might be confusing if the CI is inconsistent between the PRs and the daily runs. If someone submits a PR to fix the stable CI, we won't get a positive indication that it was successful until the day after the PR is merged. When I have a ROS project where I want to support multiple ROS distros on one branch, I make a CI matrix that runs all the tests across multiple distros. What if we do that here: Have a CI matrix (the same one for both PRs and daily CI) that runs the tests against the oldest supported Rust version as well as stable? |
Yeah, you're right, it can be confusing. In that case, what you propose sounds more like what I had in mind, basically, I want us to be able to detect when to update the version in |
This PR will test whether using the latest stable version of Rust will result in cause CI to fail as suspected in #419 (comment)