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 #77

Merged
merged 3 commits into from
Aug 15, 2023
Merged

Bump MSRV #77

merged 3 commits into from
Aug 15, 2023

Conversation

ia0
Copy link
Owner

@ia0 ia0 commented Aug 12, 2023

The bump is needed because of proc-macro2.

Also disable lang_items lint for nostd test.

@ia0 ia0 changed the title Disable lang_items lint for nostd test Bump MSRV Aug 15, 2023
@ia0 ia0 merged commit 6e5aa43 into master Aug 15, 2023
2 checks passed
@ia0 ia0 deleted the fix branch August 15, 2023 18:44
@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

This seems wrong and misguided. You don't require the newer version of proc-macro2 but would work fine with the older version too, so that proc-macro2 raised its MSRV does not really affect you apart from cargo not supporting MSRV in any meaningful way.

Ideally cargo would select the older proc-macro2 version here, but it will instead automatically select the latest version unless you have a carefully manually managed Cargo.lock file.

I think this change here should be reverted and instead this should become a cargo issue.

@ia0
Copy link
Owner Author

ia0 commented Nov 8, 2023

Thanks for bringing this to my attention! I've checked quickly the related issues and understand that you need to use an older Rust version and would prefer using data-encoding if it had a lower MSRV. Could you provide me some additional information for me to take the best decision (and not do the same mistake in the future):

  • Why do you need to run on an old compiler?
  • How old the compiler needs to be? (1 year, 2 years, 5 years, ...)
  • I'll need to check this proc-macro2 issue again, which means I might have specific questions about it later.

I hope I'll get a chance to look at this this weekend and hopefully unblock you.

@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

I personally don't care about MSRV at all, but some people have strange requirements and make their requirements everybody else's problem. In this case, someone wants to build tungstenite with data-encoding 2.5.0 or newer with the Rust toolchain provided in Debian bullseye.

But that's also not really any of your problem here. The increase of the MSRV to 1.70 was without reason in data-encoding from what I can see, and should be reverted again. You only need to raise it if you use features from newer Rust versions, or if the minimum version of a crate you depend on requires a newer Rust version. Both is not the case here, it's just that a newer proc-macro2 starts to require a newer Rust, which does not really affect you.

@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

I'm preparing a PR for this btw, including CI checks for the actually correct MSRV.

sdroege added a commit to sdroege/data-encoding that referenced this pull request Nov 8, 2023
This reverts commit 6e5aa43.
@ia0
Copy link
Owner Author

ia0 commented Nov 8, 2023

Thanks a lot! I'll take a look when I get time this week-end (and once you move it out of draft mode).

Regarding your other comments, my current thoughts are:

  • I like the idea of making sure my MSRV is smaller or equal to the packaged rust version of the most common distributions (Ubuntu, Debian, etc). I'll need to find a way to automate that or make sure I don't break it by accident.
  • I should not increase dependency versions if they break MSRV. I have mixed feelings here because increasing dependency versions is also a way to incorporate possible bug fixes. However, I guess I can draw the line at major versions. I would only bump dependency versions when there's a new major version (to avoid downstream users having multiple versions of the same crate because they have differing major versions).

What are your thoughts? How would you balance updating dependency versions and keeping a low enough MSRV?

@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

and once you move it out of draft mode

It's only in draft mode right now because I want your opinion on it first, and because I want the CI to confirm that I didn't miss anything locally :)

I like the idea of making sure my MSRV is smaller or equal to the packaged rust version of the most common distributions (Ubuntu, Debian, etc). I'll need to find a way to automate that or make sure I don't break it by accident.

For that it should be sufficient to check on the CI, or what are you thinking of?

I should not increase dependency versions if they break MSRV. I have mixed feelings here because increasing dependency versions is also a way to incorporate possible bug fixes. However, I guess I can draw the line at major versions. I would only bump dependency versions when there's a new major version (to avoid downstream users having multiple versions of the same crate because they have differing major versions).

You only really need to increase dependency versions if you want to make use of features from newer versions (or want to move to a new major version that changes API in a way that you can't just support both versions). For minor version updates with bugfixes that's something to figure out for whoever is building the final build target that is depending on data-encoding, e.g. some executable.


Related, some people say that bumping the MSRV is a breaking change and should be reflected as such in the version of the crate. I.e. in your case you'd have to update to 3.0.0 here.

I don't agree with that (neither does the proc-macro2 author apparently, and very few crates do from what I can see), FWIW, as it simply causes too much churn in the overall ecosystem and is simply a workaround for cargo not correctly considering the rust-version fields during dependency resolution.

@ia0
Copy link
Owner Author

ia0 commented Nov 8, 2023

It's only in draft mode right now because I want your opinion on it first

I see, in that case I probably would not have reverted the commit, but instead modified the MSRV directly, to reduce the diff (note that the Github workflow is generated, you only need to modify the value in xtask and run cargo xtask generate). I would also select the highest MSRV satisfying the common distribution criteria (so either 1.63 if Debian stable is enough, or 1.48 if Debian oldstable is needed).

For that it should be sufficient to check on the CI, or what are you thinking of?

I'm thinking of fetching the version from Ubuntu and Debian and making sure they are all greater or equal to my MSRV (when I'm bumping the MSRV). So I'll probably introduce a script to bump the MSRV and this script will ensure the criteria are satisfied. I'll make sure MSRV is bumped only by the script by modifying something unrelated but verified by the CI (e.g. having the hash of the MSRV somewhere).

or want to move to a new major version that changes API in a way that you can't just support both versions

What do you mean? My understanding is that this is up to cargo to resolve diamond dependencies with different major versions as always using different versions. I don't think it's possible to merge multiple major versions of the same crate. But anyway, data-encoding doesn't have dependencies, so this really only affects data-encoding-macro which only depends on syn which is stable. So I probably will never need to dump dependency versions, only update them in the lock files.

@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

[...] So either 1.63 if Debian stable is enough, or 1.48 if Debian oldstable is needed

1.63 seems fine to me personally

What do you mean? [...]

Say syn 3.0.0 is released and requires Rust 1.80. Either it doesn't require code changes on your side (you could depend on syn = ">= 2, < 4" then and all is fine, and users of your crate can decide if they want to use syn 2 or syn 3), or it requires code changes and you need to depend on syn = "3" and update your own MSRV to 1.80.

@ia0
Copy link
Owner Author

ia0 commented Nov 8, 2023

Say syn 3.0.0 is released and requires Rust 1.80. Either it doesn't require code changes on your side (you could depend on syn = ">= 2, < 4" then and all is fine, and users of your crate can decide if they want to use syn 2 or syn 3), or it requires code changes and you need to depend on syn = "3" and update your own MSRV to 1.80.

Oh I see, that's very nice. I'll probably have to encode this knowledge somewhere (probably some script to update dependencies to new major versions).

@sdroege
Copy link
Contributor

sdroege commented Nov 8, 2023

I see, in that case I probably would not have reverted the commit, but instead modified the MSRV directly, to reduce the diff [...]

Done, I didn't realize this was all autogenerated. I should look at cargo xtask for my own crates at some point :)

@ia0 ia0 mentioned this pull request Nov 12, 2023
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.

None yet

2 participants