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

sdaftuar commented at 1:32 pm on April 26, 2024: member

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

0DOCKER_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:

0fatal: 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!

sdaftuar added the label Feature on Apr 26, 2024

maflcko added the label Tests on Apr 26, 2024

achow101 commented at 4:06 pm on April 26, 2024: member

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

laanwj commented at 8:25 am on April 27, 2024: member

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 commented at 8:51 am on April 27, 2024: member

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 commented at 8:57 am on April 27, 2024: member

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 commented at 2:17 pm on April 28, 2024: none

I can’t reproduce that error:

 0bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc)
 1❯ DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter
 2[+] Building 3.4s (12/12) FINISHED                                                                                                                                    docker:default
 3 => [internal] load build definition from lint_imagefile                                                                                                                        0.0s
 4 => => transferring dockerfile: 708B                                                                                                                                            0.0s
 5 => [internal] load .dockerignore                                                                                                                                               0.0s
 6 => => transferring context: 2B                                                                                                                                                 0.0s
 7 => [internal] load metadata for docker.io/library/debian:bookworm                                                                                                              3.4s
 8 => [1/7] FROM docker.io/library/debian:bookworm@sha256:1aadfee8d292f64b045adb830f8a58bfacc15789ae5f489a0fedcd517a862cb9                                                        0.0s
 9 => [internal] load build context                                                                                                                                               0.0s
10 => => transferring context: 473B                                                                                                                                               0.0s
11 => CACHED [2/7] COPY ./.python-version /.python-version                                                                                                                        0.0s
12 => CACHED [3/7] COPY ./ci/lint/container-entrypoint.sh /entrypoint.sh                                                                                                          0.0s
13 => CACHED [4/7] COPY ./ci/lint/04_install.sh /install.sh                                                                                                                       0.0s
14 => CACHED [5/7] COPY ./test/lint/test_runner /test/lint/test_runner                                                                                                            0.0s
15 => 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
16 => CACHED [7/7] WORKDIR /bitcoin                                                                                                                                               0.0s
17 => exporting to image                                                                                                                                                          0.0s
18 => => exporting layers                                                                                                                                                         0.0s
19 => => writing image sha256:f1751706ad3a59a294be4c368f55e034b440840a6a892bcc84890ca31417d273                                                                                    0.0s
20 => => naming to docker.io/library/bitcoin-linter                                                                                                                               0.0s
21+ '[' -n '' ']'
22++ git rev-list --max-count=1 --merges HEAD
23+ COMMIT_RANGE=3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
24+ export COMMIT_RANGE
25+ echo
26
27+ git log --no-merges --oneline 3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
28+ echo
29
30+ test/lint/commit-script-check.sh 3aaf7328eb656b642e5f0f74f3e4d51645a1d0ab..HEAD
31+ RUST_BACKTRACE=1
32+ /lint_test_runner/test_runner
33src/crc32c in HEAD currently refers to tree 454691a9b89ee8b9e1f71a48a7398edba49c3805
34src/crc32c in HEAD was last updated in commit 5d45552fd4303f8d668ffbc50cce1053485aeead (tree 454691a9b89ee8b9e1f71a48a7398edba49c3805)
35GOOD
36src/crypto/ctaes in HEAD currently refers to tree 1b6c31139a71f80245c09597c343936a8e41d021
37src/crypto/ctaes in HEAD was last updated in commit 8501bedd7508ac514385806e191aec21ee978891 (tree 1b6c31139a71f80245c09597c343936a8e41d021)
38GOOD
39src/leveldb in HEAD currently refers to tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479
40src/leveldb in HEAD was last updated in commit 1a463c70a368fb20316e03efac273438cf47baa3 (tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479)
41GOOD
42src/minisketch in HEAD currently refers to tree a584efdc3ab5184a004807db66381c5c482e5f41
43src/minisketch in HEAD was last updated in commit 1eea10a6d25fd8225560347cda2b1cfdc267910d (tree a584efdc3ab5184a004807db66381c5c482e5f41)
44GOOD
45src/secp256k1 in HEAD currently refers to tree c3c4db9c0e4636135963272b005b11a451303feb
46src/secp256k1 in HEAD was last updated in commit 53eec53dca1cb677d11564b055d3b8581ddd6747 (tree c3c4db9c0e4636135963272b005b11a451303feb)
47GOOD
48Args used        : 210
49Args documented  : 222
50Args undocumented: 0
51set()
52Args unknown     : 12
53{'-zmqpubhashtx', '-zmqpubrawtx', '-zmqpubsequence', '-zmqpubhashblockhwm', '-zmqpubrawblock', '-testdatadir', '-zmqpubhashblock', '-zmqpubsequencehwm', '-zmqpubhashtxhwm', '-includeconf', '-zmqpubrawblockhwm', '-zmqpubrawtxhwm'}
54Success: no issues found in 292 source files
55+ '[' '' = bitcoin/bitcoin ']'
56bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc) took 23s
57❯ pwd
58/home/exec/Projects/github.com/bitcoin/bitcoin
59bitcoin on  exec/worktree via 🐍 v3.8.18 via ❄️   impure (btc)
60❯ git status
61On branch exec/worktree
62Changes not staged for commit:
63  (use "git add <file>..." to update what will be committed)
64  (use "git restore <file>..." to discard changes in working directory)
65        modified:   src/addrdb.cpp
66
67no changes added to commit (use "git add" and/or "git commit -a")

willcl-ark commented at 9:04 pm on May 2, 2024: member

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:~~

0DOCKER_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.:

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

willcl-ark commented at 7:51 am on May 3, 2024: member

Update:

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

0DOCKER_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 commented at 9:31 am on May 3, 2024: member

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 commented at 10:01 am on May 3, 2024: member

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 commented at 10:24 am on May 3, 2024: member

(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. :sweat_smile:

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 commented at 11:51 am on May 3, 2024: member

Thanks, that’s useful feedback.

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

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