Skip to content
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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mxgrey
Copy link
Collaborator

@mxgrey mxgrey commented Oct 3, 2024

This PR will test whether using the latest stable version of Rust will result in cause CI to fail as suspected in #419 (comment)

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>
@mxgrey mxgrey changed the title Use latest stable Rust CI Use latest stable Rust CI + Fix Test Errors Oct 7, 2024
@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 7, 2024

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

I'd rather have an explicit version of the Rust toolchain there (instead of stable)

versus this comment

we should do that here as well, with the latest stable Rust toolchain to catch any issues with the compiler

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 @stable as the PR already does, or do you want me to put in a fixed toolchain version?

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?

rclrs/Cargo.toml Outdated
@@ -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"
Copy link
Collaborator

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.

#398

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've bumped the minimum version to 1.78 because I agree that it's good to declare that the minimum version we support is the one where u128 is ABI safe.

But sadly that's not enough to let us remove the lint because the lint is still being applied for the foreseeable future, I guess because it's still not guaranteed to be ABI safe across all build targets.

@esteve
Copy link
Collaborator

esteve commented Oct 14, 2024

@mxgrey thanks for the changes.

@esteve I just need to check on how you'd like to resolve #419 (comment)

I had forgotten that we already run CI periodically and not only per PR. I meant running the periodic workflow with @stable, but the per-PR CI with a fixed version. Mainly so that we can update the minimum required version in rclrs/Cargo.toml whenever a PR fails.

@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 14, 2024

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?

@esteve
Copy link
Collaborator

esteve commented Oct 14, 2024

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 rclrs/Cargo.toml

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
…work with reusable actions

Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai>
@mxgrey
Copy link
Collaborator Author

mxgrey commented Oct 22, 2024

@esteve Sadly I tried out the matrix idea and discovered that variables cannot be used in the uses: input for GitHub actions [docs here]. This seems to be a fundamental limitation in how GitHub actions are implemented: They want to be able to resolve all reusable actions before forming any of the context data.

I've reluctantly resorted to just copy/pasting the workflow file with the only difference between the two being @stable vs @1.78.

It may be possible to improve this situation by creating a workflow template and reusing it between the two files, but based on the documentation the template needs to be written at the organization level in the .github repo, so someone will need to create a .github repo for the ros2-rust organization before we can take that approach.

Signed-off-by: Michael X. Grey <grey@openrobotics.org>
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.

3 participants