If they remain unset, they use the default anyway. Except for USER, but this seems unused anyway.
Can be checked via:
0sh-5.2# touch /tmp/empty_env
1sh-5.2# podman run --rm --env-file /tmp/empty_env 'ubuntu:24.04' env
2PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
3container=podman
4HOME=/root
5HOSTNAME=19ece5c9e052
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31678.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Prabhat1308, 0xB10C |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Concept ACK
I want to check if this helps #30852 (comment) when used with #31545 by not having to keep the base images around anymore.
11+set -o errexit -o pipefail -o xtrace
12
13 if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
14 # Export all env vars to avoid missing some.
15 # Though, exclude those with newlines to avoid parsing problems.
16 python3 -c 'import os; [print(f"{key}={value}") for key, value in os.environ.items() if "\n" not in value and "HOME" != key and "PATH" != key and "USER" != key]' | tee "/tmp/env-$USER-$CONTAINER_NAME"
For reference, the exclusion is still needed to avoid the error from #27739 (comment). Diff to reproduce the error:
0diff --git a/ci/test/02_run_container.sh b/ci/test/02_run_container.sh
1index 4181e68..168ab51 100755
2--- a/ci/test/02_run_container.sh
3+++ b/ci/test/02_run_container.sh
4@@ -12,7 +12,7 @@ set -o errexit -o pipefail -o xtrace
5 if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
6 # Export all env vars to avoid missing some.
7 # Though, exclude those with newlines to avoid parsing problems.
8- python3 -c 'import os; [print(f"{key}={value}") for key, value in os.environ.items() if "\n" not in value and "HOME" != key and "PATH" != key and "USER" != key]' | tee "/tmp/env-$USER-$CONTAINER_NAME"
9+ python3 -c 'import os; [print(f"{key}={value}") for key, value in os.environ.items() if "\n" not in value]' | tee "/tmp/env-$USER-$CONTAINER_NAME"
10
11 # Env vars during the build can not be changed. For example, a modified
12 # $MAKEJOBS is ignored in the build process. Use --cpuset-cpus as an
CMD: env -i HOME="$HOME" PATH="$PATH" USER="$USER" MAKEJOBS="-j$( nproc )" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
Also, set pipefail while touching the script.
I want to check if this helps #30852 (comment) when used with #31545 by not having to keep the base images around anymore. @0xB10C Are you waiting for me to rebase this once more? I’ve held back, because there is already one review, but I am happy to rebase.
I want to check if this helps #30852 (comment) when used with #31545 by not having to keep the base images around anymore.
@0xB10C Are you waiting for me to rebase this once more? I’ve held back, because there is already one review, but I am happy to rebase.
Thanks for the reminder. I played around with this a bit and it does indeed help:
When using #31545, the base image (e.g. docker:22.04) is stored as part of the docker build-cache. Previously, I needed to keep it around as docker:22.04 image explictily, as docker run doesn’t use the docker build-cache. Without docker run, there is no need to keep the image outside of the build cache.
My understanding is that #28138 was only needed for
DANGER_RUN_CI_ON_HOST. That’s why removing it here isn’t a regression to pre 28138, correct?
I do not know if they were ever needed to be set inside the CI pod, given that they fall back to the default anyway (see pull description). This may have just been added out of caution, as on the outside the pod, they are required sometimes to be set, see the docs:
https://github.com/bitcoin/bitcoin/blame/86528937e5c4da2e12c46085fc41e87ed759258e/ci/README.md#L46
Makes sense, thanks.
ACK fa952acdb6ef1b07b7927202d1373fa7479fd5e4