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

Rust 2021 edition is a major, not a "patch" change #53

Closed
link2xt opened this issue Jan 24, 2022 · 6 comments
Closed

Rust 2021 edition is a major, not a "patch" change #53

link2xt opened this issue Jan 24, 2022 · 6 comments

Comments

@link2xt
Copy link

link2xt commented Jan 24, 2022

I noticed commit f8cb34b

Once you release this as a "patch" you will likely get bug reports that it breaks builds for people using Rust < 1.56.0 and have to yank the version and re-release as major or revert it. This change unnecessarily raises MSRV.

@ia0
Copy link
Owner

ia0 commented Jan 24, 2022

Thanks for bringing this issue to my attention! I wasn't aware that it would break the build for old compilers. I'll take a look at it this weekend to see how to address the issue (revert, document, or something else).

@link2xt
Copy link
Author

link2xt commented Jan 24, 2022

I would prefer revert, because https://github.com/deltachat/deltachat-core-rust tries to support Rust >=1.51 (this version introduced const generics which are required by rustcrypto crates) and depends on data-encoding via some crates, including mailparse soon.

Also since you have a CI setup, it makes sense to fix some MSRV and automatically test that the crate can still be built with it. If some change breaks compatibility you will notice it and can decide whether it's worth rising the minimum version.

@ia0
Copy link
Owner

ia0 commented Jan 24, 2022

Indeed, reverting sounds like the best approach for now, but I'd like to check if there are alternatives before committing. For sure, the priority is to not break users.

Thanks for the CI idea, I'll definitely do that.

ia0 added a commit that referenced this issue Jan 27, 2022
This partially reverts commit f8cb34b to avoid
breaking users with old compilers. Only publicly visible libraries are reverted.
See #53 for more details.
@ia0
Copy link
Owner

ia0 commented Jan 27, 2022

It looks like there are no clear guidelines on how an MSRV bump should impact the crate version. The Rust Embedded WG only restricts to the latest stable.

Given that the data-encoding library is stable, I'll take the most conservative approach and only bump the MSRV when needed. I'll also do the same for the unstable macro libraries for now, but might revisit this in the future. The binary is out of scope since it cannot break a user build. Similarly, non-exported crates are out of scope.

The smallest MSRV is currently 1.46.0 according to cargo-msrv. The MSRV is documented in the package.rust-version field of each library and is checked in continuous integration.

If this sounds good to you, I'll submit #54.

Thanks again for bringing this issue to my attention before it was too late!

@link2xt
Copy link
Author

link2xt commented Jan 27, 2022

Checked #54, looks good.

@ia0
Copy link
Owner

ia0 commented Jan 27, 2022

Thanks! Merging.

@ia0 ia0 closed this as completed Jan 27, 2022
ia0 added a commit that referenced this issue Jan 27, 2022
This partially reverts commit f8cb34b to avoid
breaking users with old compilers. Only publicly visible libraries are reverted.
See #53 for more details.
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

No branches or pull requests

2 participants