icu4x
icu4x copied to clipboard
Amend MSRV policy to address individual crates
In our MSRV policy agreed in #3425, we decided on the policy for when it is permissable to upgrade MSRVs, but we didn't talk about specifically the mechanics of where the upgrades get applied.
In https://github.com/unicode-org/icu4x/issues/3425#issuecomment-1614390024 we had stated at one point:
We guarantee 6 months, attempt 9 months, and don't upgrade unless there's a need.
My interpretation of this statement is that when an individual crate needs the new feature, that crate's MSRV can be upgraded without the need for further justification.
There are some problems with this approach:
- We would need to introduce a mechanism to test MSRV at a per-crate level
- If a core low-level crate like zerovec or yoke is upgraded in MSRV, then it effectively makes all of its dependencies require the elevated MSRV, even if they have a lower one in their Cargo.toml files
I'd like to consider amending the policy as follows:
- The ~rust-toolchain file~ MSRV CI job can be updated at any time when a new MSRV feature is used anywhere in the project.
- The rust-version in the Cargo.toml files is updated only in the crates that use the feature.
- We add a main branch CI job that goes and builds the individual crates with the MSRV stated in their Cargo.toml files, based on the workspace's lockfile.
We could instead adopt the following policy:
- The ~rust-toolchain file~ MSRV CI job and rust-version Cargo.toml files should always be in sync, even if this results in some crates having a higher rust-version than they actually need.
I still hold my position that updating MSRV is hostile to users, and therefore I favor an approach that updates it as little as possible and only when there's a clear need. This is good because we can then directly answer the question, "why did you upgrade your MSRV?" with something better than "because we upgrade MSRV proactively when anything in our project needs it, even if the crate you're using does not need it".
The rust-toolchain file can be updated at any time when a new MSRV feature is used anywhere in the project.
What does the toolchain file have to do with MSRV?
The rust-toolchain file can be updated at any time when a new MSRV feature is used anywhere in the project.
What does the toolchain file have to do with MSRV?
Whoops, I meant to say "MSRV CI job". Fixed.
I am fine with such a policy if CI exists. The design of said CI may be tricky.
I assume this will still be true:
If a core low-level crate like zerovec or yoke is upgraded in MSRV, then it effectively makes all of its dependencies require the elevated MSRV, even if they have a lower one in their Cargo.toml files
I think we should still be recording that in the downstream MSRV for mandatory deps. It's nicer to not force the user to trace things back.
I think there is an interesting wrinkle: what do we do if either:
- One of these crates is an optional dependency (e.g zerovec's yoke dep)
- One of these crates is not an optional dependency, BUT a previous semver-compatible (in the case of
icu_*versions, this is patch-release-only!) version exists with a compatible MSRV.
The former can be handled by special-casing these cases in CI (record somewhere that MSRV CI should cargo build cratename with one version and .. --features foo with another and relying on the fact that the user can always go and
The latter can be handled by running tests on a cargo package blob that has been initiated with the right lockfile but has had certain deps cargo update-downgraded. This would also need some weird CI special casing.
The rust-version for a particular crate can be accessed with the following command:
cargo metadata --format-version 1 | jq '.resolve.root as $root_id | .packages[] | select(.id == $root_id).rust_version'
So a CI could go into all the packages with non-workspace rust versions and run commands like:
$ MSRV=$(cargo metadata --format-version 1 | jq -r '.resolve.root as $root_id | .packages[] | select(.id == $root_id).rust_version')
$ rustup toolchain install $MSRV
$ cargo +$MSRV check --all-features
Concrete proposal:
- ICU component and provider crates (those in the
icu_namespace) continue usingrust-versionfrom the workspace file. We don't do any special MSRV testing beyond what we already do. - Utils crates do not use the workspace MSRV and instead use their own MSRV which can be bumped according to our policy, but only when code in that particular crate needs it to be bumped.
- We add the above mentioned CI job to run on PRs. Hopefully it won't be too slow since these are only utils.
- We initialize all utils crates to the current workspace MSRV at the time this issue is taken.
Needs approval from:
- [x] @Manishearth
- [ ] @robertbastian
- [ ] @hsivonen
Utils crates do not use the workspace MSRV and instead use their own MSRV which can be bumped according to our policy, but only when code in that particular crate needs it to be bumped.
This goes both ways, right? Basically a util will have its msrv bumped when we feel the need to bump it, and similarly it will not need to be bumped if we need to bump msrv for ICU4X.
If so, yeah, this is great!
I think your test is not going to work. You probably cannot build crates with lower MSRV with the repository lock file, because that is generated at a higher MSRV. And cargo is completely unable to take a dependency's MSRV into account when building the lock file, it is not capable of picking the newest version that works at its Rust version. So I think as soon as the first util diverges from our global MSRV, we're gonna be in a world of pain.
We would probably need separate workspaces and lock files for each separate MSRV, and locally always build and update these lock files with older compiler versions.
I just still don't see the value of doing this.
For optional dependencies, I think it's reasonable to let the dependency increase its MSRV before the entirety of ICU4X does. For non-optional util dependencies, I don't see the value of not increasing the MSRV for the whole of ICU4X when the util increases its MSRV.
I defer to @robertbastian on testing, but I, too, am skeptical of a) the CI setup working out and b) a lower MSRV that the CI doesn't test staying accurate.
Our utils don't have too many external non-test dependencies, although they definitely have some. If a lockfile update breaks a utils MSRV test, then it means that dependency can't build with the lower MSRV so that would be motivation to bump the MSRV for that dependency. It is mostly a problem when dependencies bump their own MSRV willy-nilly, which is exactly the motivation for why I don't want us to be the one who bumps MSRV willy-nilly.
Also, based on the comments above, I should clarify that my mental model was that the utils MSRVs would be less than or equal to the components MSRV. I don't see a scenario where a util would want a higher MSRV than the components.
https://internals.rust-lang.org/t/pre-rfc-msrv-aware-resolver/19871
Also, based on the comments above, I should clarify that my mental model was that the utils MSRVs would be less than or equal to the components MSRV. I don't see a scenario where a util would want a higher MSRV than the components.
If someone wants to use a util outside ICU4X below our MSRV, they can fork and make it work with a local patch. Nobody has asked for lower MSRVs, and there's no value here for us.
If a crate gains wider traction in the Rust ecosystem, we move it to its own workspace in its own repo, so that people can work on it outside of Unicode/ICU4X project.
@robertbastian Are you unaligned on the motivation or the solution?
The motivation for the proposed policy:
- Utils crates are designed to be more broadly adopted and therefore more likely to be used in environments with MSRV constraints
- It is our responsibility as the crate authors to make it something easy to adopt. Forcing our users to do workarounds in userland, such as those you describe, is not being good a good steward.
- It is well understood that the utils are separable projects; they should not be made harder to use/adopt just because they live in the ICU4X repository
- Cargo.toml
rust-versionis a big hammer. It causes a build to fail even if it actually works fine at lower MSRV. It is a more correct use ofrust-versionif it actually reflects the MSRV based on the features used in the code, not just some arbitrary number. - We prioritized ICU4X developer velocity in the repository-wide MSRV policy, but for utils, we should focus more on the user within reason. My proposal will result in a small amount of code/configuration for the ICU4X team to maintain, so it is well within reason.
I'm happy removing rust-version from the Cargo.tomls and just documenting MSRV for utils. I'm not happy investing time into setting up CI and managing a separate MSRV for each crate. Using old Rust compilers is just not something that is done in the Rust ecosystem, I really think you're overestimating the user base here.
Using old Rust compilers is just not something that is done in the Rust ecosystem,
I think I'll clarify: this is done: people do have MSRV policies since systems tend to be on older compilers.
However, ICU4X's policy is stranger and stronger than most of what I've seen.
We prioritized ICU4X developer velocity in the repository-wide MSRV policy
This is not my impression. The policy we came up with was pretty strict for the ecosystem.
In general I am in favor of having separate CI for this, I don't think it's a big deal since we probably will have a handful of diverging utils. I do not like that the utils are so closely tied to ICU4X when it comes to license, rust-version, etc. If not for employer constraints I would likely have advocated these be maintained in separate repositories for most of them. It tends to promote better APIs and better decoupling, and makes it easier to get contributions. While that's not a discussion I wish to start, I do think we should try and make them as independent as possible within the same repo.