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

Remove explicit and implied references to specific MSRVs #2257

Closed
wants to merge 1 commit into from

Conversation

TheBlueMatt
Copy link

After conversation in Twitter DM re: #2255, it became clear that
serde does not, in fact, have a specific MSRV policy, but rather
strives to be buildable with older rustc as only a practical
matter.

This led to some confusion from users, as the rust-version
Cargo.toml tag is often used to communicate a specific MSRV policy.

Thus, this PR simply removes references which could be confused for
a specific MSRV policy, leaving a comment in CI that the test with
older rustc's does not imply a specific policy.

After conversation in Twitter DM re: serde-rs#2255, it became clear that
serde does not, in fact, have a specific MSRV policy, but rather
strives to be buildable with older rustc as only a practical
matter.

This led to some confusion from users, as the `rust-version`
Cargo.toml tag is often used to communicate a specific MSRV policy.

Thus, this PR simply removes references which could be confused for
a specific MSRV  policy, leaving a comment in CI that the test with
older rustc's does not imply a specific policy.
@taiki-e
Copy link
Contributor

taiki-e commented Aug 21, 2022

#2255 has been fixed in 1.0.144: https://docs.rs/crate/serde/1.0.144/source/Cargo.toml

Also, the rust-version field can only contain a version, which is clearly insufficient to explain the MSRV policy, which is the policy that indicates whether the MSRV can be raised in what cases, whether the MSRV increase should be treated as a breaking change, how to treat accidental breakage of MSRV, etc. So, your point that rust-version fields should not be set if there is no MSRV policy does not seem to be reasonable.

@TheBlueMatt
Copy link
Author

Irrespective of the other parts of MSRV policies, which I agree are nuanced, "rust-version" being specified seems like it should tell users that "the crate, at the current version, supports the given rustc". In this case, based on statements from the serde maintainer, the rust-version field is set incorrectly.

Sure, it'd be great to also have a formal policy laid out answering the questions you raised, but my only point in this PR was to remove things which had clearly confused many downstream users (myself included) in #2255.

@taiki-e
Copy link
Contributor

taiki-e commented Aug 21, 2022

my only point in this PR was to remove things which had clearly confused many downstream users (myself included) in #2255.

The problem that occurred in #2255 was an accidental MSRV breakage due to a cargo bug. And serde did not have a policy that indicated how to handle such cases, and (IIUC) the maintainer considered it not a critical enough issue to require a new release to work around it, so they seemed to leave the problem until the cargo bug was fixed. (Note also that the fix had already been submitted at the time this your comment was posted.)

It seems that some people requested a new release to work around a cargo bug, and after it was dismissed (via your comment), requested that it reflect the accidentally increased MSRV. (And this PR seems to reflect the second request?) That request was also not accepted because, that is not at all surprising considering it was known that it would not be needed after a cargo bug was fixed.

In any case, the problem has already been fixed and the rust-version field reflects the actual MSRV, so I believe there is no need to remove the rust-version field.

@TheBlueMatt
Copy link
Author

I don't think you addressed my point at all. As you note, "the rust-version field reflects the actual MSRV," ie it represents the minimum-supported-rust-version, ie the version of rust on which the code is expected to compile, and on which the maintainers support building.

You go on to say

That request was also not accepted because it was known that it would not be needed after a cargo bug was fixed.

which doesn't seem to have a source? As far as I can tell, you aren't the maintainer of this crate, and this PR was actually in response to a direct quote from the maintainer, which said:

the number in the readme is descriptive, not a policy; i'll update it

here I'm trying to implement exactly what the maintainer indicated he wished to do by having a PR implementing it.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Regarding whether people can be confused by the inclusion of rust-version metadata, people are free to get confused by whatever they want but @taiki-e has explained in #2257 (comment) why interpreting rust-version metadata as a MSRV policy is silly.

Obviously there can be benefits to a precise policy being written down, but @TheBlueMatt is ignoring the cost/downside, which I claim dwarfs any benefit. We have experience from libc that writing a policy is a 100,000 word endeavor with a zillion over-passionate stakeholders, so I do not intend to get sucked into that because everyone involved can make vastly better use of their time instead. The policy is there is no policy and you get what you get depending on how sympathetic I feel to your use case. With that, I'll close the PR because I think all of the files modified by the PR are fine as is, and let's avoid churning further on this.

@dtolnay dtolnay closed this Aug 21, 2022
@serde-rs serde-rs locked and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants