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

feat: Add sync-toolchains workflow #3

Merged
merged 4 commits into from
Jun 13, 2024

Conversation

fuzzypixelz
Copy link
Member

This pull request adds a workflow that fetches the latest stable Rust release, and updates all eclipse-zenoh Rust toolchains to use it. The workflow is scheduled to run nightly.

@Mallets
Copy link
Member

Mallets commented Feb 21, 2024

I kind of disagree to have a toolchain like that to run nightly and to automatically update the toolchain to the whole Rust project.
We need to have a better control on that and to decide when bumping the toolchain.
OSs and Distributions may package and ship a specific version of the rust toolchain and compiler that people rely on.
Adding such CI action will break those scenarios too easily.

I'd rather prefer to have a CI that can be triggered manually to sync all the repos to a specific rust toolchain version once we decide it's the right time to bump the toolchain.

@fuzzypixelz
Copy link
Member Author

fuzzypixelz commented Feb 21, 2024

I kind of disagree to have a toolchain like that to run nightly and to automatically update the toolchain to the whole Rust project. We need to have a better control on that and to decide when bumping the toolchain. OSs and Distributions may package and ship a specific version of the rust toolchain and compiler that people rely on. Adding such CI action will break those scenarios too easily.

I'd rather prefer to have a CI that can be triggered manually to sync all the repos to a specific rust toolchain version once we decide it's the right time to bump the toolchain.

I can change the workflow to take a Rust version as input, it's a trivial change.

However, this raises questions about how the Rust toolchain should be set.

  1. Arguably, it should be set to the MSRV of zenoh. This is a bit ill-defined as each crate in eclipse-zenoh has a different MSRV. For instance zenoh-collections's MSRV is 1.66.1 while zenoh's MSRV is 1.72.1 currently. And I don't see why out-of-tree plugins and backends will always share the same MSRV as zenoh. So the toolchain would be set to the maximum MSRV of all eclipse-zenoh crates, to ensure that the same toolchain is set everywhere.

  2. Calculating the MSRV depends on the lockfile. For instance, currently running cargo-msrv on the zenoh crate yields an MSRV of 1.72.1. However, if we delete the lockfile, then the MSRV becomes 1.74.1 instead! This is because the lockfile is not updated (if it were, we would be forced to update toolchain to 1.74.1).

This is why I sought to simplify all this by using the latest stable toolchain. Third parties can simply override the toolchain if they like (as long as they respect the MSRV). Moreover, crates.io ignores the toolchain altogether.

Here's an example of what can go wrong:

  • Dependency D updates its MSRV to a version higher than the Rust toolchain in use
  • Per Cargo's SemVer official policy, this will be a minor change
  • The new version of D will be compatible with the current version specification in the manifest (because it's a minor update)
  • Packaging will no longer work as (1) cargo-package ignores the lockfile, (2) the new version of D will be used and (3) Cargo will try to build the crate with the current Rust toolchain (one can override this check using --no-verify)
  • Developers won't notice anything because the lockfile points to old version of D

Such an issue would be avoided by either:

  1. Keeping the Rust toolchain higher than any dependency can set it (stable)
  2. Setting the Rust toolchain to the actual MSRV (i.e. assuming the lockfile is removed)
  3. Continuously updating the lockfile

@fuzzypixelz
Copy link
Member Author

@Mallets I updated the workflow to take the Rust toolchain version as an input.

@gabrik gabrik merged commit 50346f1 into eclipse-zenoh:main Jun 13, 2024
2 checks passed
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