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

[tcharding]

Use the new maintainer tools test script we created in rust-bitcoin/rust-bitcoin-maintainer-tools#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:

• We do not format (exists in current ci but is not enforced because of bug is test.sh)
• We just build the examples (same as current behaviour)
• We do not lint (same as current behaviour)
• We do not pin, instead we commit a lock file with working dependency versions. Please note that there are two lock files as is customary but in this repo I was unable to get a set of more recent dependencies to build so both files are the same.

This replaces #338

[tcharding]

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

[apoelstra]

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?)

[storopoli]

small comment

[storopoli]

What happens once we loop an year?
This is not "collision resistant" 😂

[apoelstra]

We also need to change the repository from tcharding/rbmt to rust-bicoin/rbmt. I was imagining then that the ref would just be a commit ID. But if we want to use tags then you're right, we should make sure the year is part of the name :)

[tcharding]

My bad, I did comment on that somewhere. Using my repo and branch is just for testing.

This is not "collision resistant" 😂

I'm currently solving this issue with a "clobber branches if they are a year old policy", its even more confusing when I accidentally use the wrong date a few days in the future

[tcharding]

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]

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

[tcharding]

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]

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]

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]

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]

Yeah, this is probably a good idea.

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

[apoelstra]

ACK 93602a9

[apoelstra]

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.