Fixes #30496
Seems odd to sometimes do an out-of-tree build (via ./ci/lint_imagefile, see test/lint/README.md) and sometimes not (via Cirrus CI, see ./ci/lint_run_all.sh).
Fix it by doing an out-of-tree build consistently in the same location.
Also, fix $CI_RETRY_EXE, while touching this.
Previous code was confusing and brittle. For example, the full import
"source ./ci/test/00_setup_env.sh" and $PATH overwrite was not needed.
Fix it by simply copying the exe to /ci_retry and use that in
$CI_RETRY_EXE.
This is also a fix, because previously ci/lint_imagefile did use an
empty $CI_RETRY_EXE.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | paplorinc, josibake, willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27745036545
Make sure to run all tests locally, according to the documentation.
The failure may 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.
This mirrors the build by ./ci/lint_imagefile, which is done out-of-tree
in "/".
Otherwise, there could be errors due to a dirty tree.
lint is green now, sorry for the force pushes, but this can now be reviewed and tested.
6 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
7
8 export LC_ALL=C
9
10-export PATH=$PWD/ci/retry:$PATH
11+export CI_RETRY_EXE="/ci_retry --"
CI_RETRY_EXE still make sense in every case?
If it does, would it make sense to use it for e.g. ${CI_RETRY_EXE} curl (like in https://github.com/bitcoin/bitcoin/blob/master/ci/test/01_base_install.sh#L88) or ${CI_RETRY_EXE} cargo build commands as well?
ci/test
ci/lint is completely separate from ci/test (well after the first commit), so there is no interaction.
63@@ -62,3 +64,5 @@ MLC_VERSION=v0.18.0
64 MLC_BIN=mlc-x86_64-linux
65 curl -sL "https://github.com/becheran/mlc/releases/download/${MLC_VERSION}/${MLC_BIN}" -o "/usr/bin/mlc"
66 chmod +x /usr/bin/mlc
67+
68+popd || exit
popd fail if pushd succeeded?
I don’t known. I had to change this due to a lint error:
0In ci/lint/04_install.sh line 68:
1popd
2^--^ SC2164 (warning): Use 'popd ... || exit' or 'popd ... || return' in case popd fails.
3Did you mean:
4popd || exit
5For more information:
6 https://www.shellcheck.net/wiki/SC2164 -- Use 'popd ... || exit' or 'popd ....
7^---- ⚠️ Failure generated from lint-shell.py
63@@ -62,3 +64,5 @@ MLC_VERSION=v0.18.0
64 MLC_BIN=mlc-x86_64-linux
65 curl -sL "https://github.com/becheran/mlc/releases/download/${MLC_VERSION}/${MLC_BIN}" -o "/usr/bin/mlc"
git clone have --depth=1 throughout this file?
utACK https://github.com/bitcoin/bitcoin/commit/fa8d73e86e1c11cdfe8154ab84edc1948283454b
ran DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter, more as a sanity check since this isn’t really testing the underlying issue this PR is fixing.
utACK fa8d73e86e1c11cdfe8154ab84edc1948283454b
Thanks for addressing @maflcko, this makes things much more sanitary.
That said, our mlc lint is supposed to exclude all files/dirs which are not tracked by git, by calling git ls-files --ignored --others --exclude-standard.
It appears that the --ignored flag causes some directories/files to not appear in this listing, leading to the failure:
0will@ubuntu in ~/src/bitcoin on 2407-lint-fixes [$?] : 🐍 (bitcoin)
1$ git ls-files --ignored --others --exclude-standard | rg pyenv
2
3will@ubuntu in ~/src/bitcoin on 2407-lint-fixes [$?] : 🐍 (bitcoin)
4✗ git ls-files --others --exclude-standard | rg pyenv
5.pyenv/readme.md
I’ll try an upstream fix so that we can avoid a future recurrence (with another file/dir)
That said, our
mlclint is supposed to exclude all files/dirs which are not tracked by git, by callinggit ls-files --ignored --others --exclude-standard.
Yes, I know, but someone will need to create a pull request to pull in the mlc version that fixes this, if such a version exists.
I think the consistency fixes here make sense either way. Fixing the bug is just a convenient side effect.
