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

Set an msrv requirement and test it on CI #81

Closed
Veetaha opened this issue Aug 29, 2024 · 8 comments · Fixed by #101
Closed

Set an msrv requirement and test it on CI #81

Veetaha opened this issue Aug 29, 2024 · 8 comments · Fixed by #101

Comments

@Veetaha
Copy link
Collaborator

Veetaha commented Aug 29, 2024

The current code is tested with 1.80.1 on CI, but I tried to keep it compatible with older versions manually.
We need to set the required-rust-version in Cargo.toml and the MSRV version to the tests on CI. CI should have matrix jobs for each check that validate the code on both MSRV and latest stable

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Aug 30, 2024

Personally I would try to get 1.63 (or earlier) as MSRV in order to support current Debian bullseye. That would allow for tools depending on bon to be packaged natively for Debian Bullseye.

Also, -Zmsrv-policy (part of the upcoming resolver = "3" / edition 2024) should help with that. See rust-lang/cargo#13873

The following graphic contains the Rust version in currently supported (no out of support) distros known to repology.org:

Packaging status

@Veetaha
Copy link
Collaborator Author

Veetaha commented Aug 30, 2024

Thank you for the insight! derive-builder supports 1.56, although it depends on syn 2.0 that supports 1.61+. I believe it's better to follow syn in this regard, since any change to syn may accidentally break its current untested compatibility with rustc 1.56.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Aug 30, 2024

Correction syn 2.0.56 supports rust version 1.56, but syn 2.0.57 bumped that requirement. I think to be able to replace derive-builder we need to do the same then (1.56 MSRV). Not sure how hard it'll be and if it will be hard I'm fine with using 1.61 as MSRV to follow the cutting edge of syn. The advantage of derive-builder that makes it easy for them to support 1.56 is that the crate is mostly feature complete and slow-moving, while bon is moving fast.

@EdJoPaTo
Copy link
Contributor

Personally I think it's fine to bump MSRV when the rust-version on the published versions on crates.io is correct. That way an older version of bon can still be used when an older version of Rust (like on Debian) is required for some reason.

Also, something can have an older MSRV when a dependency of it has a newer one. -Zmsrv-policy will still be able to figure it out. And crate patches also exist to adapt / remove code from some crate.

@Veetaha
Copy link
Collaborator Author

Veetaha commented Aug 30, 2024

Personally I think it's fine to bump MSRV when the rust-version on the published versions on crates.io is correct

What do you mean by "when the rust-version is correct"? If it's incorrect, then it's a bug

@Veetaha
Copy link
Collaborator Author

Veetaha commented Sep 3, 2024

I've set MSRV to 1.70.0 for now. The problem with making it lower is that crates we use for testing (such as trybuild, for example), don't compile on versions lower than that. I may consider lowering this requirement in the future if a compelling use case emerges or once I just have enough will to fight with this

@Veetaha
Copy link
Collaborator Author

Veetaha commented Sep 3, 2024

I created a followup issue to keep lowering the MSRV (#102). This first iteration is just a good enough first attempt at lowering it. So no reason to be sad

@Veetaha
Copy link
Collaborator Author

Veetaha commented Sep 8, 2024

This feature is now available in bon 2.2. Here is the release blog post on Reddit

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 a pull request may close this issue.

2 participants