Skip to content

ci: add secondary MSRV job of 1.75.0#1803

Merged
notmandatory merged 5 commits intobitcoindevkit:masterfrom
oleonardolima:ci/add-secondary-msrv-build-test-job
Jan 21, 2025
Merged

ci: add secondary MSRV job of 1.75.0#1803
notmandatory merged 5 commits intobitcoindevkit:masterfrom
oleonardolima:ci/add-secondary-msrv-build-test-job

Conversation

@oleonardolima
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima commented Jan 16, 2025 •
edited
Loading

partially fixes #1750

Description

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

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

Changelog notice

  • Add a secondary MSRV of 1.75.0 for bdk_electrum.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • 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

@oleonardolima oleonardolima force-pushed the ci/add-secondary-msrv-build-test-job branch 4 times, most recently from fd10d46 to 42c2a4b Compare January 16, 2025 23:44
@ValuedMammal ValuedMammal mentioned this pull request Jan 17, 2025
Closed
@oleonardolima oleonardolima self-assigned this Jan 20, 2025
@oleonardolima oleonardolima marked this pull request as ready for review January 20, 2025 16:45
@oleonardolima oleonardolima mentioned this pull request Jan 20, 2025
Closed
@ValuedMammal
Copy link
Copy Markdown
Collaborator

ValuedMammal commented Jan 20, 2025

I sort of like the idea of separating the "check" and "test" jobs. For example this CI run https://github.com/ValuedMammal/bdk/actions/runs/12872751304

@oleonardolima
Copy link
Copy Markdown
Collaborator Author

oleonardolima commented Jan 20, 2025

I sort of like the idea of separating the "check" and "test" jobs. For example this CI run https://github.com/ValuedMammal/bdk/actions/runs/12872751304

That's a good idea, I'll cherry-pick the commit.

@oleonardolima
Copy link
Copy Markdown
Collaborator Author

oleonardolima commented Jan 20, 2025

I sort of like the idea of separating the "check" and "test" jobs. For example this CI run https://github.com/ValuedMammal/bdk/actions/runs/12872751304

That's a good idea, I'll cherry-pick the commit.

However, @ValuedMammal any reason to not have tests run with --no-default-features too?

@ValuedMammal
Copy link
Copy Markdown
Collaborator

ValuedMammal commented Jan 20, 2025

I sort of like the idea of separating the "check" and "test" jobs. For example this CI run https://github.com/ValuedMammal/bdk/actions/runs/12872751304

That's a good idea, I'll cherry-pick the commit.

However, @ValuedMammal any reason to not have tests run with --no-default-features too?

I thought the same thing and I don't know. It didn't seem like that would pick up any tests that weren't already included in --all-features, but in any case I'm in favor of running all possible tests.

@oleonardolima
Copy link
Copy Markdown
Collaborator Author

oleonardolima commented Jan 21, 2025

I sort of like the idea of separating the "check" and "test" jobs. For example this CI run https://github.com/ValuedMammal/bdk/actions/runs/12872751304

That's a good idea, I'll cherry-pick the commit.

However, @ValuedMammal any reason to not have tests run with --no-default-features too?

I thought the same thing and I don't know. It didn't seem like that would pick up any tests that weren't already included in --all-features, but in any case I'm in favor of running all possible tests.

I cherry-picked and add the features to matrix, but it now fails because not all crates has miniscript/no-std, I'll need to sort that out.

@notmandatory notmandatory added this to the 1.1.0 milestone Jan 21, 2025
@oleonardolima oleonardolima force-pushed the ci/add-secondary-msrv-build-test-job branch from 735c116 to ac3dcd7 Compare January 21, 2025 17:26
@oleonardolima
Copy link
Copy Markdown
Collaborator Author

oleonardolima commented Jan 21, 2025

Alright, I kept it all under a single build-test job, using the ./ci/pin-msrv.sh script and referring to it on README.md.

It should be ready for review.

notmandatory
notmandatory reviewed Jan 21, 2025
View reviewed changes
Copy link
Copy Markdown
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.

Looks good, I found a couple nits to fix and it needs to be rebased now that #1801 is merged.

oleonardolima and others added 5 commits January 21, 2025 16:51
@oleonardolima
ci: add secondary MSRV job of 1.75.0
e3faeb6
@oleonardolima
chore(cleanup): remove rustls from pinned deps
12eabd1
@ValuedMammal @oleonardolima
docs: update README.md with MSRV policy
f7c9bc5 
- bdk_electrum: add MSRV section to README.md
@ValuedMammal @oleonardolima
ci: add pin-msrv.sh
78bbfbe 
- add ci/pin-msrv.sh, which pins the dependencies for 1.63.0 MSRV
@oleonardolima @notmandatory
chore: fix punctuation and use rustup override set instead
c8b55da 
- fix some nits and punctuation typos.
- recommend usage of `rustup override set` instead of `rustup default` command.

Co-authored-by: Steve Myers <github@notmandatory.org>
@oleonardolima oleonardolima force-pushed the ci/add-secondary-msrv-build-test-job branch from d411cec to c8b55da Compare January 21, 2025 19:51
notmandatory
notmandatory approved these changes Jan 21, 2025
View reviewed changes
Copy link
Copy Markdown
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 c8b55da

@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Jan 21, 2025

I updated Github required checks to match changes in this PR.

@notmandatory notmandatory merged commit 7664574 into bitcoindevkit:master Jan 21, 2025
@notmandatory notmandatory mentioned this pull request Jan 23, 2025
Merged
3 tasks
@ValuedMammal ValuedMammal mentioned this pull request Feb 4, 2025
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@notmandatory notmandatory notmandatory approved these changes

@ValuedMammal ValuedMammal Awaiting requested review from ValuedMammal

@LagginTimes LagginTimes Awaiting requested review from LagginTimes

Assignees

@oleonardolima oleonardolima

Labels

None yet

Projects

BDK Wallet
Archived in project

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

Consider bumping MSRV

3 participants

@oleonardolima @ValuedMammal @notmandatory