Fixes: #30028
AFAIK mounting the git root should not present any additional security issues, as this would already be mounted (in RO mode) when not using a worktree.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31787.
See the guideline for information on the review process. A summary of reviews will appear here.
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.
143+ --mount "${CI_CCACHE_MOUNT}"
144+ --mount "${CI_DEPENDS_MOUNT}"
145+ --mount "${CI_DEPENDS_SOURCES_MOUNT}"
146+ --mount "${CI_PREVIOUS_RELEASES_MOUNT}"
147+ )
148+ # Add worktree root mount, if needed
Why is any .git data needed to be copied, when an init .git is sufficient?
It would be easier to just call git init?
Hmmm, this was actually my first approach in the lint script, as it was simpler. Once I got this working in the Ci script, I backported a “matching” version to the linter script.
Looking at:
…I mistakenly thought that .git was actually needed, but i) it’s mounted RO, and ii) it’s only used as above here as far as I can see in the whole CI.
Can we just touch a (any) file to persist the flag then, and drop all .git requirement?
…I mistakenly thought that
.gitwas actually needed, but i) it’s mounted RO, and ii) it’s only used as above here as far as I can see in the whole CI.
It is also used in tidy via git --no-pager diff.
Can we just touch a (any) file to persist the flag then, and drop all
.gitrequirement?
Yes, but I don’t know how to do that in a one-liner cross-platform (linux+mac), taking into account the different root trees and permissions. So just using git seems simplest?
Is the (possible) running of 01_base_install.sh more than once deliberate? If so, then I’m curious why we ever do that? Is it just for the DANGER_RUN_CI_ON_HOST path or is there another use?
If not then it seems like we could just mark install as done when building the image, something like this…
It is also used in tidy via git –no-pager diff.
Does this not mean that it requires the actual .git directory to be mounted for the refs, meaning git init wouldn’t work in that case?
Is the (possible) running of 01_base_install.sh more than once deliberate? If so, then I’m curious why we ever do that? Is it just for the
DANGER_RUN_CI_ON_HOSTpath or is there another use?
It should only install once (otherwise there will be issues). Yes, it is for DANGER_RUN_CI_ON_HOST.
If not then it seems like we could just mark install as done when building the image, something like this…
lgtm. Could rename the file to /ci_base_install_done to include the ci prefix?
Does this not mean that it requires the actual .git directory to be mounted for the refs, meaning
git initwouldn’t work in that case?
Just for printing the diff, as long as the repo commits to the same tree (or even the staging index is the same), it should work. I see why you opted for copying .git, as it is a bit more future-proof and less confusing, given that the commit id also ends up in the binary or may otherwise be printed in the ci script? No strong opinion on how to fix it, though.
git init && git add ./, so no strong opinion.
30+ -it
31+)
32+
33+if [[ -n "$WORKTREE_ROOT" ]]; then
34+ # If in a worktree, mount both the current directory and the git root
35+ DOCKER_ARGS+=(--mount "type=bind,src=$WORKTREE_ROOT,dst=$WORKTREE_ROOT,readonly")
As far as I can tell, running bash -c '( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )' in a worktree already works fine for me, because the .git directory (referenced by the .git file that a worktree creates) is available to me, unlike in the containerised scenario where this path must be explicitly mounted into the container at runtime, which is what the helper script does.
I added some more comments to describe why the mount is needed in the script itself.
Apologies if I’ve misunderstood what you mean here.
Run CI in worktrees by mounting the .git root target into the container.
22@@ -23,7 +23,7 @@ to install the rust toolchain using your package manager of choice or
23 Then you can use:
24
25 ```sh
26-( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 cargo run )
27+( cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && RUST_BACKTRACE=1 COMMIT_RANGE=$(git rev-list --merges -n 1 HEAD)..HEAD cargo run )
32+ WORKTREE_ROOT=$(echo "$GIT_DIR" | sed 's/\.git\/worktrees\/.*/\.git/')
33+ echo "Worktree root: $WORKTREE_ROOT"
34+ DOCKER_ARGS+=(--mount "type=bind,src=$WORKTREE_ROOT,dst=$WORKTREE_ROOT")
35+fi
36+
37+RUST_BACKTRACE=full docker run "${DOCKER_ARGS[@]}" bitcoin-linter
0docker run "${DOCKER_ARGS[@]}" bitcoin-linter
I don’t think this env var will affect anything?
Also, style-wise I wonder if the contents of this file should be put into ci/lint_run_all.sh, so that there is only one bash script, instead of multiple.
Ideally there would only be one script, used by ci and locally, but that can be a follow-up. For now, just a single large if in ci/lint_run_all.sh should be fine to switch between the cirrus_ci code paths ( current content of ci/lint_run_all.sh) and a local run (current content of this file).
Run linter in worktrees by mounting the .git root target into the
container.
Rename `get_git_root` to `get_git_toplevel` to avoid confusion around
what this variable represents when modifying the code in the future.
Labels
Tests