Skip to content

Bump msrv to 1.56.1 #2225

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

Merged
merged 1 commit into from
Jun 23, 2022
Merged

Bump msrv to 1.56.1 #2225

merged 1 commit into from
Jun 23, 2022

Conversation

justsmth
Copy link
Contributor

Older toolchain versions, including 1.56.0, fail msrv check:

[I] ➜ rm Cargo.lock && cargo +1.56.0 build --lib
    Updating crates.io index
error: package `hashbrown v0.12.1` cannot be built because it requires rustc 1.56.1 or newer, while the currently active rustc version is 1.56.0

Copy link
Member

@kulp kulp left a comment

Choose a reason for hiding this comment

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

Hello @justsmth, and thanks for submitting this PR.

I think you did indeed find something that needs to change. However, I think the Cargo.lock file itself should not be updated in this PR.

Removing Cargo.lock during the automation script helps to ensure that resolving dependencies from scratch will work under the given MSRV. However, bindgen may still, as a policy, be conservative when updating Cargo.lock in the repository itself.

In particular, updating a lot of package versions in Cargo.lock entails non-zero risk (unless all the upgrades introduce zero bugs and zero semver violations), and it seems better to keep a mechanical-looking change like "Bump MSRV" free of that kind of risk.

Consequently, I really think this change should be only to the two files:

  • .github/workflows/bindgen.yml
  • README.md

and that the changes to Cargo.lock should be undone.

If you disagree with my reasoning, let me know how. Otherwise, if you force-push a commit that just keeps the two files I mentioned, I think this should be able to merge.

Thanks again!

Copy link
Member

@kulp kulp left a comment

Choose a reason for hiding this comment

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

I am sorry I did not think of the following sooner:

Since this pushes the MSRV past 1.56.0, where rust-version was added to Cargo.toml,
it would seem like a good time to add rust-version = "1.56.1" to Cargo.toml.

If you happen to make this change and find that it works, and if you rebase/squash all the commits down to a single one, I will apply the resulting commit.

The rust-version change is optional, though, so if you would rather get this in as-is, I can squash what you already have.

Let me know; or if I do not hear from you in a day or so, I will squash into master.

Thanks again!

@justsmth
Copy link
Contributor Author

I applied the requested changes then squashed/rebased the commits.

@emilio emilio merged commit a1a0043 into rust-lang:master Jun 23, 2022
@emilio
Copy link
Contributor

emilio commented Jun 23, 2022

Thank you!

@justsmth justsmth deleted the update-msrv branch January 27, 2023 15:53
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