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

Fix msrv: Run msrv checks with minimal versions #425

Merged
merged 7 commits into from
Aug 24, 2024

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Aug 23, 2024

Since it is possible to for dependencies to bump MSRV in patch release, msrv checking should use the minimal versions supported by flate2.

Users cares deeply about msrv can also pin them to the minimal versions to maintain their current msrv.

This would have also help discover bugs in #424

Since it is possible to for dependencies to bump MSRV in patch release, msrv checking should use the minimal versions supported by flate2.

Users cares deeply about msrv can also pin them to the minimal versions to maintain their current msrv.

This would have also help discover bugs in rust-lang#424
@Byron
Copy link
Member

Byron commented Aug 23, 2024

Oh, right:

Run cargo build --features zlib-rs --no-default-features
 Downloading crates ...
  Downloaded libz-rs-sys v0.2.1
  Downloaded zlib-rs v0.2.1
error: package `zlib-rs v0.2.1` cannot be built because it requires rustc 1.75 or newer, while the currently active rustc version is 1.56.1

The MSRV is only valid for the C bindings, not for zlib-rs. Maybe this could also be clarified. For now, I think it's OK to remove zlib-rs from the set of tested deps, or add something separate with its own MSRV.

@NobodyXu
Copy link
Contributor Author

Thanks removed it

@NobodyXu
Copy link
Contributor Author

It can finally reproduce the error 🎉

@Byron
Copy link
Member

Byron commented Aug 23, 2024

Let me put my commit in and bump the crate version…

@jongiddy
Copy link
Contributor

jongiddy commented Aug 23, 2024

I disagree here. The rust-version causes users with earlier compiler version to see failures to download a recent version of flate2 that works for them. Users with version 1.56.1 can currently download the most recent version of flate2 and use it with the default features.

Increasing the rust-version will cause problems for them. Yes, they will have problems if they enable any of the other features. But they can deal with that by not using those features or upgrading their compiler.

@Byron
Copy link
Member

Byron commented Aug 23, 2024

Thanks for chiming in @jongiddy!

This makes sense and I have undone the change to the rust-version field, and updated the commit message accordingly.

@NobodyXu Could you provide another one of these PRs for the libz-sys crate? It seems it's unknowingly using features of more recent vcpkg versions.

@jongiddy
Copy link
Contributor

To clarify some more, the problem with rust-version is that there are multiple things it could mean:

  1. Any compiler version lower than rust-version will fail to build the flate2 code, probably due to flate2 use of a feature added to the compiler.
  2. Any compiler version lower than rust-version will fail to build flate2 and its dependencies. This may be different depending on features (and their differing dependency sets) as well as over time as dependencies raise their minimal versions with a non-breaking version bump.
  3. The compiler version that the maintainers are happy to support, generally known as the MSRV. This can be unrelated to compilability and also changes over time (e.g. flate2 has an MSRV policy that increases even if we make no changes to the code).

Essentially rust-version is not adequate for real MSRV handling and the only option above that really works is the first. At least with that option, the minimal version only changes when there is a commit to the flate2 repo.

If we raise the rust-version higher we will get complaints that users cannot get the newest version, even though it would work for them. I'd rather get complaints that a feature doesn't work with their old compiler than that they cannot use crates.io but can use GitHub main with their old compiler.

@Byron
Copy link
Member

Byron commented Aug 23, 2024

Thanks for the clarification, very insightful!

@jongiddy
Copy link
Contributor

AIUI, what we are looking for here is to check that all features work when the MSRV (as defined by option 3 above) is used? I think that could exist as a separate task. The problem is that since the MSRV is current release - 1, then every 6 weeks we need to modify the workflow to update the compiler version.

It might work if we say the MSRV is 1.79 and then only update it when we feel like it, or when a dependency breaks CI.

@Byron
Copy link
Member

Byron commented Aug 23, 2024

Actually it's the smallest possible version that is still able to build the core features (i.e. without zlib-rs. In the README, it talks about the MSRV very conservatively, but I wouldn't be surprised if people were very upset if we'd all of the sudden stick to that minimal requirement only. Thus, previously, Rust 1.56 was tested and this PR adds a lot of detail which shows that it was already too low, while also showing that the Cargo.toml versions declared in dependencies aren't working everywhere either.

But you are absolutely right to ask under which circumstances this setup would break - in theory it does so only if this crate changes, as dependencies would be stuck at their minimal versions.

Maybe there should also be a forward-facing version of this task that tests with the latest dependencies - after all, these would be what truly can affect the typically possible MSRV.

I hope that makes some sense, and I am writing this thinking that I was probably missing some or all of your point :/.

@NobodyXu
Copy link
Contributor Author

Could you provide another one of these PRs for the libz-sys crate? It seems it's unknowingly using features of more recent vcpkg versions.

@Byron Opened rust-lang/libz-sys#207

@jongiddy
Copy link
Contributor

My main concern is that the rust-version should not be increased unnecessarily.

A user who happens to be stuck on Rust 1.56.1 can currently add flate2 = "1" to their dependencies and their code will build. Due to the current minimum version test, they will continue to receive working updates until the rust-version increases. When rust-version does eventually increase, Cargo should keep them on the last version that works with 1.56.1.

Now, if this user adds a backend feature, they will need cc and that increases the minimum versions that will work. But flate2 cannot control that. The next version of cc might reduce the supported compiler version! So flate2 should not specify that, but should let Cargo fail and tell the user which crate is the problem.

This change seems to be because users of flate2 see a message like error: package cc v1.0.99 cannot be built because it requires rustc 1.63 or newer when using specific features and you want that to change so that the direct dependency (flate2 or something higher) gets the blame. I don't think anything like that is practical right now. As you see, if you really want that, then the minimum version is 1.75 and can be increased at the whim of any dependency. Users need to learn to use cargo tree --invert to help identify the dependency hierarchy that is causing the problem.

@NobodyXu
Copy link
Contributor Author

The next version of cc might reduce the supported compiler version!

Well that's impossible.

cc has a msrv of 1.63, same as debian.

The reason we keep 1.63 is because of complaints from users, otherwise we would bump to newer rustc already.

Previously we tried bumping it to 1.65/1.66c and gets pushed back, that's probably why you had experience of a reduction in msrv, it's actually a revert of msrv bump we did.

P.S. I'm co-maintainer of cc

@NobodyXu
Copy link
Contributor Author

BTW for cc we really want to bump the msrv to drop some code and use some new features.

@jongiddy
Copy link
Contributor

@NobodyXu Not suggesting that cc will do this. It was just an example that flate2 cannot control the evolution of minimum versions of its dependencies, so should not try to propagate the highest of their minimum versions as its minimum version, but instead let Cargo handle it.

@NobodyXu
Copy link
Contributor Author

For the zlib-ng-cc part, I think cc is the only dependency?

In that case I suppose it's pretty easy to track.

so should not try to propagate the highest of their minimum versions as its minimum version, but instead let Cargo handle it.

I love the idea that cargo automatically selects one that fits in my msrv, but unfortunately they aren't ready yet.

…test releases.

That way, those compiling with `-Zminimal-versions` have higher chances of it to work.

See GitoxideLabs/gitoxide#1541 for reference.
That way it should compile with minimal versions.

Reason for this is the following:

````
error: package `cc v1.0.98` cannot be built because it requires rustc 1.63 or newer, while the currently active rustc version is 1.56.1
````

Note that the expressed MSRV is still a lower one as it may work for
some people that use different settings and/or configuration.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Great, it works now!

@jongiddy Please note that only the tested MSRV was increased to 1.63, the claimed one is unchanged. Thus I would expect that there is no issue downstream.

@Byron Byron merged commit 50852c6 into rust-lang:main Aug 24, 2024
13 checks passed
@NobodyXu NobodyXu deleted the patch-1 branch August 24, 2024 12:50
@jongiddy
Copy link
Contributor

@Byron We should keep the original minimum test as well, as we need to know when a PR uses a compiler feature that needs rust-version to be increased.

@Byron
Copy link
Member

Byron commented Aug 24, 2024

Strange, I am not seeing how this isn't covered particularly well now. But I agree, ideally nothing gets lost with these changes.

In any case, maybe you want to submit a PR for getting that test back and then I will probably see what I am missing right away.

renovate bot referenced this pull request in oxc-project/oxc Aug 26, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [flate2](https://togithub.com/rust-lang/flate2-rs) |
workspace.dependencies | patch | `1.0.31` -> `1.0.33` |
| [prettyplease](https://togithub.com/dtolnay/prettyplease) |
workspace.dependencies | patch | `0.2.20` -> `0.2.22` |
| [quote](https://togithub.com/dtolnay/quote) | workspace.dependencies |
patch | `1.0.36` -> `1.0.37` |
| [serde](https://serde.rs)
([source](https://togithub.com/serde-rs/serde)) | workspace.dependencies
| patch | `1.0.208` -> `1.0.209` |
| [serde_json](https://togithub.com/serde-rs/json) |
workspace.dependencies | patch | `1.0.125` -> `1.0.127` |

---

### Release Notes

<details>
<summary>rust-lang/flate2-rs (flate2)</summary>

###
[`v1.0.33`](https://togithub.com/rust-lang/flate2-rs/releases/tag/1.0.33):
- fix minimal manifest versions

[Compare
Source](https://togithub.com/rust-lang/flate2-rs/compare/1.0.32...1.0.33)

#### What's Changed

- Fix msrv: Run msrv checks with minimal versions by
[@&#8203;NobodyXu](https://togithub.com/NobodyXu) in
[https://github.com/rust-lang/flate2-rs/pull/425](https://togithub.com/rust-lang/flate2-rs/pull/425)

#### New Contributors

- [@&#8203;NobodyXu](https://togithub.com/NobodyXu) made their first
contribution in
[https://github.com/rust-lang/flate2-rs/pull/425](https://togithub.com/rust-lang/flate2-rs/pull/425)

**Full Changelog**:
rust-lang/flate2-rs@1.0.32...1.0.33

###
[`v1.0.32`](https://togithub.com/rust-lang/flate2-rs/releases/tag/1.0.32):
- turn panic into error

[Compare
Source](https://togithub.com/rust-lang/flate2-rs/compare/1.0.31...1.0.32)

#### What's Changed

##### Fix

- Return error instead of packing on Z_MEM_ERROR by
[@&#8203;crazymerlyn](https://togithub.com/crazymerlyn) in
[https://github.com/rust-lang/flate2-rs/pull/423](https://togithub.com/rust-lang/flate2-rs/pull/423)

##### Other

- prepare new release by [@&#8203;Byron](https://togithub.com/Byron) in
[https://github.com/rust-lang/flate2-rs/pull/416](https://togithub.com/rust-lang/flate2-rs/pull/416)
- update miniz_oxide dependency to 0.8.x by
[@&#8203;oyvindln](https://togithub.com/oyvindln) in
[https://github.com/rust-lang/flate2-rs/pull/420](https://togithub.com/rust-lang/flate2-rs/pull/420)
- update maintenance guide with recent news by
[@&#8203;Byron](https://togithub.com/Byron) in
[https://github.com/rust-lang/flate2-rs/pull/419](https://togithub.com/rust-lang/flate2-rs/pull/419)
- Check minimal version of Rust that compiles by
[@&#8203;jongiddy](https://togithub.com/jongiddy) in
[https://github.com/rust-lang/flate2-rs/pull/421](https://togithub.com/rust-lang/flate2-rs/pull/421)
- Remove non-existent build in CI by
[@&#8203;jongiddy](https://togithub.com/jongiddy) in
[https://github.com/rust-lang/flate2-rs/pull/422](https://togithub.com/rust-lang/flate2-rs/pull/422)

#### New Contributors

- [@&#8203;crazymerlyn](https://togithub.com/crazymerlyn) made their
first contribution in
[https://github.com/rust-lang/flate2-rs/pull/423](https://togithub.com/rust-lang/flate2-rs/pull/423)

**Full Changelog**:
rust-lang/flate2-rs@1.0.31...1.0.32

</details>

<details>
<summary>dtolnay/prettyplease (prettyplease)</summary>

###
[`v0.2.22`](https://togithub.com/dtolnay/prettyplease/releases/tag/0.2.22)

[Compare
Source](https://togithub.com/dtolnay/prettyplease/compare/0.2.21...0.2.22)

- Support formatting precise capture `use<>` syntax
([#&#8203;80](https://togithub.com/dtolnay/prettyplease/issues/80))

###
[`v0.2.21`](https://togithub.com/dtolnay/prettyplease/releases/tag/0.2.21)

[Compare
Source](https://togithub.com/dtolnay/prettyplease/compare/0.2.20...0.2.21)

- Support formatting tail-call `become` expressions
([#&#8203;79](https://togithub.com/dtolnay/prettyplease/issues/79))

</details>

<details>
<summary>dtolnay/quote (quote)</summary>

### [`v1.0.37`](https://togithub.com/dtolnay/quote/releases/tag/1.0.37)

[Compare
Source](https://togithub.com/dtolnay/quote/compare/1.0.36...1.0.37)

- Implement ToTokens for CStr and CString
([#&#8203;283](https://togithub.com/dtolnay/quote/issues/283))

</details>

<details>
<summary>serde-rs/serde (serde)</summary>

###
[`v1.0.209`](https://togithub.com/serde-rs/serde/releases/tag/v1.0.209)

[Compare
Source](https://togithub.com/serde-rs/serde/compare/v1.0.208...v1.0.209)

- Fix deserialization of empty structs and empty tuples inside of
untagged enums
([#&#8203;2805](https://togithub.com/serde-rs/serde/issues/2805), thanks
[@&#8203;Mingun](https://togithub.com/Mingun))

</details>

<details>
<summary>serde-rs/json (serde_json)</summary>

###
[`v1.0.127`](https://togithub.com/serde-rs/json/releases/tag/1.0.127)

[Compare
Source](https://togithub.com/serde-rs/json/compare/1.0.126...1.0.127)

- Add more removal methods to OccupiedEntry
([#&#8203;1179](https://togithub.com/serde-rs/json/issues/1179), thanks
[@&#8203;GREsau](https://togithub.com/GREsau))

###
[`v1.0.126`](https://togithub.com/serde-rs/json/releases/tag/1.0.126)

[Compare
Source](https://togithub.com/serde-rs/json/compare/1.0.125...1.0.126)

- Improve string parsing on targets that use 32-bit pointers but also
have fast 64-bit integer arithmetic, such as
aarch64-unknown-linux-gnu_ilp32 and x86\_64-unknown-linux-gnux32
([#&#8203;1182](https://togithub.com/serde-rs/json/issues/1182), thanks
[@&#8203;CryZe](https://togithub.com/CryZe))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 10am on monday" in timezone
Asia/Shanghai, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/oxc-project/oxc).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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