Changing into the build dir is confusing and brittle, because the following commands implicitly assume it. So they could break on unrelated changes.
The changes are required for stuff like:
So remove the cd and just make the build dir explicit.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32880.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto |
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.
126
127-bash -c "cmake -S $BASE_ROOT_DIR $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( (cat $(cmake -P "${BASE_ROOT_DIR}/ci/test/GetCMakeLogFiles.cmake")) && false)"
128+bash -c "cmake -S $BASE_ROOT_DIR -B ${BASE_BUILD_DIR} $BITCOIN_CONFIG_ALL $BITCOIN_CONFIG || ( (cat $(cmake -P "${BASE_ROOT_DIR}/ci/test/GetCMakeLogFiles.cmake")) && false)"
129
130-bash -c "cmake --build . $MAKEJOBS --target all $GOAL" || ( echo "Build failure. Verbose build follows." && cmake --build . --target all "$GOAL" --verbose ; false )
131+bash -c "cmake --build ${BASE_BUILD_DIR} $MAKEJOBS --target all $GOAL" || ( echo "Build failure. Verbose build follows." && cmake --build . --target all "$GOAL" --verbose ; false )
0bash -c "cmake --build ${BASE_BUILD_DIR} $MAKEJOBS --target all $GOAL" || ( echo "Build failure. Verbose build follows." && cmake --build "${BASE_BUILD_DIR}" --target all "$GOAL" --verbose ; false )
bash -c, because if it was needed, the fallback code would be broken
yeah, the fallback was missing -j1 (required after ninja) and had a wordsplitting bug, due to the use of Bash :(
Fixed those as well.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/45728542136
LLM reason (✨ experimental): The CI failure is caused by errors in the linting step, specifically from shellcheck and spelling checks.
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.
Changing into the build dir is confusing and brittle.
This can be reviewed using the git option `--word-diff-regex=.`.
Also:
* add missing -j1 to the fallback that prints a verbose build failure
* remove quotes around $GOAL in the fallback