search

rust-bitcoin / rust-bitcoincore-rpc

Rust RPC client library for the Bitcoin Core JSON-RPC API.
313 stars 231 forks source link

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

Closed tcharding closed 1 month ago

tcharding commented 2 months ago

Use the new maintainer tools test script we created in https://github.com/rust-bitcoin/rust-bitcoin-maintainer-tools/pull/4

For this repo usage of the script is a big change, the coverage does not change that much except we run one example instead of just building it and we run cargo using cargo --locked whereas currently for stable and nightly the dependencies used are much more recent.

FTR:

This replaces #338

tcharding commented 2 months ago

I'm not sure ATM why the script doesn't pin properly, the pin.sh script seems to work locally.

apoelstra commented 2 months ago

I'm confused about the relationship between the lockfiles and pin.sh. Why have both? pin.sh is guaranteed to go out of date as non-pinned deps break CI.

Also lol there is no way you can make cargo +nightly fmt a requirement on a crate that Steven literally maintains. (It doesn't look like you have done this ... so then why stick a formatting commit into this?)

tcharding commented 2 months ago

Also lol there is no way you can make cargo +nightly fmt a requirement on a crate that Steven literally maintains.

Yes I woke up yesterday morning realising I had been a massive dick and done that (use nightly), current state does not.

(It doesn't look like you have done this ... so then why stick a formatting commit into this?)

I believe formatting was intended to be enforced in CI, the current test.sh script looks like that anyway but is buggy, so I ran the formatter (with stable) and "kept" that behaviour.

tcharding commented 2 months ago

Please note, the solution I found to pinning issues is to just only run MSRV job using the minimal lock file.

tcharding commented 2 months ago

EDIT: (original message below) After sleeping on this I think a better approach is to just not have a formatting job and note the bug in the current test.sh script in this PR for the record. We could then, if desired, add that job as a separate PR with an explicit requirement for review from @stevenroose.

Hey @stevenroose

I believe I am acting correctly here but can you please ack because we have had prior history re formatting.

rustfmt appears to be meant to be enforced in CI at the moment but it is not because the cargo fmt -- --check result is not being handled correctly.

I have therefore enforced formatting in this PR using the stable toolchain as it currently does. However this required a formatting patch because there are formatting issues currently on master.

No probs if you want the format patch and CI job removed - just holla at me.

Thanks

apoelstra commented 2 months ago

Please note, the solution I found to pinning issues is to just only run MSRV job using the minimal lock file.

The recent lockfile should also work with the MSRV. If it doesn't then we need to back off on the deps in the recent lockfile. It's supposed to represent the most recent dependencies that work.

tcharding commented 1 month ago

It's supposed to represent the most recent dependencies that work.

I never realised it was supposed to represent the most recent dependencies that work with MSRV. Was that always what you had in mind? Since most users will not be using MSRV does that not mean the file has way less relevance? Also does that not mean our test coverage is worse because we may miss issues with current versions that most people are then going to hit?

If we want a recent for MSRV I rekon we should have a third lock file and have a recent for stable as well.

apoelstra commented 1 month ago

I never realised it was supposed to represent the most recent dependencies that work with MSRV. Was that always what you had in mind?

Yes.

Since most users will not be using MSRV does that not mean the file has way less relevance?

Maybe. If "most users" want to use dependencies I've never tested (or rather, I have tested and confirmed are broken) and don't care about, that is their perogative. But the point of these lockfiles is to provide specific versions that we've tested to work.

If we want a recent for MSRV I rekon we should have a third lock file and have a recent for stable as well.

Yeah, this is probably a good idea.

tcharding commented 1 month ago

Yeah, this is probably a good idea.

Just checking, this can be done separate to this PR, right?

apoelstra commented 1 month ago

I think this is OK to merge without Steven. It's only CI and the existing stuff is broken and it doesn't introduce any new dependencies or requirements for nightly tooling or anything.