qa: Support git worktrees when running the linters locally via Docker

[sdaftuar]

I discovered that running this command (from test/lint/README.md):

DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter

doesn't work when trying to run it out of a git worktree:

fatal: not a git repository: /home/sdaftuar/bitcoin/.git/worktrees/2024-04-cluster-mempool-txgraph-rebased

(Though it did work for me when I tried checking out my branch in a regular git repo).

I don't know anything about these scripts, but if there's an easy way to support worktrees I think it would be useful!

[achow101]

Yes, this happens if you try to run any of the CI tasks locally in a worktree.

[laanwj]

i think the root problem here is that docker only provides access to the source directory to the container, and a worktree isn't self-contained, it contains a path to the actual repository the worktree comes from, which won't be available there. The linters need access to git metadata to do their checks (for example, to make sure that they only check files in git, not generated ones).

[maflcko]

Yes, this happens if you try to run any of the CI tasks locally in a worktree.

At least for the "real" CI tasks, we should consider making a fallback, as I don't think much from git is needed? So far I could only find git log -1 in ci/test/, but that should work in a worktree, no?

[laanwj]

but that should work in a worktree, no?

All git commands work in a worktree, but not when it is seperated from its main repository by containers.

[eval-exec]

I can't reproduce that error:

bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc)
❯ DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter
[+] Building 3.4s (12/12) FINISHED                                                                                                                                    docker:default
 => [internal] load build definition from lint_imagefile                                                                                                                        0.0s
 => => transferring dockerfile: 708B                                                                                                                                            0.0s
 => [internal] load .dockerignore                                                                                                                                               0.0s
 => => transferring context: 2B                                                                                                                                                 0.0s
 => [internal] load metadata for docker.io/library/debian:bookworm                                                                                                              3.4s
 => [1/7] FROM docker.io/library/debian:bookworm@sha256:1aadfee8d292f64b045adb830f8a58bfacc15789ae5f489a0fedcd517a862cb9                                                        0.0s
 => [internal] load build context                                                                                                                                               0.0s
 => => transferring context: 473B                                                                                                                                               0.0s
 => CACHED [2/7] COPY ./.python-version /.python-version                                                                                                                        0.0s
 => CACHED [3/7] COPY ./ci/lint/container-entrypoint.sh /entrypoint.sh                                                                                                          0.0s
 => CACHED [4/7] COPY ./ci/lint/04_install.sh /install.sh                                                                                                                       0.0s
 => CACHED [5/7] COPY ./test/lint/test_runner /test/lint/test_runner                                                                                                            0.0s
 => CACHED [6/7] RUN /install.sh &&   echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc &&   chmod 755 /entrypoint.sh &&   rm -rf /var/lib/apt/lists/*                    0.0s
 => CACHED [7/7] WORKDIR /bitcoin                                                                                                                                               0.0s
 => exporting to image                                                                                                                                                          0.0s
 => => exporting layers                                                                                                                                                         0.0s
 => => writing image sha256:f1751706ad3a59a294be4c368f55e034b440840a6a892bcc84890ca31417d273                                                                                    0.0s
 => => naming to docker.io/library/bitcoin-linter                                                                                                                               0.0s
+ '[' -n '' ']'
++ git rev-list --max-count=1 --merges HEAD
+ COMMIT_RANGE=3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
+ export COMMIT_RANGE
+ echo

+ git log --no-merges --oneline 3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
+ echo

+ test/lint/commit-script-check.sh 3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
+ RUST_BACKTRACE=1
+ /lint_test_runner/test_runner
src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
src/crc32c in HEAD was last updated in commit 5d45552fd4303f8d668ffbc50cce1053485aeead (tree 454691a9b89ee8b9e1f71a48a7398edba49c3805)
GOOD
src/crypto/ctaes in HEAD currently refers to tree 1b6c31139a71f80245c09597c343936a8e41d021
src/crypto/ctaes in HEAD was last updated in commit 8501bedd7508ac514385806e191aec21ee978891 (tree 1b6c31139a71f80245c09597c343936a8e41d021)
GOOD
src/leveldb in HEAD currently refers to tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479
src/leveldb in HEAD was last updated in commit 1a463c70a368fb20316e03efac273438cf47baa3 (tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479)
GOOD
src/minisketch in HEAD currently refers to tree a584efdc3ab5184a004807db66381c5c482e5f41
src/minisketch in HEAD was last updated in commit 1eea10a6d25fd8225560347cda2b1cfdc267910d (tree a584efdc3ab5184a004807db66381c5c482e5f41)
GOOD
src/secp256k1 in HEAD currently refers to tree c3c4db9c0e4636135963272b005b11a451303feb
src/secp256k1 in HEAD was last updated in commit 53eec53dca1cb677d11564b055d3b8581ddd6747 (tree c3c4db9c0e4636135963272b005b11a451303feb)
GOOD
Args used        : 210
Args documented  : 222
Args undocumented: 0
set()
Args unknown     : 12
{'-zmqpubhashtx', '-zmqpubrawtx', '-zmqpubsequence', '-zmqpubhashblockhwm', '-zmqpubrawblock', '-testdatadir', '-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubhashtxhwm', '-includeconf', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
Success: no issues found in 292 source files
+ '[' '' = bitcoin/bitcoin ']'
bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc) took 23s
❯ pwd
/home/exec/Projects/github.com/bitcoin/bitcoin
bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc)
❯ git status
On branch exec/worktree
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   src/addrdb.cpp

no changes added to commit (use "git add" and/or "git commit -a")

[willcl-ark]

On closer inspection, this seems to end up writing some files as root in the mypy_cache dire worktree which is not ideal as it means the worktree can't be deleted in the usual way.

I think it can be done, although it's quite hacky:

DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && GIT_DIR=$(git rev-parse --path-format=absolute --git-common-dir) docker run --rm -v $(pwd):/bitcoin -v "$GIT_DIR":"$GIT_DIR" -it bitcoin-linter

This mounts the external (real) .git directory (which is a symlink in the worktree, and not possible to follow from inside of the container) to the same location inside of the container, making the symlink work again. The mount is not quite perfect and could perhaps be refined, but git does not seem to mind.

I don't think it will be possible to support worktrees in a less hacky way with the container, so the other option is to activate a python (v)env with required deps + shellcheck, and run the test-runner manually in the worktree with e.g.:

cd test/lint/test_runner
COMMIT_RANGE="$( git rev-list --max-count=1 --merges HEAD )..HEAD" cargo run

[willcl-ark]

Update:

To avoid creating root-owned mypy files you can run the container with your user:group specified:

DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && GIT_DIR=$(git rev-parse --path-format=absolute --git-common-dir) docker run --rm -v $(pwd):/bitcoin -v "$GIT_DIR":"$GIT_DIR" -u $(id -u):$(id -g) -it bitcoin-linter

This allows git worktree remove to succeed.

[maflcko]

To avoid creating root-owned mypy files

Probably unrelated, but ideally, I'd say that the container should not modify anything on the host. The "real" CI script use a readonly --mount.

If something needs to be cached, it can be done in a persisted volume?

[willcl-ark]

Yes I agree, this bothers me too.

Seems like we could perhaps use mypy's --cache-dir option to have it use /dev/null and avoid modifying source dir. I might try it out with the RO mount soon and see if it works OK that way or if there's anything else trying to make changes to the FS.

I don't want to derail this issue, but in a somewhat-related changeset I was also wondering if I could speed up the linter image build (with better layer caching) by building from the python base image and absorbing the install script into the image (as part of a general python revamp in this branch).

I'd be curious what you thought of this approach @maflcko (perhaps I should open another issue to discuss it?) ISTM that without the install script, and with the introduction of requirements-lint.txt developers would generally be able to run the local linter (pip install -r requirements-lint.txt && cd test/lint/test_runner && cargo run ...) more easily, and the container can build can be sped up quite significantly. What I'm not sure of is if the script has other purposes which this would interfere with...

NB: the CI stuff in that branch is broken/WIP, and I'm not convinced in any benefit to merging .style.yapf into a potential pyproject.toml.

[maflcko]

(as part of a general python revamp in this branch).

Not sure about complicating the cirrus yml. I think it should be considered to be set in stone, because any changes will break re-running the lint task on old pull requests, IIRC. At this point, if you want to change it substantially, it could make sense to move it to GHA, along with an update to DrahtBot to be able to re-run GHA tasks. 😅

Also, not sure about dockers python:, as that removes the flexibility to use (let's say) debian:trixie, or ubuntu:noble.

No objection about using more layers, if you think that is useful and needed often.

[willcl-ark]

Thanks, that's useful feedback.

TBH I really don't enjoy making changes to the CI anyway.