CI: Use run_task from maintainer tools #682
Conversation
|
Lint warnings are real, they are in the previously un-linted |
|
No love ( |
|
You mean in |
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
6 times, most recently
from
19097e2
to
779f1ee
Compare
|
Includes |
Yes, but stale documentation always reveals more than non-existent. |
|
Fair point. |
72e42b3 Add CI shell scripts (Tobin C. Harding) Pull request description: We would like to put all the CI scripts in a single place instead of copied to each repository. Add a `ci/` directory and in it a `run_task.sh` script as well as auxilary scripts required. Include a README to document the directory. When the following three PRs have green CI runs then I believe we can merge this. And then I will update each of the PRs to use this repo and `master` (instead my fork and the PR branch `05-02-ci`). - [x] `rust-bitcoin`: rust-bitcoin/rust-bitcoin#2736 - [x] `rust-miniscript`: rust-bitcoin/rust-miniscript#682 - [x] `rust-bitcoincore-rpc`: rust-bitcoin/rust-bitcoincore-rpc#348 - [x] `rust-chf` https://github.com/tcharding/rust-chf/actions/runs/8931701405 All green with one minute till my End Of Day - BOOM! ACKs for top commit: apoelstra: utACK 72e42b3 Tree-SHA512: 199203ff283cce6f05ac69c971aaf563b1df9574900cf24ef0d979dee34419d6d3ba01de930e08318878e49ce0dd5e30e81c5eb4603b8404acce909eca03a6a6
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
779f1ee
to
a9646ce
Compare
|
This will need to be rebased on #685 |
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
a9646ce
to
befd49c
Compare
|
Ready to go! |
| @@ -86,8 +86,8 @@ fn setup_keys( | |||
| let mut x_only_keypairs = vec![]; | |||
| let mut x_only_pks = vec![]; | |||
|
|
|||
| for i in 0..n { | |||
| let keypair = bitcoin::secp256k1::Keypair::from_secret_key(&secp_sign, &sks[i]); | |||
| for sk in sks.iter().take(n) { | |||
There was a problem hiding this comment.
In 3ad8c0e:
Pretty sure take is unnecessary here. You can just do for sk in &sks.
|
Other than the single nit befd49c looks great! Can you please split this into two PRs -- doing the clippy stuff as one and the CI stuff as another (starting with the "add lockfiles" commit) would be a more natural split. Plus it would be really annoying/difficult for me to locally test a PR where the lockfile location were to change partway through. Finally, what happens to the existing contrib/test.sh? Should there be another commit which deletes it? |
|
Thanks man, I'll sort it out! |
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
befd49c
to
dfcd989
Compare
b9a207b Use is_err instead of redundant pattern matching (Tobin C. Harding) ef868cc Use unwrap_or_else instead of expect (Tobin C. Harding) a1a5467 Remove clone call from Copy type (Tobin C. Harding) f939367 Remove useless conversion (Tobin C. Harding) c20be39 Remove explicit reference (Tobin C. Harding) 7ed6680 Do not mutate local variable (Tobin C. Harding) e361cfc Remove unneeded return statement (Tobin C. Harding) f9907b0 Use += operator (Tobin C. Harding) 5e0df21 Use single quotes (Tobin C. Harding) 7218e5e Remove redundant field names (Tobin C. Harding) 6a2577f Remove useless let binding (Tobin C. Harding) ccc808a Use iterator instead of array access (Tobin C. Harding) badeb44 Remove unnecessary cast (Tobin C. Harding) Pull request description: All the clippy patches rom #682, only touches `bitcoind_test`. ACKs for top commit: apoelstra: utACK b9a207b Tree-SHA512: ae9451601bc5232f0ce194be1b7c6c1c31c8118ea87951f3eac9c6c4349c7ff963516c8cc34c2a63c9f1b99f03c781e687c74b46dc24624bbfa81a0decb513ef
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
dfcd989
to
3eff47c
Compare
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
2 times, most recently
from
836f559
to
2e37b34
Compare
Just so you know this sort of thing is no longer annoying or difficult for me. My CI script is now smart enough to look for lockfiles in several locations within the source tree, and to fallback to externally provided lockfiles if none are present. |
|
Sweet, nice one. |
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
3 times, most recently
from
b95b503
to
46d59b7
Compare
|
In 46d59b7 can you fold the lockfile changes into the first commit so that the lockfiles are correct as soon as they appear? As for "the lockfile-updating script should appear in maintainer-tools", I'm tempted to move to a model where we copy the files out of rust-bitcoin-maintainer-tools, along with a file indicating their git commit of origin, and in CI we check that the files are exact copies. Then people have all the scripts available locally without needing to go to the other repo, but we still guarantee everything is in sync. But that doesn't need to be part of this PR. |
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
46d59b7
to
3564e9a
Compare
Do as clippy says, remove the borrow.
As we do in `rust-bitcoin` add two lock files, one with minimal dependency version numbers and one with recent dependency version numbers The minimal was created using `-Z minimal-versions` then tested by building with `RUSTUP_TOOLCHAIN=1.56.1 cargo --locked check`. The recent was created by the `contrib/test.sh` script with MSRV toolchain. Add script to update the lock files and a `just` command to run it.
Done in preparation for using the new maintainer tools script. Pull the bitoind test out into a separate script because it is specific to this repo. No change in test coverage.
We have a CI script in the `rust-bitcoin-maintainer-tools` repository, lets use it.
tcharding
force-pushed
the
05-02-ci-maintainer-tools
branch
from
3564e9a
to
6fffb8b
Compare
|
Put all the lock file stuff in a single patch and put the clippy fix up front. No other changes. |
This PR slightly improves test coverage by linting the integration tests, no meaningful change of coverage really though just improvement to the CI setup.