test: add an easy way to run linters locally #26906

1. 
   jamesob commented at 4:55 pm on January 17, 2023: member

   Adds a Dockerfile configuration (originally written mostly by fanquake) that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally.

   I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches.

   Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI.

2. 
   DrahtBot commented at 4:55 pm on January 17, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	aureleoules, stickies-v, john-moffett

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Jan 17, 2023
4. in contrib/devtools/lint.dockerfile:15 in ffc627ec0f outdated

   10+ARG FLAKE=5.0.4
   11+ARG MYPY=0.971
   12+ARG PYTHON=3.6.15
   13+ARG PYZMQ=24.0.1
   14+ARG SHELLCHECK=v0.8.0
   15+ARG VULTURE=2.6
   

   ---

   ---

   

   maflcko commented at 5:03 pm on January 17, 2023:

   You claim that

   live in two separate places is a recipe for version mismatches.

   But in this pull you are duplicating the versions to two places in this repo. (As well as all other stuff)

5. 
   maflcko commented at 5:04 pm on January 17, 2023: member

   Not sure about duplicating the versions and everything else. As you say, this is a recipe for errors.

   Also, the lint CI fails on this

6. 
   jamesob commented at 5:06 pm on January 17, 2023: member

   Okay well I don’t really care which way we go here, but the situation right now is really untenable because without this dockerfile it’s basically not feasible to replicate linting locally. As far as I can tell, it either lives here or in a third party repo.

   Do you have an alternate suggestion that will make it reasonable for developers to run the CI lints locally?

7. 
   maflcko commented at 5:12 pm on January 17, 2023: member

   I am not too fluent in docker, so I can’t provide alternative suggestions. However, if you find a way to not duplicate everything, it should be more acceptable to merge?
8. 
   jamesob commented at 5:14 pm on January 17, 2023: member

   I don’t even care about docker, I just want a way to run linters locally. How do you do it?
9. 
   maflcko commented at 5:32 pm on January 17, 2023: member

   How do you do it?

   I don’t run the linters. When I write a scripted diff I call the script-check directly; I know not to modify subtrees, so I don’t need to lint for that; And for the python stuff I just fix it up after opening a PR.

   Depending on how many features you want if you run locally, there are alternatives:

   • If you only want to run the python stuff, Extracting the versions into a requirements.txt might be enough before calling lint-all.
   • If you want to run more than that, it can be extracted into new scripts from the scripts in ./ci/lint/. (Excluding the destructive stuff like sed -i -e 's/bionic/jammy/g' /etc/apt/sources.list, …)
   • If you want to run the exact CI env, I don’t think it will be possible without docker.
10. 
    maflcko commented at 5:38 pm on January 17, 2023: member

    (Related merge commit: 01ec5308bf616740c804247043046b23b483df5c)
11. jamesob force-pushed on Jan 17, 2023
12. 
    jamesob commented at 5:44 pm on January 17, 2023: member

    Okay, I think the new HEAD here is a pretty good compromise. Thanks for pushing me to avoid the duplication.

    Basically, I’m just reusing the 04_install.sh script from within a container.

13. jamesob force-pushed on Jan 17, 2023
14. in ci/lint/Dockerfile:1 in 84ed0ce192 outdated

    0@@ -0,0 +1,19 @@
    1+# See test/lint/README.md for usage.
    

    ---

    ---

    

    maflcko commented at 6:20 pm on January 17, 2023:

    nit: Given that this isn’t used by the ci, mabye move this file to test/lint and remove this comment? (The paths below would need to be adjusted)

    ---

    

    jamesob commented at 6:36 pm on January 17, 2023:

    Yeah I started this way, but because the docker build context either (i) pulls in entire tree when run from root (super slow) and (ii) doesn’t allow “back references” to files outside of the context, it kind of has to live here. There wasn’t another good workaround that I could find.

    ---

    

    maflcko commented at 6:42 pm on January 17, 2023:

    nit: Ah ok, maybe add a comment why this needs to live here.
15. in ci/lint/Dockerfile:11 in 84ed0ce192 outdated

     6+
     7+# Must be built from ./ci/lint/ for these paths to work.
     8+COPY ./docker-entrypoint.sh /entrypoint.sh
     9+COPY ./04_install.sh /install.sh
    10+
    11+RUN apt-get update && \
    

    ---

    ---

    

    maflcko commented at 6:22 pm on January 17, 2023:

    nit: apt update is already done by install.sh?
16. in ci/lint/Dockerfile:13 in 84ed0ce192 outdated

     8+COPY ./docker-entrypoint.sh /entrypoint.sh
     9+COPY ./04_install.sh /install.sh
    10+
    11+RUN apt-get update && \
    12+  /install.sh && \
    13+  mv /tmp/shellcheck-*/shellcheck /usr/bin/ && \
    

    ---

    ---

    

    maflcko commented at 6:24 pm on January 17, 2023:

    nit: Maybe just put the bin in this dir in ./ci/lint/04_install.sh to avoid duplication here?

    ---

    

    jamesob commented at 8:49 pm on January 17, 2023:

    Good call, will do.
17. in ci/lint/Dockerfile:25 in 84ed0ce192 outdated

    11+RUN apt-get update && \
    12+  /install.sh && \
    13+  mv /tmp/shellcheck-*/shellcheck /usr/bin/ && \
    14+  echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc && \
    15+  chmod 755 /entrypoint.sh && \
    16+  rm -rf /var/lib/apt/lists/*
    

    ---

    ---

    

    maflcko commented at 6:26 pm on January 17, 2023:

    (unrelated: It may be possible to remove the packages used to build python, but that seems overkill)
18. maflcko approved
19. 
    maflcko commented at 6:28 pm on January 17, 2023: member

    lgtm. Left some nits (feel free to ignore)
20. jamesob force-pushed on Jan 18, 2023
21. 
    jamesob commented at 1:46 pm on January 18, 2023: member

    Okay, so I’ve addressed @MarcoFalke’s feedback. Because of the merge of https://github.com/bitcoin/bitcoin/commit/01ec5308bf616740c804247043046b23b483df5c, I’ve had to introduce kind of a controversial change that hopefully won’t be at all controversial when we merge #26226 - because the .python-version file is difficult to access during container build for previously discussed reasons, I’m just basing this lint container on the preexisting python:3.7-buster image. One benefit here is that it’s way quicker to build than compiling Python from source - but obviously it isn’t Python 3.6 because 3.6 is so old that official Docker channels don’t even offer an image for it.

    I think this is totally okay for running linters locally, and once we bump to 3.7 (hopefully soon) this shouldn’t raise any objections…

22. 
    maflcko commented at 1:58 pm on January 18, 2023: member

    3.6 was removed three days ago: https://github.com/docker-library/python/commit/2e0f1dc3300acc08b82a95857fd9c87ac1f91672

    But yeah, seems fine to use any python version locally, as this should rarely lead to errors and the local thing is best-effort anyway.

23. in test/lint/README.md:14 in 80693887e3 outdated

     9+```sh
    10+cd ./ci/lint
    11+docker build -t bitcoin-linter .
    12+
    13+cd /root/of/bitcoin/repo
    14+docker run --rm -v $(pwd):/bitcoin -w /bitcoin -it bitcoin-linter
    

    ---

    ---

    

    aureleoules commented at 2:07 pm on January 18, 2023:

    Could move the -w /bitcoin argument to the Dockerfile using WORKDIR /bitcoin. Makes the command shorter.
24. 
    aureleoules commented at 2:16 pm on January 18, 2023: member

    From Cirrus:

    +# This is used by the 04_install.sh script; we can’t read the Python version from ^—- failure generated from lint-whitespace.py

    Nice to see the linter linting the linter. But when I execute the linter locally on this PR with docker run --rm -v $(pwd):/bitcoin -w /bitcoin -it bitcoin-linter I don’t get the warning, why?

     0docker run --rm -v $(pwd):/bitcoin -w /bitcoin -it bitcoin-linter
     1src/crypto/ctaes in HEAD currently refers to tree 1b6c31139a71f80245c09597c343936a8e41d021
     2src/crypto/ctaes in HEAD was last updated in commit 8501bedd7508ac514385806e191aec21ee978891 (tree 1b6c31139a71f80245c09597c343936a8e41d021)
     3GOOD
     4src/secp256k1 in HEAD currently refers to tree afb14c4f45fa7eb8651daced71faddebe0fba179
     5src/secp256k1 in HEAD was last updated in commit 9d47e7b71b2805430e8c7b43816efd225a6ccd8c (tree afb14c4f45fa7eb8651daced71faddebe0fba179)
     6GOOD
     7src/minisketch in HEAD currently refers to tree a749309e1b4edf48091edb57da6de2e8a16498a5
     8src/minisketch in HEAD was last updated in commit e9f1d8c27261070d209a28570ad36fe91531cdd2 (tree a749309e1b4edf48091edb57da6de2e8a16498a5)
     9GOOD
    10src/leveldb in HEAD currently refers to tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479
    11src/leveldb in HEAD was last updated in commit 1a463c70a368fb20316e03efac273438cf47baa3 (tree bd49d3c60051c1a8d8f030fa4ba7a29237fe7479)
    12GOOD
    13src/crc32c in HEAD currently refers to tree 5a1b5f7188bd2181e2a71d431fff282d50058e46
    14src/crc32c in HEAD was last updated in commit 08269e54a9a74e06c9fb72720a216d8c4d4532a2 (tree 5a1b5f7188bd2181e2a71d431fff282d50058e46)
    15GOOD
    16Args used        : 208
    17Args documented  : 219
    18Args undocumented: 0
    19set()
    20Args unknown     : 11
    21{'-zmqpubhashtx', '-zmqpubrawtx', '-zmqpubrawblockhwm', '-zmqpubhashblockhwm', '-zmqpubsequence', '-zmqpubhashblock', '-zmqpubhashtxhwm', '-zmqpubrawtxhwm', '-zmqpubrawblock', '-zmqpubsequencehwm', '-includeconf'}
    22Success: no issues found in 258 source files
    

25. 
    jamesob commented at 2:45 pm on January 18, 2023: member

    But when I execute the linter locally on this PR with docker run –rm -v $(pwd):/bitcoin -w /bitcoin -it bitcoin-linter I don’t get the warning, why?

    I’m working on this right now - it turns out that the lint script uses an envvar set by Cirrus CI to specify a range of commits to look at for whitespace issues. Another discrepancy that this PR will soon resolve I hope.

26. test: add an easy way to run linters locally

    Adds a Dockerfile configuration
    that allows straightforward running of linters with compatible versions
    locally. This removes a ton of annoyance when trying to appease CI,
    because many of the linter versions are quite old and difficult to
    maintain locally.
    
    I realize that people may not be thrilled to more ancillary tooling to
    the repo, but I think this makes a lot of sense given the linter
    versions listed in this container configuration are dictated by this
    repo (within the CI configuration), so having these things live in
    two separate places is a recipe for version mismatches.
    
    Eventually we can likely just use this container on CI directly to avoid
    any chance of inconsistencies between local dev experience and CI.

    dff7ed5732
27. lint: specify the right commit range when running locally

    When running lints on Cirrus, a special envvar is set ($CIRRUS_PR);
    emulate this when running linters locally by setting $LOCAL_BRANCH
    to any value.

    b68e5a7fef
28. jamesob force-pushed on Jan 18, 2023
29. 
    jamesob commented at 2:50 pm on January 18, 2023: member

    I’m working on this right now - it turns out that the lint script uses an envvar set by Cirrus CI to specify a range of commits to look at for whitespace issues. Another discrepancy that this PR will soon resolve I hope. @aureleoules this is now fixed. I think this is pretty near ready to go from a code standpoint.

30. in ci/lint/Dockerfile:23 in b68e5a7fef

    18+# Must be built from ./ci/lint/ for these paths to work.
    19+COPY ./docker-entrypoint.sh /entrypoint.sh
    20+COPY ./04_install.sh /install.sh
    21+
    22+RUN /install.sh && \
    23+  echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc && \
    

    ---

    ---

    

    aureleoules commented at 3:10 pm on January 18, 2023:

    nit: This is unused.

    ---

    

    jamesob commented at 3:16 pm on January 18, 2023:

    Useful when dropping into bash (i.e. not using default command). Going to leave as-is.
31. aureleoules approved
32. 
    aureleoules commented at 3:11 pm on January 18, 2023: member

    ACK b68e5a7feff3e93027e75da0cd9a590fef99aac1

    I verified it builds and runs successfully and is now able to detect whitespace issues.

33. 
    stickies-v commented at 11:37 pm on January 18, 2023: contributor

    Concept ACK, thanks for working on this tooling
34. 
    jamesob commented at 4:13 pm on January 19, 2023: member

    Is good? Pretty low-risk change that would be nice to have in.
35. in test/lint/README.md:13 in b68e5a7fef

     8+
     9+```sh
    10+cd ./ci/lint
    11+docker build -t bitcoin-linter .
    12+
    13+cd /root/of/bitcoin/repo
    

    ---

    ---

    

    stickies-v commented at 4:23 pm on January 19, 2023:

    since we’re already assuming starting in the root 2 lines up, I’d just replace this with

    0cd ../..
    

36. in ci/lint/Dockerfile:5 in b68e5a7fef

    0@@ -0,0 +1,29 @@
    1+# See test/lint/README.md for usage.
    2+#
    3+# This container basically has to live in this directory in order to pull in the CI
    4+# install scripts. If it lived in the root directory, it would have to pull in the
    5+# entire repo as docker context during build; if it lived elsewhere, it wouldn't be
    

    ---

    ---

    

    stickies-v commented at 4:43 pm on January 19, 2023:

    If it lived in the root directory, it would have to pull in the entire repo as docker context during build;

    Would you have preferred it lived in root? I think it makes more sense in /ci/lint, but if it would be better you could use .dockerignore to prevent adding everything to your docker context, as in this diff:

     0diff --git a/.dockerignore b/.dockerignore
     1new file mode 100644
     2index 000000000..142df6af9
     3--- /dev/null
     4+++ b/.dockerignore
     5@@ -0,0 +1,2 @@
     6+*
     7+!/ci/lint
     8\ No newline at end of file
     9diff --git a/ci/lint/Dockerfile b/Dockerfile
    10similarity index 91%
    11rename from ci/lint/Dockerfile
    12rename to Dockerfile
    13index 03c20c728..87c9bb85a 100644
    14--- a/ci/lint/Dockerfile
    15+++ b/Dockerfile
    16@@ -16,8 +16,8 @@ ENV LC_ALL=C.UTF-8
    17 ENV SKIP_PYTHON_INSTALL=1
    18 
    19 # Must be built from ./ci/lint/ for these paths to work.
    20-COPY ./docker-entrypoint.sh /entrypoint.sh
    21-COPY ./04_install.sh /install.sh
    22+COPY ./ci/lint/docker-entrypoint.sh /entrypoint.sh
    23+COPY ./ci/lint/04_install.sh /install.sh
    24 
    25 RUN /install.sh && \
    26   echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc && \
    

    ---

    

    maflcko commented at 2:10 pm on January 27, 2023:

    For reference, you can avoid passing the full context by using buildkit or podman. See

    • https://docs.docker.com/build/buildkit/#overview
    • https://docs.podman.io/en/latest/markdown/podman-build.1.html#name
37. stickies-v approved
38. 
    stickies-v commented at 4:49 pm on January 19, 2023: contributor

    ACK b68e5a7fe

    Not an expert in our linter or Docker, but the code looks safe and reasonable to me. I tested locally, and verified that bitcoin-linter errors as expected after added a whitespace commit.

    Looks low-risk to add and can be improved upon later if needed.

39. john-moffett approved
40. 
    john-moffett commented at 5:57 pm on January 19, 2023: contributor

    ACK b68e5a7feff3e93027e75da0cd9a590fef99aac1

    Works as expected on Intel Mac and detected my test issues. Thanks!

41. maflcko merged this on Jan 19, 2023
42. maflcko closed this on Jan 19, 2023

    ---

43. sidhujag referenced this in commit edef872c4f on Jan 19, 2023
44. bitcoin locked this on Jan 27, 2024

Contributors
jamesob DrahtBot maflcko aureleoules stickies-v john-moffett

Labels
Tests