ci: Slim down lint image

maflcko commented at 2:36 pm on April 11, 2025: member

Currently, the lint_test_runner is built and installed into the lint CI image. This is problematic, because it triggers a full image build on every change to its source code. Doing a build of the lint test_runner on every run is easier and faster.

DrahtBot commented at 2:36 pm on April 11, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32250.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, l0rinc
Stale ACK	kevkevinpal

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Apr 11, 2025

maflcko marked this as a draft on Apr 11, 2025

maflcko marked this as ready for review on Apr 11, 2025

ci: Bump lint imagefile FROM base

According to https://wiki.debian.org/DebianReleases#Production_Releases,
the current image will be EOL in about one year, so it seems fine to
slowly bump it to something more recent.

3333273a8f

ci: Slim down lint image

Currently, the lint_test_runner is built and installed into the lint CI
image. This is problematic, because it triggers a full image build on
every change to its source code. Doing a build of the lint test_runner
on every run is easier and faster.

fae322a43a

maflcko force-pushed on Apr 11, 2025

kevkevinpal commented at 9:02 pm on April 11, 2025: contributor

utACK fae322a

This change makes sense, it is more efficient. Not sure if you have run times from the build and how much faster this change makes it?

Google provides a table for what is the appropriate path to upgrade and Debian 12 "Bookworm" -> Ubuntu 24.04 looks right https://cloud.google.com/software-supply-chain-security/docs/base-images#google-provided_base_images
I also did grep -nri "LINT_RUNNER_PATH" ./ -I and found no results

maflcko commented at 6:35 am on April 12, 2025: member

Not sure if you have run times from the build and how much faster this change makes it?

It should skip the whole build completely of the image when the test_runner code is modified. Thus, the savings are how long it takes to build. (A few minutes)

You can try it via https://github.com/bitcoin/bitcoin/blob/master/test/lint/README.md#running-locally (using the docker commands). And something like echo >> ./test/lint/test_runner/src/main.rs to modify the code.

Previously, on master, it should trigger a full image re-build every time (including a full python build). Now, it should skip the re-build completely, because it is not needed.

Google provides a table for what is the appropriate path to upgrade and Debian 12 "Bookworm" -> Ubuntu 24.04 looks right https://cloud.google.com/software-supply-chain-security/docs/base-images#google-provided_base_images

I don’t think this is related. The google mirror is used, but not the google base images. Also, all packages, except for those installed via apt are hard-coded anyway, so the base image shouldn’t matter much, as long as it is not EOL.

in ci/lint_imagefile:7 in 3333273a8f outdated

3@@ -4,7 +4,7 @@
4 
5 # See test/lint/README.md for usage.
6 
7-FROM mirror.gcr.io/debian:bookworm
8+FROM mirror.gcr.io/ubuntu:24.04

l0rinc commented at 1:52 pm on April 13, 2025:

👍, End of Standard Support for 24.04 LTS (Noble Numbat) is Apr 2029

Nit: in a few other cases we’ve used the name of the release, e.g. mirror.gcr.io/ubuntu:noble. I like the version number more, in the future we might consider unifying these to simplify searches.

Q: the update reasoning seems to apply to CI_IMAGE_NAME_TAG as well - if you push again, consider unifying it - unless the variety is a deliberate feature, of course

maflcko commented at 7:35 am on April 14, 2025:

Nit: in a few other cases we’ve used the name of the release, e.g. mirror.gcr.io/ubuntu:noble. I like the version number more, in the future we might consider unifying these to simplify searches.

thx, done with a new trivial refactor commit.

Q: the update reasoning seems to apply to CI_IMAGE_NAME_TAG as well - if you push again, consider unifying it - unless the variety is a deliberate feature, of course

We want to check the minimum versions, so this is a bit more involved. Happy to review a pull, if someone creates one.

in ci/lint_imagefile:14 in fae322a43a outdated

12@@ -13,7 +13,6 @@ COPY ./ci/retry/retry /ci_retry
13 COPY ./.python-version /.python-version
14 COPY ./ci/lint/container-entrypoint.sh /entrypoint.sh

l0rinc commented at 2:12 pm on April 13, 2025:

nit: I see that we’re chmod 755-ing this later, given that we’re calling with DOCKER_BUILDKIT=1, we could do it inline instead:

0COPY --chmod=755 ./ci/lint/container-entrypoint.sh /entrypoint.sh

maflcko commented at 6:38 am on April 14, 2025:

Does it work with podman and on Cirrus CI’s container engine?

l0rinc commented at 5:57 pm on April 14, 2025:

It’s available apparently since 2017, I would expect it to work - we can try it in a follow-up PR

in ci/lint_imagefile:18 in fae322a43a outdated

12@@ -13,7 +13,6 @@ COPY ./ci/retry/retry /ci_retry
13 COPY ./.python-version /.python-version
14 COPY ./ci/lint/container-entrypoint.sh /entrypoint.sh
15 COPY ./ci/lint/04_install.sh /install.sh
16-COPY ./test/lint/test_runner /test/lint/test_runner
17 
18 RUN /install.sh && \
19   echo 'alias lint="./ci/lint/06_script.sh"' >> ~/.bashrc && \

l0rinc commented at 2:37 pm on April 13, 2025:

Where are we using this alias? lint_run.sh and container-entrypoint.sh both contain ./ci/lint/06_script.sh and build seems to pass without it.

Which leaves rm -rf /var/lib/apt/lists/* still, which seems like it belongs to the installation script instead - since it knows whether it dirtied the apt cache in the first place and would make the cleanup more self-contained.

maflcko commented at 6:41 am on April 14, 2025:

I didn’t add the alias, but I presume it exists when someone jumps into the container and wants to use it. No strong opinion from my side, I’ll leave as-is for now.

in ci/lint/04_install.sh:40 in fae322a43a outdated

46-    mv target/debug/test_runner "${LINT_RUNNER_PATH}"
47-  )
48-fi
49-
50 ${CI_RETRY_EXE} pip3 install \
51   codespell==2.2.6 \

l0rinc commented at 2:41 pm on April 13, 2025:

unrelated nit: when running the build, I notice again that we have a few leftover typos (gloabl and inteference), thought we might as well fix them here to avoid all those typo fixing spam PRs. But updating codespell reveals even more false positives, so it it should likely be done in a separate PR. Shellcheck is also quite outdated (version used is from Nov 7, 2021, latest is from Mar 8, 2024), we could bump those (and the other dependencies) as well in a separate PR.

maflcko commented at 6:36 am on April 14, 2025:

No strong opinion from my side, I’ll leave as-is for now. someone can create a separate PR for those.

in ci/lint_imagefile:9 in fae322a43a outdated

 3@@ -4,7 +4,7 @@
 4 
 5 # See test/lint/README.md for usage.
 6 
 7-FROM mirror.gcr.io/debian:bookworm
 8+FROM mirror.gcr.io/ubuntu:24.04
 9 
10 ENV DEBIAN_FRONTEND=noninteractive

l0rinc commented at 2:48 pm on April 13, 2025:

While we don’t seem to have any interactivity currently (and there are more explicit ways to tackle that) and since we might only need this during build, we could either prefix the run command with it, or make it an ARG (instead of an ENV).

0ARG DEBIAN_FRONTEND=noninteractive

maflcko commented at 6:37 am on April 14, 2025:

No strong opinion from my side, I’ll leave as-is for now.

l0rinc commented at 5:57 pm on April 14, 2025:

What’s the rationale for polluting the runtime with an environmental variable that is only needed during build (and in this case not even during build)?

maflcko commented at 7:37 am on April 15, 2025:

What’s the rationale for polluting the runtime with an environmental variable that is only needed during build (and in this case not even during build)?

The same happens in the ci/test system, see ci/test/00_setup_env.sh:export DEBIAN_FRONTEND=noninteractive.
It seems cleaner to just set up the expectations once for the apt install step, instead of having to possibly adjust it every time a package is added or removed from the list.

Seems fine to move it to the install script itself, if you want in a follow-up.

in ci/lint/04_install.sh:19 in fae322a43a outdated

11@@ -12,10 +12,11 @@ pushd "/"
12 
13 ${CI_RETRY_EXE} apt-get update
14 # Lint dependencies:
15+# - cargo (used to run the lint tests)
16 # - curl/xz-utils (to install shellcheck)
17 # - git (used in many lint scripts)
18 # - gpg (used by verify-commits)
19-${CI_RETRY_EXE} apt-get install -y curl xz-utils git gpg
20+${CI_RETRY_EXE} apt-get install -y cargo curl xz-utils git gpg

l0rinc commented at 3:27 pm on April 13, 2025:

later we’re reinstalling curl and xz-utils, we could remove those

maflcko commented at 6:35 am on April 14, 2025:

I don’t think they are re-installed. Installing a package twice will just skip it the second time, but in scripts each occurrence is there for explicit documentation purposes.

l0rinc commented at 5:57 pm on April 14, 2025:

That’s what I meant, of course, but if you think duplicating them is better for documentation, I’m fine with it of course

l0rinc commented at 3:46 pm on April 13, 2025: contributor

Checked that we don’t rebuild anymore (I trust that we did before):

0DOCKER_BUILDKIT=1 docker build --file "./ci/lint_imagefile" ./ && \
1echo >> ./test/lint/test_runner/src/main.rs && \
2DOCKER_BUILDKIT=1 docker build -t bitcoin-linter --file "./ci/lint_imagefile" ./ && docker run --rm -v $(pwd):/bitcoin -it bitcoin-linter

0 => [1/7] FROM mirror.gcr.io/ubuntu:24.04@sha256:1e622c5f073b4f6bfad6632f2616c7f59ef256e96fe78bf6a595d1dc4376ac02                                                          0.0s
1 => [internal] load build context                                                                                                                                          0.0s
2 => => transferring context: 239B                                                                                                                                          0.0s
3 => CACHED [2/7] COPY ./ci/retry/retry /ci_retry                                                                                                                           0.0s
4 => CACHED [3/7] COPY ./.python-version /.python-version                                                                                                                   0.0s
5 => CACHED [4/7] COPY --chmod=755 ./ci/lint/container-entrypoint.sh /entrypoint.sh                                                                                         0.0s
6 => CACHED [5/7] COPY ./ci/lint/04_install.sh /install.sh                                                                                                                  0.0s
7 => CACHED [6/7] RUN /install.sh                                                                                                                                           0.0s
8 => CACHED [7/7] WORKDIR /bitcoin

Left a few cleanup nits, but LGTM otherwise, ACK fae322a43a4b71b95b68f0674a5d2bd597fee68b

janb84 commented at 5:50 pm on April 14, 2025: contributor

ACK fac9709

Tested ✅
- image is slightly larger 2.82 GB vs 2.74 GB
- image does not rebuild when the Rust source is changed ✅. (Works as advertised, also tested that the version on master rebuilds on change)
- (test env. Mac arm, docker desktop version 4.40)

DrahtBot requested review from l0rinc on Apr 14, 2025

l0rinc commented at 6:06 pm on April 14, 2025: contributor

reACK fac97092975e75f074505c77429b57ba9bb62b3e

The new links also seem to point to the correct locations - though the first comment states version 13.3, while the link seems to point to version 13.2.

ci: refactor: Use version id over version codename consistently

Also, remove the minor version from the comment, because it does not
matter and may even change at any time.

faeb1babe2

maflcko force-pushed on Apr 15, 2025

maflcko commented at 7:47 am on April 15, 2025: member

The new links also seem to point to the correct locations - though the first comment states version 13.3, while the link seems to point to version 13.2.

Pushed a doc-only comment fixup, should be trivial to re-ack without testing again.

janb84 commented at 7:50 am on April 15, 2025: contributor

Re- ACK faeb1ba

Difference since last ACK:

small comment change.

l0rinc commented at 7:52 am on April 15, 2025: contributor

ACK faeb1babe283d8d61d830ce78310ce23984543e2

maflcko added this to the milestone 30.0 on Apr 15, 2025

maflcko removed this from the milestone 30.0 on Apr 15, 2025

ci: Slim down lint image #32250

Code Coverage & Benchmarks

Reviews

Conflicts