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

Reduce MSRV to 1.63 with minor change in RawFd type #62

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

hoxxep
Copy link
Contributor

@hoxxep hoxxep commented Nov 22, 2023

I noticed that a patch version increment has caused other crates MSRV values to change, such as the zstd crate: gyscos/zstd-rs#253

This MR changes the RawFd type to one present in older versions of rust, so that the MSRV can be reduced to 1.63.

My understanding is that this type should be equivalent because it's gated against #[cfg(unix)], and so the benefits of std::os::fd::RawFd also supporting WASM is negated. rust-lang/rust#98699

In theory the MSRV of the crate library is 1.60, but there are dev-dependencies that require 1.63 which would break the MSRV testing workflow (rustix, required by tempfile).

If we insist on keeping the usage of std::os::fd::RawFd, I can feed back to the zstd crate and ask them to bump their MSRV instead. Thank you!

@hoxxep hoxxep changed the title Reduce MSRV to 1.60 with minor change in RawFd type Reduce MSRV to 1.63 with minor change in RawFd type Nov 22, 2023
@epage
Copy link

epage commented Nov 22, 2023

Not speaking to the value of this change but to the motivation:

I noticed that a patch version increment has caused other crates MSRV values to change, such as the zstd crate: gyscos/zstd-rs#253

A change in MSRV in this crate does not affect the MSRV of any other crate unless they take an action like bump their version requirement. You can maintain a valid dependency tree for your MSRV via Cargo.lock / cargo update. You can even use cargo +nightly generate-lockfile -Zmsrv-policy. We are discussing stabilization on internals.

@hoxxep
Copy link
Contributor Author

hoxxep commented Nov 22, 2023

I agree in principle that the jobserver crate should be allowed to increase its MSRV, but might want to be careful about doing so.

On a rust 1.64 toolchain cargo update is no longer able to resolve a valid dependency tree for the zstd crate. Currently, the only real option for end users on older toolchains is to manually search for and specify an older version of jobserver.

The reasons for submitting this PR are:

  1. This crate is widely used (https://crates.io/crates/cc/reverse_dependencies)
  2. Rust 1.66 is <1y old.
  3. The MSRV bump was introduced by jobserver in a patch version, introducing breaking downstream changes, potentially for enterprise users on older toolchains. Example: for a fresh install of the zstd dependency on rustc 1.64, the zstd crate will no longer compile by default as it resolves jobserver 0.1.27. There are workarounds for end users as you say, but that involves searching though older jobserver versions as cargo update is currently unable to do MSRV-aware dependency resolution.
  4. The change required to enable jobserver to use a lower MSRV is fairly trivial in this PR. If it made the code objectively worse, I would understand that there's a bigger trade-off between compatibility and allowing crates to use newer features.

@epage I see you're contributing to the issue on MSRV-aware dependency resolution at rust-lang/cargo#9930. My two cents would be that MSRV bumps could require at least a minor version bump, so that older toolchains can still benefit from bug and/or security patches in the future.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

I was the person bumping rust-version just because it happened to be 1.66, without thinking of the consequence. This PR looks good me.

@epage
Copy link

epage commented Nov 22, 2023

MSRV bumps are generally recognized as non-breaking.

From a tooling perspective, minor and patch are equivalent.

Also, a 1 year old MSRV covers 99% of requests to crates.io.

@hoxxep
Copy link
Contributor Author

hoxxep commented Nov 22, 2023

@epage I've copied our discussion into your internals thread since it seemed a better place to discuss than this PR 😊 https://internals.rust-lang.org/t/pre-rfc-msrv-aware-resolver/19871/50

Agreed the current interpretation is that MSRV bumps are considered non-breaking. They clearly are breaking for some users though, and so perhaps alongside the MSRV aware RFC we could change the precedent to consider them "semi-breaking" changes. An MSRV bump doesn't require a major version bump, but crates could benefit by leaving room in the versioning scheme to have the option to release patches on the old MSRV for security fixes.

Regarding the crates.io request metrics, I don't doubt the rust community is more up to date than C++ users, but I'd note that legacy/enterprise users will often have their own crates.io mirrors and forked repositories. So crates.io metrics could be skewed in favour of smaller non-legacy projects.

@petrochenkov petrochenkov merged commit d357534 into rust-lang:main Jan 10, 2024
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.

4 participants