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

Bump MSRV to Rust 1.64 and raise edition to 2021 #2199

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

mkrasnitski
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added the ci Related to the continuous integration. label Oct 2, 2022
@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Oct 2, 2022

Clippy lints will be fixed in a followup PR, this is a minimal PR to just bump MSRV.

@GnomedDev
Copy link
Member

Should we wait to do this until just before next version releases and is put on current? As MSRV checks aren't run on next anymore

@mkrasnitski
Copy link
Collaborator Author

Better to regularly bump MSRV so we can use new language features as they're stabilized.

@GnomedDev
Copy link
Member

Yes, but I'm saying that on next MSRV shouldn't need even a CI PR to bump, MSRV is literally "the current latest version of rust" and the actual MSRV is set in stone once it releases.

@mkrasnitski
Copy link
Collaborator Author

I think keeping the bumps explicit, keeping CI in lockstep with each Rust release, is the better option, since right now serenity's major release schedule is quite long. It'll be harder to forget to bump it before a major release.

@GnomedDev
Copy link
Member

I don't exactly see your reasoning? Is it in the case that serenity releases breaking versions much more frequently the current MSRV could forget to be bumped? Seems unlikely since (at least in my mind) 1.0 should probably happen at some point.

@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Oct 2, 2022

1.0 releasing or not is besides the point. But it's the opposite of what you say. If there are long periods between releases, without updating CI to reflect MSRV, then when a major release is cut it might be easy to forget.

Maybe it's worth setting up autohooks to update the CI every time a new stable release comes out? For now at least though, I think we should explicitly update each time.

@kangalio
Copy link
Collaborator

kangalio commented Oct 2, 2022

When we release, don't we move next over to current, thereby checking MSRV automatically?

@mkrasnitski
Copy link
Collaborator Author

Rather, a major release would require next already be on top of current (through a previous rebase), but what you suggest would require filing a PR to pull current up to next so that the MSRV check could be run. However, keep in mind that just updating CI isn't enough, you'd have to remember to update the README. Also, clippy lints keep track of their own MSRV and don't run if clippy.toml has an MSRV lower then theirs. So, that's an additional benefit this gives us (note that this has already happened if you look at the clippy run for this PR).

@kangalio
Copy link
Collaborator

kangalio commented Oct 2, 2022

what you suggest would require filing a PR to pull current up to next so that the MSRV check could be run

Yes (not what I originally had in mind, but yours makes more sense)

However, keep in mind that just updating CI isn't enough, you'd have to remember to update the README

That can be done in the PR, right?

@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Oct 2, 2022

I mean, sure. The point about clippy lints still stands though. Also, someone working off the next branch might look at the MSRV mentioned in the README and wrongly assume that they can use an older compiler, when in reality they can't.

The source of contention here is that the MSRV check is skipped on next, but I think maintaining coherence even if the CI check isn't explicitly run is still important. The point of skipping it is that it's redundant, but if the MSRV is simply implicit on next, then that's a pain to have to figure out.

Committing to an MSRV should be something done in writing, logged in the git branch. Say at some point in the future the MSRV policy changes to not track the most recent Rust release automatically, then it would be pertinent to actually have an up-to-date value written down.

Is it really so much churn to explicitly PR an MSRV bump every six weeks?

@kangalio
Copy link
Collaborator

kangalio commented Oct 2, 2022

The point about clippy lints still stands though

We can just remove the msrv = line in clippy.toml on next. Then we don't need to update it every 6 weeks

Also, someone working off the next branch might look at the MSRV mentioned in the README and wrongly assume that they can use an older compiler, when in reality they can't.

In my opinion the solution is to add a paragraph to the README explaining that next's MSRV is always latest stable. Then we can have the same README on next and current and don't need to awkwardly maintain two separate README versions.

Committing to an MSRV should be something done in writing, logged in the git branch. Say at some point in the future the MSRV policy changes to not track the most recent Rust release automatically, then it would be pertinent to actually have an up-to-date value written down.

I don't understand the point. Once we change MSRV policy to be fixed to a certain version, we can write down that version, sure. But why worry about that now?

@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Oct 2, 2022

We can just remove the msrv = line in clippy.toml on next. Then we don't need to update it every 6 weeks

We'd have to add it back on current when we cut a major release. That's yet another task to take care of that risks being forgotten about.

In my opinion the solution is to add a paragraph to the README explaining that next's MSRV is always latest stable. Then we can have the same README on next and current and don't need to awkwardly maintain two separate README versions.

This is already the case.

I don't understand the point. Once we change MSRV policy to be fixed to a certain version, we can write down that version, sure. But why worry about that now?

So we don't have to worry about it in the future. There's already a separate open PR (#2173) that makes use of some newer features, and it elects to update CI to reflect MSRV. If this PR is merged, no future PRs will feel the need to do that, or even assume they might have to. Keeping MSRV up to date in a timely fashion eases the maintenance burden of all the other maintainers and drive-by contributors, and reduces (however small a reduction it may be) some mental load.

It also (selfishly) satisfies my want to see this process through to the end. It makes me a bit sad that the last step of that process is being met with consternation. The alternatives proposed seem like more work, not less.

Also, don't get me wrong, future PRs to bump MSRV on next should not be seen as blockers to the usage of any new language features. People shouldn't wait for serenity's MSRV to catch up before implementing things. Ideally that catch-up would happen quickly (as in, on the day of stable's release), but it's flexible precisely because CI doesn't check for it anymore.

@vaporoxx
Copy link
Contributor

vaporoxx commented Oct 2, 2022

FWIW, starting with Rust 1.56, the msrv field in clippy.toml can be replaced with a rust-version field in Cargo.toml, which is also used to error when trying to build serenity with an outdated Rust version. See [1] and [2]

@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Oct 2, 2022

Yes, it's worth considering doing that. That reminds me, we should also probably switch to edition2021 if we're pushing MSRV past 1.56. Thoughts on combining that with this PR, or should I file a separate one?

@mkrasnitski mkrasnitski changed the title Bump MSRV to Rust 1.64 Bump MSRV to Rust 1.64 and raise edition to 2021 Oct 3, 2022
@github-actions github-actions bot added builder Related to the `builder` module. model Related to the `model` module. labels Oct 3, 2022
@arqunis arqunis added enhancement An improvement to Serenity. breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users labels Oct 3, 2022
@arqunis arqunis merged commit f7f6162 into serenity-rs:next Oct 3, 2022
@mkrasnitski mkrasnitski deleted the bump_msrv branch October 4, 2022 22:32
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Nov 7, 2022
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Feb 28, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request May 18, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request May 30, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Sep 21, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 17, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The public API is changed, resulting in miscompilations or unexpected new behaviour for users builder Related to the `builder` module. ci Related to the continuous integration. enhancement An improvement to Serenity. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants