This was requested in #32573 (comment), as it is currently not possible to catch this type of error in a Bash linter (https://github.com/bitcoin/bitcoin/pull/32573#pullrequestreview-2857077463). Also, reviewers are not familiar with it and didn’t catch the above bug either. (Thus, it is discouraged in the dev notes: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripts)
So just use a simple python script.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32680.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/43523418048
LLM reason (✨ experimental): Lint test failed due to permission issue on a script file.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
0@@ -0,0 +1,104 @@
1+// Copyright (c) The Bitcoin Core developers
0---cargo
1[package]
2edition = "2024"
3---
4// Copyright (c) The Bitcoin Core developers
nit / idea, add embedded manifest for direct cargo execution
80@@ -81,7 +81,7 @@ jobs:
81 - name: Compile and run tests
82 run: |
83 # Run tests on commits after the last merge commit and before the PR head commit
84- git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && ./.github/ci-test-each-commit-exec.sh && git reset --hard" ${{ env.TEST_BASE }}
85+ git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && rustc -D warnings --edition=2024 ./.github/ci-test-each-commit-exec.rs -o ./a && ./a && git reset --hard" ${{ env.TEST_BASE }}
0 git rebase --exec "git merge --no-commit origin/${GITHUB_BASE_REF} && cargo -Zscript ./.github/ci-test-each-commit-exec.rs && git reset --hard" ${{ env.TEST_BASE }}
This removes the extra execution step if cargo is used. Do note that this is stil in “unstable features” .
Concept ACK
What do you think about the idea to use cargo for this (maybe not now because it’s still in ‘unstable) ?
Hmm, yeah the boilerplate overhead is a bit unfortunate. I switched to python, which still has downsides such as #29068 (comment), but it seems manageable here. Also, updated the pull description.
I think the danger of Bash, that it looks harmless and superficially simple and correct, but in many such “harmless” constructs can silently drop a failing return status is reason enough to prefer a slightly more verbose language. Maybe rust is too verbose here, but python seems acceptable. Bash also has many other issues, for example the version shipped by macOS is so ancient that it is older than Bitcoin itself and writing a CI script on a modern platform that uses a newer Bash feature will then only fail in CI when the pull is created/pushed. (Failing loudly if you are lucky, failing silently, if you are not)
This solves an URL redirect. (The commit is also used to trigger the test-each-commit CI)
After some contemplating I think the Python code is the better way to go. The Rust code was written a bit verbose and could have been made smaller, but Python seems like the more suitable tool for this task because:
And Python still gives us the advantages over Bash!
ACK https://github.com/bitcoin/bitcoin/commit/fa9cfdf3be754b01e6ce73a0cc2f998b81e12970
code reviewed ✅ build & tested ✅ ran the python script separately ✅
7+import sys
8+import shlex
9+
10+
11+def run(cmd, **kwargs):
12+ print("+ " + shlex.join(cmd), flush=True)
