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

docs: add explanation of how to check MSRV locally #5851

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bryceberger
Copy link
Member

@bryceberger bryceberger commented Mar 1, 2025

There have been a few attempts to add a rust-toolchain.toml file to pin developers' rust version. The most recent attempt was reverted.

The current opinion (comment) is that using a newer compiler version is desired, and MSRV violations should be caught by clippy::incompatible_msrv. This commit writes that opinion in the docs/contributing.md.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@bryceberger bryceberger force-pushed the bryce/push-kwkklvxrwukl branch from ae60c62 to 0306b10 Compare March 1, 2025 20:17
@ilyagr
Copy link
Contributor

ilyagr commented Mar 1, 2025

I'm a little confused by your "Note". What's the story there? Is there no way to enforce that lint in tests by more regular means?

I was thinking we should update our MSRV to 1.84 (IIRC, maybe it was 1.85) for the MSRV-aware cargo resolver, btw (also in preparation for some of our dependencies switching to 2024 Rust edition and requiring 1.85, which I'm guessing will happen), and we could move up to 1.87 in a few months (I'm still using the debian stable testing rustc version as a rough guide for the maximum MSRV, which also seems to correspond to "stable - 1").

Enforcing the incompatible-msrv lint would seem like a good reason to update the MSRV as well (and you're saying 1.78 is sufficient for that, though my impression is that people are fine with moving straight to 1.84 or something). Is it not enabled by default in newer versions?

There have been a few attempts to add a `rust-toolchain.toml` file to
pin developers' rust version. The most recent attempt was [reverted].

The current opinion ([comment]) is that using a newer compiler
version is desired, and MSRV violations should be caught by
`clippy::incompatible_msrv`. This commit writes that opinion in the
`docs/contributing.md`.

[reverted]: #5090

[comment]: #1913 (comment)
@bryceberger
Copy link
Member Author

Is incompatible-msrv not enabled by default in newer versions?

It is, but explicitly giving the name of the lint tells what to look out for and will give a warning if they're using a version that doesn't support the lint.

Is there no way to enforce that lint in tests by more regular means?

Unfortunately, I don't think there is. rust-lang/rust-clippy#12257 argues that tests shouldn't be checked (since they're not required to use as a library). rust-lang/rust-clippy#14279 was just merged, which allows it to run in test code again, but requires a config option.

MSRV-aware resolver is 1.84, edition2024 is 1.85.

In terms of updating MSRV, I know it's been said somewhere that we follow helix, which in turn follows firefox. Currently FF is still on 1.76, but will be on 1.82 in April (https://firefox-source-docs.mozilla.org/writing-rust-code/update-policy.html).

@bryceberger bryceberger force-pushed the bryce/push-kwkklvxrwukl branch from 0306b10 to 51acff7 Compare March 1, 2025 20:42
@ilyagr
Copy link
Contributor

ilyagr commented Mar 1, 2025

I would prefer upgrading our MSRV to documenting the incompatible-msrv lint, especially since a relatively small upgrade will do. We might still need to document the issue with tests, though it also seems less important to me.

I personally would also make a larger jump in MSRV, seems worth it to me. Unlike helix/firefox, we are not packaged in conservative distros yet. Once we are, it might make sense to become as conservative as them, which might also create a headache when our dependencies have a more aggressive MSRV. (BTW, I meant "debian testing" in the above comment, fixed now, pretty significant typo).

I'm not sure that there is a consensus, but the MSRV policy I'd personally suggest for now is: don't bump it for no reason, but feel free to bump it for any reason that saves anyone time, up to the "debian testing" version ("Rust stable - 1" is probably also fine, though I still check Debian. For example, that might alert me if there was something subtly wrong with "rust stable -1" at some point in the future)

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.

2 participants