Use rust-bitcoin-maintainer-tools and re-write CI #119

tcharding · 2024-05-17T03:50:35Z

As we have been doing in other crates, use the new maintainer tools test script we created from rust-bitcoin/rust-bitcoin-maintainer-tools/ci.

This should not change test coverage in any meaningful way.
The integration tests are not currently run in CI, this patch preserves that behaviour.
Introduces recent/minimal lock files.

apoelstra · 2024-05-17T14:56:43Z

Looks like the serde_json version is too low and some other crates' versions are too high, in the lockfiles.

We also probably want to pin nightly rustc in the same PR.

tcharding · 2024-05-31T02:26:27Z

Note please the recent and minimal lock files are the same. I spent 10 minutes trying to fix them otherwise, but my fingers started to bleed.

Also, I'll fix the lint job after #117 goes in.

tcharding · 2024-06-14T02:27:41Z

I don't see whats up with the lint job, I can't reproduce locally? I tried, with just, with run_task, and also using the cargo command directly.

apoelstra · 2024-06-15T13:03:22Z

@tcharding it looks like your lockfiles have serde 1.0.101, which does not compile anymore.

It looks like serde 1.0.103 will work, which is what we have in the Cargo.toml for bitcoin and units and internals (hashes has 1.0 which is a bug, oops). I remember the minimal version needing to be even higher...our Cargo-minimal.lock in the rust-bitcoin repo is at 1.0.156, which is suspicious because the minimal lockfile should match Cargo.toml.

Anyway, to get this PR compiling we just need to bump the serde version. But separately I should go audit the lockfiles in rust-secp and rust-bitcoin.

tcharding · 2024-06-17T03:00:08Z

Since this crate is MSRV 1.48 still, I wasn't able to use the other repos lock files for guidance. Simply using cargo update -p serde --precise 1.0.103 doesn't seem to work here. I'm on an epic fast to try get over all the illnesses I keep getting clobbered with at the moment so I'll come back to this when I'm eating again.

tcharding · 2024-06-17T03:12:25Z

Oh the MSRV thing is going to be a problem if we want to move this to bitcoind-json-rpc repo, any reason not to bump this crate to 1.56.1?

apoelstra · 2024-06-17T14:06:12Z

Nope, no reason. Let's do it.

tcharding · 2024-06-20T23:02:34Z

Now includes the MSRV bump, are you ok to do that in this PR or is that too much going on?

tcharding · 2024-06-20T23:17:19Z

Not sure whats up with the lint fail.

apoelstra · 2024-06-21T15:12:18Z

The lint failure looks like an outdated version of serde is being used.

tcharding · 2024-06-21T22:40:28Z

oh thanks, nice. I'll try to fix that up.

apoelstra · 2024-06-26T13:33:28Z

Still having trouble with your lockfiles. I will try to figure it out and submit a diff.

tcharding · 2024-08-06T23:10:47Z

Rebased to pick up the MSRV bump. Also added a patch upgrading honggfuzz.

apoelstra · 2024-08-07T12:48:34Z

contrib/crates.sh

+#!/usr/bin/env bash
+
+# Crates in this workspace to test (excl. fuzz an integration-tests).
+CRATES=(".")                    # Non-workspaces don't have crates.


In a065c9f:

What is going on with this file? What does "non-workspaces don't have crates" mean, and does this actually exclude testing the fuzz and integration-test crates, and why?

Oh yes you are correct this should include fuzz, and I have that wrong in miniscript as well. The comment is also wrong and was always wrong, I wrote "Non-workspcase" when I meant "repos that have crate root in the same directory as repo root" (I don't know the name for that). We need to keep the "." so it should be

CRATES=("." "fuzz") # Some comment about the dot directory.

I think you want then

# Directories of workspaces to test CRATES=("." "fuzz")

Saying "crates in this workspace" is just confusing. Workspaces are crates.

BTW the reason I'm complaining about this is that the existing lockfiles don't work with MSRV, which you can tell if you try to compile the integration_test workspace.

I think I also forgot that this crate is 1.48 still, at least that was not in my mind yesterday.

~~oh there's the problem, I bumped the MSRV in this PR - I'll pull it out into a separate one.~~ We bumped to 1.63 last week.

Alright, I'm well confused. I copied Cargo-recent.lock to Cargo.lock and cd'ed into integration_test and ran

cargo +1.63.0 --locked check

And it was ok.

I just went right ahead and added fuzz and integration_test to CRATES also.

FTR, note that we do not run the integration_test/run.sh script now or previously.

I think I also forgot that this crate is 1.48 still

What the hell was I thinking

In preparation for using a pinned nightly in CI add a `nightly-version` file and also a `justfile` that uses it.

Use the newly release patch version of `honggfuzz`.

Clippy emits: error: initializer for `thread_local` value can be made `const` As suggested make the call const.

apoelstra · 2024-08-08T23:07:56Z

If you run cargo +1.63.0 test --all-features --locked in the integration test directory, none of object backtrace addr2line (at least) compile. In 04dc4b2.

tcharding · 2024-08-08T23:58:01Z

hmm, something is not quite right then because that should have been caught by CI. I'll investigate further.

tcharding · 2024-08-09T00:19:43Z

Found the bug in run_task, we have a break instead of a continue in a loop. Funny after we just hit that same mistake in that bech32 PR the other day. We were not hitting it before because fuzz was always at the end of the list of CRATES.

rust-bitcoin/rust-bitcoin-maintainer-tools#10

As we have been doing in other crates, use the new maintainer tools test script we created from `rust-bitcoin/rust-bitcoin-maintainer-tools/ci`. - This should not change test coverage in any meaningful way. - The integration tests are not currently run in CI, this patch preserves that behaviour. - Introduces recent/minimal lock files.

apoelstra · 2024-08-09T13:04:26Z

Oh, neat. I feel much better about what a PITA this PR has been. Apparently we needed to use the maintainer tools on a crate whose CI was a total mess :).

tcharding · 2024-08-10T19:50:49Z

Ok, this one should be right to merge if it gets past your local CI.

apoelstra

ACK 8f17794 successfully ran local tests

Import the `rust-jsonrpc` crate from https://github.com/apoelstra/rust-jsonrpc using current tip of master `59646e6 Merge apoelstra/rust-jsonrpc#119: Use rust-bitcoin-maintainer-tools and re-write CI` Full commit hash: 59646e6e6ac95f07998133b1709e4a1fa2dbc7bd Do so using the following commands: mkdir jsonrpc mkdir jsonrpc/contrib rsync -avz ../../rust-jsonrpc/master/README.md jsonrpc rsync -avz ../../rust-jsonrpc/master/src jsonrpc rsync -avz ../../rust-jsonrpc/master/contrib/test_vars.sh jsonrpc/contrib Then: - Update `contrib/crates.sh` to include `jsonrpc`. - Remove workspaces from `jsonrpc/Cargo.toml`. - Add `jsonrpc` to repository workspace.

Import the `rust-jsonrpc` crate from https://github.com/apoelstra/rust-jsonrpc using current tip of master `59646e6 Merge apoelstra/rust-jsonrpc#119: Use rust-bitcoin-maintainer-tools and re-write CI` Full commit hash: 59646e6e6ac95f07998133b1709e4a1fa2dbc7bd Do so using the following commands: mkdir jsonrpc mkdir jsonrpc/contrib rsync -avz ../../rust-jsonrpc/master/README.md jsonrpc rsync -avz ../../rust-jsonrpc/master/src jsonrpc rsync -avz ../../rust-jsonrpc/master/contrib/test_vars.sh jsonrpc/contrib Then: - Update `contrib/crates.sh` to include `jsonrpc`. - Remove workspaces from `jsonrpc/Cargo.toml`. - Add `jsonrpc` to repository workspace (and add patch section). Note, this PR does not bring over the `fuzz` directory, that will be done separately. Also we do not copy the integration testing because we get sufficient coverage from the current integration tests.

Import the fuzzing from `jsonrpc` source: https://github.com/apoelstra/rust-jsonrpc commit hash: `59646e6e6ac95f07998133b1709e4a1fa2dbc7bd` commit: `59646e6 Merge apoelstra/rust-jsonrpc#119: Use rust-bitcoin-maintainer-tools and re-write CI` Then I update the `generate-files.sh` script to mimic a recent one from `rust-bitcoin`. Add a README explaining how we got to the current state.

7238857 Add fuzzing for jsonrpc (Tobin C. Harding) 49b72c1 Import jsonrpc crate (Tobin C. Harding) 45addbd justfile: Add a docs build command (Tobin C. Harding) Pull request description: Import the `rust-jsonrpc` crate from https://github.com/apoelstra/rust-jsonrpc using current tip of master `59646e6 Merge apoelstra/rust-jsonrpc#119: Use rust-bitcoin-maintainer-tools and re-write CI` Full commit hash: 59646e6e6ac95f07998133b1709e4a1fa2dbc7bd Do so using the following commands: mkdir jsonrpc mkdir jsonrpc/contrib rsync -avz ../../rust-jsonrpc/master/README.md jsonrpc rsync -avz ../../rust-jsonrpc/master/src jsonrpc rsync -avz ../../rust-jsonrpc/master/contrib/test_vars.sh jsonrpc/contrib Then: - Update `contrib/crates.sh` to include `jsonrpc`. - Remove workspaces from `jsonrpc/Cargo.toml`. - Add `jsonrpc` to repository workspace. Finally import fuzzing, and fix up to mimic current `rust-bitcoin` setup. ACKs for top commit: apoelstra: ACK 7238857 successfully ran local tests Tree-SHA512: 309c214d50e78bcd67b49b8f68df91792100d95fda65a6ec700c422d28d64f41498e52f4eabeebcfae46685ee997ffb8c444180863d8f8003ed542a5ed80d174

tcharding mentioned this pull request May 17, 2024

Allow assigning_clones #118

Closed

tcharding force-pushed the 05-17-ci-maintainer-tools branch from bf1f843 to bb77e42 Compare May 31, 2024 01:54

tcharding marked this pull request as draft May 31, 2024 01:54

tcharding force-pushed the 05-17-ci-maintainer-tools branch 2 times, most recently from 78f6004 to 0513663 Compare May 31, 2024 02:01

tcharding mentioned this pull request May 31, 2024

Add --cfg fuzzing build to CI #83

Open

tcharding force-pushed the 05-17-ci-maintainer-tools branch from 0513663 to 6ffa4a3 Compare May 31, 2024 02:23

tcharding mentioned this pull request May 31, 2024

Move to rust-bitcoin tracking issue #78

Open

tcharding force-pushed the 05-17-ci-maintainer-tools branch 3 times, most recently from 61dd7b8 to ebeac68 Compare June 14, 2024 02:16

tcharding marked this pull request as ready for review June 14, 2024 02:27

tcharding force-pushed the 05-17-ci-maintainer-tools branch 2 times, most recently from a98253a to 5cf4293 Compare June 20, 2024 23:01

tcharding force-pushed the 05-17-ci-maintainer-tools branch from 5cf4293 to dd4d0d3 Compare June 20, 2024 23:03

tcharding changed the title ~~Use rust-bitcoin-maintainer-tools and re-write CI~~ Use rust-bitcoin-maintainer-tools and re-write CI (bump MSRV) Jun 20, 2024

tcharding force-pushed the 05-17-ci-maintainer-tools branch 2 times, most recently from 0035623 to 10972a7 Compare June 24, 2024 02:43

tcharding force-pushed the 05-17-ci-maintainer-tools branch 2 times, most recently from b234fcd to af66b21 Compare August 6, 2024 23:10

tcharding marked this pull request as ready for review August 6, 2024 23:10

tcharding force-pushed the 05-17-ci-maintainer-tools branch from af66b21 to a065c9f Compare August 6, 2024 23:14

apoelstra reviewed Aug 7, 2024

View reviewed changes

tcharding force-pushed the 05-17-ci-maintainer-tools branch 3 times, most recently from d4ed555 to 6ece4f0 Compare August 8, 2024 20:47

tcharding added 3 commits August 9, 2024 07:06

Add justfile and nightly config

f749975

In preparation for using a pinned nightly in CI add a `nightly-version` file and also a `justfile` that uses it.

Upgrade honggfuzz

fd2f7e8

Use the newly release patch version of `honggfuzz`.

Make thread local value const

9a84a10

Clippy emits: error: initializer for `thread_local` value can be made `const` As suggested make the call const.

tcharding force-pushed the 05-17-ci-maintainer-tools branch 2 times, most recently from df170ac to 04dc4b2 Compare August 8, 2024 21:18

tcharding force-pushed the 05-17-ci-maintainer-tools branch from 04dc4b2 to 8f17794 Compare August 9, 2024 00:26

apoelstra approved these changes Aug 11, 2024

View reviewed changes

apoelstra merged commit 59646e6 into apoelstra:master Aug 11, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rust-bitcoin-maintainer-tools and re-write CI #119

Use rust-bitcoin-maintainer-tools and re-write CI #119

tcharding commented May 17, 2024 •

edited

Loading

apoelstra commented May 17, 2024

tcharding commented May 31, 2024

tcharding commented Jun 14, 2024 •

edited

Loading

apoelstra commented Jun 15, 2024

tcharding commented Jun 17, 2024

tcharding commented Jun 17, 2024

apoelstra commented Jun 17, 2024

tcharding commented Jun 20, 2024

tcharding commented Jun 20, 2024

apoelstra commented Jun 21, 2024

tcharding commented Jun 21, 2024

apoelstra commented Jun 26, 2024

tcharding commented Aug 6, 2024

apoelstra Aug 7, 2024

tcharding Aug 7, 2024

apoelstra Aug 7, 2024 •

edited

Loading

apoelstra Aug 7, 2024

tcharding Aug 7, 2024

tcharding Aug 8, 2024 •

edited

Loading

tcharding Aug 8, 2024

tcharding Aug 8, 2024 •

edited

Loading

tcharding Aug 8, 2024

apoelstra commented Aug 8, 2024

tcharding commented Aug 8, 2024

tcharding commented Aug 9, 2024 •

edited

Loading

apoelstra commented Aug 9, 2024

tcharding commented Aug 10, 2024

apoelstra left a comment

Use rust-bitcoin-maintainer-tools and re-write CI #119

Use rust-bitcoin-maintainer-tools and re-write CI #119

Conversation

tcharding commented May 17, 2024 • edited Loading

apoelstra commented May 17, 2024

tcharding commented May 31, 2024

tcharding commented Jun 14, 2024 • edited Loading

apoelstra commented Jun 15, 2024

tcharding commented Jun 17, 2024

tcharding commented Jun 17, 2024

apoelstra commented Jun 17, 2024

tcharding commented Jun 20, 2024

tcharding commented Jun 20, 2024

apoelstra commented Jun 21, 2024

tcharding commented Jun 21, 2024

apoelstra commented Jun 26, 2024

tcharding commented Aug 6, 2024

apoelstra Aug 7, 2024

Choose a reason for hiding this comment

tcharding Aug 7, 2024

Choose a reason for hiding this comment

apoelstra Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

apoelstra Aug 7, 2024

Choose a reason for hiding this comment

tcharding Aug 7, 2024

Choose a reason for hiding this comment

tcharding Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

tcharding Aug 8, 2024

Choose a reason for hiding this comment

tcharding Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

tcharding Aug 8, 2024

Choose a reason for hiding this comment

apoelstra commented Aug 8, 2024

tcharding commented Aug 8, 2024

tcharding commented Aug 9, 2024 • edited Loading

apoelstra commented Aug 9, 2024

tcharding commented Aug 10, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented May 17, 2024 •

edited

Loading

tcharding commented Jun 14, 2024 •

edited

Loading

apoelstra Aug 7, 2024 •

edited

Loading

tcharding Aug 8, 2024 •

edited

Loading

tcharding Aug 8, 2024 •

edited

Loading

tcharding commented Aug 9, 2024 •

edited

Loading