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

ci(msrv): bump MSRV to 1.75 and rustls to 0.23.21 #159

Merged
merged 3 commits into from
Jan 23, 2025

Conversation

oleonardolima
Copy link
Collaborator

@oleonardolima oleonardolima commented Jan 16, 2025

partially fixes bitcoindevkit/bdk#1750

It bumps the project MSRV to 1.75.0 in order to follow along with LDK changes, and to remove the pinned version of rustls, which now is under 1.71 MSRV, bumping it to latest 0.23.21.

It also updates the clippy step on CI to use rust 1.84, fixing the found clippy issues.

@oleonardolima oleonardolima changed the title Ci/bump msrv to 1.75 ci(msrv): bump MSRV to 1.75 and rustls to 0.23.21 Jan 20, 2025
@oleonardolima oleonardolima self-assigned this Jan 20, 2025
@oleonardolima oleonardolima marked this pull request as ready for review January 20, 2025 16:37
README.md Outdated Show resolved Hide resolved
- bumps the MSRV on CI to 1.75.
- updates the `Cargo.toml` rust-version to 1.75.
- removes the guideline to pin `rustls` version, as previously required
  by old 1.63 MSRV.
- update `README.md` badges to `1.75` MSRV.
- update `clippy.toml` to `1.75` MSRV.
- updates the rust version on clippy step to `1.84`
- fixes the found clippy issues
@ValuedMammal
Copy link
Contributor

Looks good to me, except CI still believes that Test (1.63.0) is a required check?

@oleonardolima
Copy link
Collaborator Author

Looks good to me, except CI still believes that Test (1.63.0) is a required check?

Yes, that's weird. I guess it could be ignored, seems to not be blocking.

notmandatory added a commit to bitcoindevkit/bdk that referenced this pull request Jan 21, 2025
c8b55da chore: fix punctuation and use `rustup override set` instead (Leonardo L.)
78bbfbe ci: add `pin-msrv.sh` (valued mammal)
f7c9bc5 docs: update `README.md` with MSRV policy (valued mammal)
12eabd1 chore(cleanup): remove rustls from pinned deps (Leonardo Lima)
e3faeb6 ci: add secondary MSRV job of 1.75.0 (Leonardo Lima)

Pull request description:

  partially fixes #1750

  <!-- You can erase any parts of this template not applicable to your Pull Request. -->

  ### Description

  <!-- Describe the purpose of this PR, what's being adding and/or fixed -->

  It adds a secondary MSRV version of `1.75.0` for the `bdk_electrum` crate, excluding it from the `1.63.0` MSRV dependency pinning and CI step.

  As it's the new CI will run without `bdk_electrum` for `1.63.0` MSRV step, will run another step for `1.75.0` MSRV and the latest stable.

  Currently,, the only affected crate is `electrum-client`, therefore `bdk-electrum`, as it relies directly on `rustls` which migrated to `1.71.0` MSRV. The `esplora-client` relies on `rustls` as a dependency of `minreq` or `reqwest`, but those crates didn't upgraded it's `rustls` versions yet.

  In a further PR it should also bump the `electrum-client` crate to it's latest version, which relies on `1.75.0` MSRV, see bitcoindevkit/rust-electrum-client#159.

  ### Notes to the reviewers

  <!-- In this section you can include notes directed to the reviewers, like explaining why some parts
  of the PR were done in a specific way -->

  It's open for discussion if this approach is the expected one, or another one would be better.

  ### Changelog notice

  <!-- Notice the release manager should include in the release tag message changelog -->
  <!-- See https://keepachangelog.com/en/1.0.0/ for examples -->

  - Add a secondary MSRV of `1.75.0` for `bdk_electrum`.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [ ] I've added tests for the new feature
  * [ ] I've added docs for the new feature

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [ ] I've added tests to reproduce the issue which are now passing
  * [ ] I'm linking the issue being fixed by this PR

ACKs for top commit:
  notmandatory:
    ACK c8b55da

Tree-SHA512: 186d82d6d295c688a608e1dd457e2718e0ae3ee92b7d1a0aad094711c3baeb70dccd581e3067e6305c1fc702c78f1704399f7472a5ae62824041ac68c384b770
Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3dc4dac

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3dc4dac

before we merge this I'd like to get a 👍 from top production dependents:

@notmandatory
Copy link
Member

I update the CI required MSRV test version to 1.75.0.

@tnull
Copy link
Contributor

tnull commented Jan 22, 2025

ACK 3dc4dac

before we merge this I'd like to get a 👍 from top production dependents:

* ldk_node (current MSRV 1.63) @tnull

* lightning-transaction-sync (no MSRV?) @TheBlueMatt

* bdk_reserves (current MSRV 1.63) @ulrichard

Can confirm the new MSRV for LDK Node and lightning-transaction-sync is 1.75.0 as well.

@ulrichard
Copy link

Ok with me.
At Amina our benchmark is what is shipped with Alpine. That is currently 1.78
If possible I like to be compatible with what is shipped in Debian stable. That would be 1.63
But maintaining such old versions is a hassle.

@tnull
Copy link
Contributor

tnull commented Jan 22, 2025

If possible I like to be compatible with what is shipped in Debian stable. That would be 1.63

Yes, on our end we tried very hard to stay on 1.63, too. Unfortunately we depend on rustls and the maintainers weren't very accommodating but rather very clear they were going ahead with the MSRV bump.

@notmandatory notmandatory merged commit 805ea0a into bitcoindevkit:master Jan 23, 2025
4 checks passed
@notmandatory
Copy link
Member

Thanks all for the feedback. I'll work with @oleonardolima to figure out if anything else needs to be done before publishing a new release of this crate.

@oleonardolima oleonardolima deleted the ci/bump-msrv-to-1.75 branch January 27, 2025 14:12
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.

Consider bumping MSRV
5 participants