👍 l0rinc approved a pull request: "ci: Only write docker build images to Cirrus cache"
(https://github.com/bitcoin/bitcoin/pull/33639#pullrequestreview-3360383974)
Concept ACK, I understand that you already have an ACK but please consider my suggestions

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448019150)
hmmm, not sure I get why we need this randomness here, is it to allow multiple runs without synchronization?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448054647)
I assume that changing the `MAYBE_CPUSET` position is irrelevant, right?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448072260)
nit: we were logging `CI_IMAGE_NAME_TAG` before (just resolve if that's intentional)

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448057503)
for symmetry can we inline + unpack this?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448120770)
slightly tangential, but maybe there's room for optimizing the docker layers by putting the variable parts as the last layers, using ARGs over ENVs, combining multiple layers, pinning versions and paths and variables early, pruning old layers, multi-stage builds, etc.

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448149644)
I don't yet understand why we're pinning CPUs here, what problem is that solving that `--cpus=` wouldn't?
The comment is a bit misleading since my understanding is that `--cpuset-cpus` doesn't actually limit CPU usage.

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448113232)
Do we still need this? My understanding is that we're not really using this - can we delete and add back when there's a well-defined usecase for it?

If we do, we might want to add `MAKEJOBS` guard as well since that is also needed

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448139543)
Is this related to the mentioned https://www.shellcheck.net/wiki/SC2086? Because we need split to handle quotes and escaping properly?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448081592)
Can we inline `cmd_build`?
```python
run(["docker", "buildx", "build",
f"--file={os.environ['BASE_READ_ONLY_DIR']}/ci/test_imagefile",
f"--build-arg=CI_IMAGE_NAME_TAG={os.environ['CI_IMAGE_NAME_TAG']}",
f"--build-arg=FILE_ENV={os.environ['FILE_ENV']}",
f"--build-arg=BASE_ROOT_DIR={os.environ['BASE_ROOT_DIR']}",
f"--platform={os.environ['CI_IMAGE_PLATFORM']}",
f"--label={CI_IMAGE_LABEL}",
f"--tag={os.environ['CONTAINER_NAME']}",
*maybe_cpuset(),
*shlex.split(os
...

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448342637)
Ah nice, changed all

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344025)
thx, took your doc comment

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344564)
(removed)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344952)
Yes, exactly, see also the commit message :)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346083)
In theory yes, but in practise it does not work due to a docker bug. Also, I don't think it is worth it in practise, see https://github.com/bitcoin/bitcoin/pull/33620

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346402)
(removed)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346644)
No, a follow-up requires it to be separate :)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448347024)
Yes, `CI_IMAGE_NAME_TAG` is just a general `ubuntu:24.04`. I think logging the more specific name, like `ci_native_tidy` seems useful.

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448347492)
thx, inlined

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448348039)
(yes, it should be irrelevant, but removed)