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

Document MSRV policy and change MSRV to 1.49.0 #547

Closed
wants to merge 8 commits into from

Conversation

notmandatory
Copy link
Member

@notmandatory notmandatory commented Feb 17, 2022

Description

Set MSRV policy to be at most Debian "testing" release version of rustc. Update build-test CI job matrix to use the current latest stable version and MSRV 1.56.0 1.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 latest stable because we haven't been updating it manually and this will ensure we're always testing against current stable rust.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@notmandatory notmandatory changed the title Ci/msrv 156 Document MSRV policy and change MSRV to 1.56.0 Feb 17, 2022
@notmandatory notmandatory added ci documentation Improvements or additions to documentation labels Feb 17, 2022
@notmandatory notmandatory force-pushed the ci/msrv_156 branch 3 times, most recently from 588407f to ad2fbd4 Compare February 17, 2022 06:28
@notmandatory
Copy link
Member Author

notmandatory commented Feb 17, 2022

Prior to merging I also need to change the required github checks to 1.56.0 1.49.0 and stable.

@afilini
Copy link
Member

afilini commented Feb 17, 2022

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

@notmandatory
Copy link
Member Author

notmandatory commented Feb 17, 2022

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 1.48.0

@notmandatory notmandatory force-pushed the ci/msrv_156 branch 2 times, most recently from e1201ae to ef1e19e Compare February 17, 2022 17:28
@notmandatory
Copy link
Member Author

Looks like the tokio MSRV is now 1.49.0.

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
@notmandatory notmandatory changed the title Document MSRV policy and change MSRV to 1.56.0 Document MSRV policy and change MSRV to ~~1.56.0~~ 1.49.0 Feb 17, 2022
@notmandatory notmandatory changed the title Document MSRV policy and change MSRV to ~~1.56.0~~ 1.49.0 Document MSRV policy and change MSRV to 1.49.0 Feb 17, 2022
@notmandatory
Copy link
Member Author

As mentioned by @moneyball on Discord, the LDK team pinned their tokio version to ~1.14 instead of increasing their MSRV, see lightningdevkit/rust-lightning#1315. I believe one downside of this approach is we won't be getting new tokio fixes or features unless that team plans to back port them to 1.14.x releases.

@TheBlueMatt
Copy link

TheBlueMatt commented Feb 17, 2022

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).

@notmandatory
Copy link
Member Author

@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.

@notmandatory notmandatory removed this from the Release 0.17.0 Feature Freeze milestone Feb 18, 2022
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a 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..

@notmandatory notmandatory marked this pull request as draft March 22, 2022 20:13
@ulrichard ulrichard mentioned this pull request Apr 1, 2022
2 tasks
@ulrichard
Copy link
Contributor

I like and support this change.
We recently upgraded all our other stuff to tokio 1.17 and it worked well with bdk up to 0.16. Now I tried upgrading bdk to 0.17 and got conflicts.
With this change in place, we could upgrade to a newer bdk, without having to downgrade tokio.

@notmandatory notmandatory self-assigned this Apr 4, 2022
@notmandatory
Copy link
Member Author

notmandatory commented Apr 29, 2022

Another transitive dependency, http, has now updated their MSRV to 1.49, see #595.

@TheBlueMatt
Copy link

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 cargo update -p tokio --precise "1.14.1", which works to make the ldk-sample build with 1.41 even though by default it will try to build with a newer tokio.

@notmandatory
Copy link
Member Author

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 cargo update -p tokio --precise "1.14.1", which works to make the ldk-sample build with 1.41 even though by default it will try to build with a newer tokio.

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 Cargo.toml for now.

@TheBlueMatt
Copy link

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 :/

@danielabrozzoni
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci documentation Improvements or additions to documentation
Projects
None yet
6 participants