Test with minimal dependency versions by tcharding · Pull Request #1764 · rust-bitcoin/rust-bitcoin

tcharding · 2023-03-29T23:06:16Z

This is work originally done by Kixunil in #1272, I picked it up to help out. The only changes I made were rebasingg, updating the recent lock file, adding --locked to hashes contrib file, and adding a co-developed-by tag for accountability.

It could happen that we unknowingly depend on a new version of a crate without updating Cargo.toml. This could cause resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate.

This change commits two lock files: minimal and recent. minimal contains minimal dependency versions, while recent contains dependency versions at the time of making the change.

Further, this adds CI jobs to test with both lock files, CI job for internals crate, removes old serde pinning and prints a warning if recent is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.)

The documentation is also updated accordingly.

Closes #1230

6c61e10 Fix pinning (schemars and MSRV) (Tobin C. Harding) c8e38d6 hashes: Implement JsonSchema for sha256t::Hash<T> (Tobin C. Harding) Pull request description: This has grown due to now including pinning work also done in #1736, I decided to do this because the PRs conflict and doing it all here saves accidentally getting out of sync. And #1764 requires this PR. - Patch 1 is unchanged - Patch 2 now fixes pinning in bitcoin and hashes CI scripts and in the docs of both as well as the manifest stuff relating to `schemars` - phew. Fix: #1687 ACKs for top commit: Kixunil: ACK 6c61e10 apoelstra: ACK 6c61e10 Tree-SHA512: eae4aa9700817bab6ad444e07709e8b1a4ffb1625e08be6ba399abde38bf6f8e5ea216a0836e2e26dfaddc76c392802cd016738ea6e753a1bca2584d9d2a9796

apoelstra

ACK 5a6332d5dfd4733dd0390e26de88d81c69772e99

apoelstra · 2023-04-20T01:35:42Z

In my local scripts, I have a comment that says "no-std does not work on 1.48" and I test with 1.50 instead. @tcharding that may be what you need to do here.

tcharding · 2023-04-20T04:17:54Z

I don't understand why this is showing up in this PR? If we cannot build "no-std" with 1.48 why is CI ok for current master?

Kixunil · 2023-04-20T09:19:17Z

I think this is caused by old core2? We should probably bump its version. (IDK to which number)

apoelstra · 2023-04-20T18:50:16Z

Neat. Apparently core2 0.3.2 works. I guess we need to set our minimal version to that.

tcharding · 2023-04-20T22:43:32Z

Changes in force push

Added a patch to the front of the PR to set core2 dependency to 0.3.2
Updated the Cargo-minimal.lock file after building with the new core2 dependency

tcharding · 2023-04-21T00:05:57Z

This is blocking hex crate work.

apoelstra

ACK c129eb6d0016ddf110c9ed72300eee53a7ca769d

stevenroose · 2023-04-24T17:40:20Z

I haven't followed the details of this MR, but would it be hard to have support for this in the github CI matrix thing? So specify different versions to test for certain deps and then use update --precise for those in the CI script?

I'm asking because in dependent crates it'd be nice to have easy access to ways to support multiple rust-bitcoin versions as well.

Ofc this would be a great tool for the rust ecosystem as a whole, but as we know I don't think many other projects care enough about msrv and dep mgmt that this kind of thing would already exist in the CI tooling.

stevenroose · 2023-04-24T17:43:00Z

contrib/test.sh

Is this like a format that GitHub CI will pick up and somehow make visible?

This is some github syntax that gives color in their logs, @Kixunil found it but I didn't look up the docs when he told me about it.

IIRC it's visible when you open the lock file from the PR. It was long time since I looked into it.

stevenroose

utACK e6dc6f48da7e79a3c2a188ad69ef8a9d8d799ad7

apoelstra · 2023-04-24T20:07:36Z

Would like an ack from @Kixunil as well since this is based on his work.

@stevenroose yeah, I think this would be generally useful. This PR does introduce CI support for this (in contrib/test.sh) though in a little bit of a hacky way. I think we should merge this and then see what kind of problems we run into before cleaning it up and maybe trying to do a more general thing. I am curious what kind of support you'd like for downstream crates....I guess, ideally your downstream projects could "merge" these lockfiles from rust-bitcoin into their own. I'm not sure if there's any tool that can do that.

apoelstra

Needs rebase after #1732 since the included lockfiles now need to include honggfuzz.

BTW in #1732 I set the wrong honggfuzz version. It is at 0.5 but it needs to be 0.5.55 (or possibly lower....but certainly 0.5.0 is too low).

Kixunil

Looks reasonable, will wait until rebase with proper review. Unfortunately despite having more time I find it harder to get myself into reviewing large PRs. (Yes Tobin, it could be WOE-related, still debugging.)

Kixunil · 2023-04-28T17:13:16Z

contrib/test.sh

IIRC it's visible when you open the lock file from the PR. It was long time since I looked into it.

tcharding · 2023-04-30T21:54:14Z

(Yes Tobin, it could be WOE-related, still debugging.)

What does WOE mean, Way Of Eating?

`core2` versions 0.3.0 and 0.3.1 do not work with Rust 1.48.0, set minimum version to 0.3.2 in the `bitcoin` manifest.

tcharding · 2023-05-01T01:52:28Z

I couldn't get a minimal honggfuzz dep because where ever I put --locked running ./fuzz.sh still updated the lock file. Does the Cargo-minimal.lock file have to commit to a minimal dependency version for honggfuzz?

Kixunil · 2023-05-01T10:45:07Z

Does the Cargo-minimal.lock file have to commit to a minimal dependency version for honggfuzz?

Probably, but I'm not sure if we need to test with minimal dev dependencies.

apoelstra · 2023-05-01T16:09:33Z

@tcharding I have had no problem with a minimal version of 0.5.55.

apoelstra · 2023-05-01T21:48:45Z

Note that #1821 actually changes fuzz/Cargo.toml to have the correct minimal honggfuzz reason. So I think it's fine that this PR merely includes it in the Cargo.lock.

tcharding · 2023-05-01T23:35:47Z

Note that #1821 actually changes fuzz/Cargo.toml to have the correct minimal honggfuzz reason. So I think it's fine that this PR merely includes it in the Cargo.lock.

Yep, sweet. I think you are right, we can merge these in any order.

Kixunil

Looks like there's a bug, the rest should be fine.

Kixunil · 2023-05-02T12:25:38Z

contrib/test.sh

$deps is not defined.

face palm, thanks man. I added DEPS="recent minimal", and put quotes around $dep everywhere as suggested by shellcheck.

It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could cause resolution issues for downstream users. It's also unclear for outsiders to see which dependencies we tested the crate with. This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal depdendency versions, while `recent` contains dependency versions at the time of making the change. Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.) The documentation is also updated accordingly. Co-developed-by: Tobin C. Harding <me@tobin.cc> Closes rust-bitcoin#1230

apoelstra

ACK c4c64c0

Kixunil

ACK c4c64c0

This replaces manual pins in the README and CI workflow. Close #337 based on rust-bitcoin/rust-bitcoin#1764

tcharding mentioned this pull request Mar 29, 2023

Test with minimal dependency versions #1272

Closed

tcharding force-pushed the 03-30-commit-lock-files branch 3 times, most recently from 561d424 to cff6c38 Compare March 30, 2023 22:11

tcharding mentioned this pull request Mar 31, 2023

Remove schemars pin from manifest #1696

Merged

tcharding marked this pull request as draft March 31, 2023 00:35

tcharding force-pushed the 03-30-commit-lock-files branch 2 times, most recently from d3399e0 to 5a6332d Compare April 19, 2023 23:46

tcharding marked this pull request as ready for review April 19, 2023 23:46

apoelstra previously approved these changes Apr 20, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via c129eb6 April 20, 2023 22:42

tcharding force-pushed the 03-30-commit-lock-files branch from 5a6332d to c129eb6 Compare April 20, 2023 22:42

tcharding mentioned this pull request Apr 20, 2023

Add hex crate #1573

Closed

tcharding added the P-high High priority label Apr 21, 2023

apoelstra approved these changes Apr 21, 2023

View reviewed changes

stevenroose reviewed Apr 24, 2023

View reviewed changes

stevenroose previously approved these changes Apr 24, 2023

View reviewed changes

apoelstra requested changes Apr 28, 2023

View reviewed changes

Kixunil reviewed Apr 28, 2023

View reviewed changes

Bump core2 dependency from 0.3.0 -> 0.3.2

d5655d5

`core2` versions 0.3.0 and 0.3.1 do not work with Rust 1.48.0, set minimum version to 0.3.2 in the `bitcoin` manifest.

tcharding dismissed stevenroose’s stale review via 485dcc5 May 1, 2023 01:51

tcharding force-pushed the 03-30-commit-lock-files branch from c129eb6 to 485dcc5 Compare May 1, 2023 01:51

tcharding requested review from Kixunil and apoelstra May 1, 2023 21:42

Kixunil requested changes May 2, 2023

View reviewed changes

tcharding force-pushed the 03-30-commit-lock-files branch from 485dcc5 to c4c64c0 Compare May 2, 2023 22:08

tcharding requested a review from Kixunil May 2, 2023 22:09

apoelstra approved these changes May 3, 2023

View reviewed changes

Kixunil approved these changes May 3, 2023

View reviewed changes

apoelstra merged commit 23d80bf into rust-bitcoin:master May 3, 2023

tcharding deleted the 03-30-commit-lock-files branch May 4, 2023 22:06

apoelstra mentioned this pull request Oct 26, 2023

Remove 1.46 from build file #2125

Closed

oleonardolima mentioned this pull request Apr 7, 2025

discussion: should we adopt the Cargo-minimal.lock and Cargo-recent.lock approach ? bitcoindevkit/bdk_wallet#79

Open

DanGould mentioned this pull request Sep 12, 2024

Add Cargo-{minimal,recent}.lock to test deps payjoin/rust-payjoin#357

Merged

DanGould added a commit to payjoin/rust-payjoin that referenced this pull request Sep 19, 2024

Add Cargo-{minimal,recent}.lock to test deps (#357)

6460c85

This replaces manual pins in the README and CI workflow. Close #337 based on rust-bitcoin/rust-bitcoin#1764

node-smithxby72w added a commit to node-smithxby72w/rust-payjoin that referenced this pull request Sep 28, 2025

Add Cargo-{minimal,recent}.lock to test deps (#357)

377f648

This replaces manual pins in the README and CI workflow. Close #337 based on rust-bitcoin/rust-bitcoin#1764

Conversation

tcharding commented Mar 29, 2023 • edited by apoelstra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 20, 2023

Uh oh!

tcharding commented Apr 20, 2023

Uh oh!

Kixunil commented Apr 20, 2023

Uh oh!

apoelstra commented Apr 20, 2023

Uh oh!

tcharding commented Apr 20, 2023

Uh oh!

tcharding commented Apr 21, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Apr 24, 2023

Uh oh!

stevenroose Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

Kixunil Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 24, 2023

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil Apr 28, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding commented Apr 30, 2023

Uh oh!

tcharding commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented May 1, 2023

Uh oh!

apoelstra commented May 1, 2023

Uh oh!

apoelstra commented May 1, 2023

Uh oh!

tcharding commented May 1, 2023

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil May 2, 2023

Choose a reason for hiding this comment

Uh oh!

tcharding May 2, 2023

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tcharding commented Mar 29, 2023 •

edited by apoelstra

Loading

tcharding commented May 1, 2023 •

edited

Loading