The remaining ci/test/02_run_container.sh is fine, but has a bunch of shellcheck SC2086 word splitting violations.
This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.
However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33732.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | frankomosh, m3dwards |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
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.
189+ "--human-readable",
190+ f"{os.environ['BASE_READ_ONLY_DIR']}/",
191+ f"{os.environ['BASE_ROOT_DIR']}",
192+ ]
193+ if ci_exec(rsync_cmd, check=False).returncode != 0:
194+ print(f"Nothing to copy from {os.environ['BASE_READ_ONLY_DIR']}")
This is just moving the code one-to-one, but possibly rsync, along with the rsync command passing, could be required here. (IIRC the error message may have been better worded to reflect the intention: rsync not found, no need to copy from ...)
At least on macOS, it seems to pass: https://github.com/bitcoin/bitcoin/actions/runs/18913390006/job/53990076461?pr=33732#step:7:134
0+ rsync --recursive --perms --stats --human-readable /Users/runner/work/bitcoin/bitcoin/ /Users/runner/work/bitcoin/bitcoin
1Number of files: 3105
2Number of files transferred: 0
3...
The container_id is already known in the Python script, as well as the
folders to create, so just do it there.
The `retry` script is required for CI_RETRY_EXE and there are two ways
to put it into PATH:
* When running in a container engine, by copying it into /usr/bin
* When running without a container engine, by prepending its location to PATH
This move-only refactor clarifies that macos assumes and requires
DANGER_RUN_CI_ON_HOST.
So move the snippet under the condition for self-documenting code.
Can be reviewed with the git options:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
It contains a large `bash -c` string, which is hard to parse. So pull
out components:
* CI_EXEC is only called with absolute folders as args, so the `cd` is
not needed in CI_EXEC. It is only needed to specify the working dir of
running the tests in 03_test_script.sh, so move it there.
* The PATH modification is only needed after commit
4756114e505cff8848fb6344ef9a48d8822066c1 to check that depends does
work properly, even when the PATH contains a space.
* This allows to also drop the `bash -c` and use the proper and safer
"$@" to forward args without the risk of word splitting.
In theory one could run the CI without the rsync package installed, and
with DANGER_RUN_CI_ON_HOST=1. However, this seems to be an edge case.
Simply requiring rsync to be installed is less code and avoids brittle
edge cases around rsync failures.
The Bash script was acceptable, but CI_EXEC_CMD_PREFIX was a single
string, relying on brittle word splitting that the shellcheck SC2086
would warn about.
So just fix that by moving everything to the Python script and deleting
the Bash script.
This also removes the need to export the CI_CONTAINER_ID env var.
174- os.environ["IN_GETOPT_BIN"] = f"{prefix}/bin/getopt"
175+ def ci_exec(cmd_inner, **kwargs):
176+ if os.getenv("DANGER_RUN_CI_ON_HOST"):
177+ prefix = []
178+ else:
179+ prefix = ["docker", "exec", container_id]
container_id = None before the if/else? …. Or am I missing something?
container_id = run(... above.
DANGER_RUN_CI_ON_HOST=1) where the else block is skipped and container_id would still be referenced later, a case that would need an initial None, otherwise an error.
179+ prefix = ["docker", "exec", container_id]
180+
181+ return run([*prefix, *cmd_inner], **kwargs)
182+
183+ # Normalize all folders to BASE_ROOT_DIR
184+ ci_exec([
try/finally to guarantee cleanup? ..am trying to imagine if ci_exec() fails, and the container is never killed.
ACK fa336053aada79d13cd771ce025857256814465e
Tested on my own fork on GHA Tested locally on Debian both with DANGER_RUN_CI_ON_HOST set and unset Tested locally on MacOS
