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: add secondary MSRV job of 1.75.0 #1803

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Jan 16, 2025

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
@oleonardolima oleonardolima self-assigned this Jan 20, 2025
@oleonardolima oleonardolima marked this pull request as ready for review January 20, 2025 16:45
@ValuedMammal
Copy link
Contributor

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
Contributor Author

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor Author

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.

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.

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
ci/pin-msrv.sh Outdated Show resolved Hide resolved
oleonardolima and others added 5 commits January 21, 2025 16:51
- bdk_electrum: add MSRV section to README.md
- add ci/pin-msrv.sh, which pins the dependencies for 1.63.0 MSRV
- fix some nits and punctuation typos.
- recommend usage of `rustup override set` instead of `rustup default` command.

Co-authored-by: Steve Myers <[email protected]>
@oleonardolima oleonardolima force-pushed the ci/add-secondary-msrv-build-test-job branch from d411cec to c8b55da Compare January 21, 2025 19:51
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 c8b55da

@notmandatory
Copy link
Member

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

@notmandatory notmandatory merged commit 7664574 into bitcoindevkit:master Jan 21, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Consider bumping MSRV
3 participants