-
Notifications
You must be signed in to change notification settings - Fork 412
Document MSRV policy and change MSRV to 1.49.0 #547
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
Conversation
588407f to
ad2fbd4
Compare
|
Prior to merging I also need to change the required github checks to |
|
I would avoid bumping the MSRV unless we get stuck or have issues somewhere. Maybe our policy could be "at most" debian testing, meaning that we will never go above, but we'll still try to support older versions if we can |
|
I should have linked to the PR that triggered this PR: #522 (comment) Maybe we don't need to bump the MSRV all the way to 1.56.0, I'll see if we can get away with rust |
e1201ae to
ef1e19e
Compare
|
Looks like the |
Not using `split_once` is a warning for `stable` but is not yet supported with our MSRV. https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once
02dde89 to
b43aae3
Compare
|
As mentioned by @moneyball on Discord, the LDK team pinned their |
|
Note that tokio does backport fixes, so I'm not sure how much of a concern it is in practice. As for MSRV of 1.49, that seems exceedingly recent - notably most linux distros still ship 1.41 (and rustup is rather unacceptable for anything security-conscious, like Bitcoin). |
|
@TheBlueMatt, good points. I created #550 as a fix to get our CI working and then we can take our time to decide about any MSRV changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review ACK 7469bd9
edit: Just realized this might not be ready yet.. Will see again once final..
|
I like and support this change. |
|
Another transitive dependency, |
|
Note that even if a dep updates their MSRV you can keep the looser bound on the MSRV and users can pin-back with something like |
That does seem like a clean way to do it, but if we used this approach would I also need to add that command to my CI pipeline matrix for testing with 1.46? If so I think it's easier to pin it in the |
|
Yes, you'd have to do that in CI as well. I suppose it'd be one thing if it were a truly ancient rustc, but 1.41 is still the version shipped on many distros, so its kinda hard to demand that for every user :/ |
|
Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open! |
Description
Set MSRV policy to be at most Debian "testing" release version of rustc. Update
build-testCI job matrix to use the current latest stable version and MSRV1.56.01.49.0.Fixes #331
Fixes #496
Notes to the reviewers
Fixes CI error caused by tokio MSRV changing to 1.49.0, see #522.
I also changed from a fixed stable version (was
1.56.0) to the lateststablebecause we haven't been updating it manually and this will ensure we're always testing against currentstablerust.Checklists
All Submissions:
cargo fmtandcargo clippybefore committing