I discovered that running this command (from test/lint/READM

I can't reproduce that error: <div class="snippet-clipboard-content notranslate po

(as part of a general python revamp in <a href="https://github.com/bitcoi

qa: Support git worktrees when running the linters locally via Docker about bitcoin HOT 11 OPEN

sdaftuar commented on May 27, 2024

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

from bitcoin.

Comments (11)

laanwj commented on May 27, 2024 1

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.

from bitcoin.

achow101 commented on May 27, 2024

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

from bitcoin.

laanwj commented on May 27, 2024

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).

from bitcoin.

maflcko commented on May 27, 2024

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?

from bitcoin.

eval-exec commented on May 27, 2024

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")

from bitcoin.

willcl-ark commented on May 27, 2024

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

from bitcoin.

willcl-ark commented on May 27, 2024

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.

from bitcoin.

maflcko commented on May 27, 2024

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?

from bitcoin.

willcl-ark commented on May 27, 2024

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.

from bitcoin.

maflcko commented on May 27, 2024

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

from bitcoin.

willcl-ark commented on May 27, 2024

Thanks, that's useful feedback.

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

from bitcoin.

qa: Support git worktrees when running the linters locally via Docker about bitcoin HOT 11 OPEN

Comments (11)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent