ci: Add missing set -e to 01_base_install.sh #27739
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
Looks like it is working: https://cirrus-ci.com/task/4766589006905344?logs=build#L2788 Added another unrelated commit to re-trigger CI and to give more yummy review to reviewers. |
|
Still not sure what is going on here: Command runs fine on master, so likely something that is now surfaced with |
|
Can you replace |
|
I can't see how this can happen. The error doesn't happen in base_install, from what I can tell, so it seems unrelated to the changes here. However, it passing on master makes me puzzle. |
Does it reproduce on your system? I tried removing any remnants of the image, but the problem persists. |
|
Can you share the output where it should say "Skip base install"? Because it should never be called twice |
Unrelated: Anyone know why it prints "true" as "\t\r\u\e"? Maybe we should remove bash, but I am not sure if there is an alternative. |
Details
ci_native_tidy_ccache Number of files: 979 (reg: 918, dir: 61) sent 74.44M bytes received 17.79K bytes 148.91M bytes/sec
|
|
Ok so bitcoin/ci/test/01_base_install.sh Line 81 in fa534d7 (This should happen when building the image) |
Here is the entire output I am willing to share (the rest above prints my env, which I don't want to paste here)
Creating ubuntu:lunar container to run in
[+] Building 1.0s (9/9) FINISHED
=> [internal] load build definition from test_imagefile 0.0s
=> => transferring dockerfile: 42B 0.0s
=> [internal] load .dockerignore 0.0s
=> => transferring context: 2B 0.0s
=> [internal] load metadata for docker.io/library/ubuntu:lunar 0.9s
=> [internal] load build context 0.0s
=> => transferring context: 257B 0.0s
=> [1/4] FROM docker.io/library/ubuntu:lunar@sha256:db2764b64a490a9ce44a7ce0c7ba7cc6d0472c8c44684960dbca7a06525eb597 0.0s
=> CACHED [2/4] COPY ./ci/retry/retry /usr/bin/retry 0.0s
=> CACHED [3/4] COPY ./ci/test/00_setup_env.sh ././ci/test/00_setup_env_native_tidy.sh ./ci/test/01_base_install.sh /ci_base_install/ci/test/ 0.0s
=> CACHED [4/4] RUN ["bash", "-c", "cd /ci_base_install/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh"] 0.0s
=> exporting to image 0.0s
=> => exporting layers 0.0s
=> => writing image sha256:e30eaeeb008ee7d59ed41ca8ddbd92bc95d93aadfc64b90daff6e00198c85d99 0.0s
=> => naming to docker.io/library/ci_native_tidy 0.0s
ci_native_tidy_ccache
ci_native_tidy_depends
ci_native_tidy_previous_releases
Number of files: 979 (reg: 918, dir: 61) sent 74.44M bytes received 17.78K bytes 148.91M bytes/sec From what I am seeing this is in the image building stage. |
|
Well, it isn't building the image, just printing |
If you can reproduce on a fresh install of your OS, that'd help too |
Not a fresh install, but another OS where I have not run this before:
MAKEJOBS="-j12" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
Setting specific values in env
Fallback to default values in env (if not yet set)
...
CONTAINER_NAME=ci_native_tidy
DEPENDS_DIR=/home/drgrid/bitcoin/depends
TERM=xterm-256color
SHELL=/usr/bin/zsh
BITCOIN_CONFIG=CC=clang-16 CXX=clang++-16 --with-incompatible-bdb --disable-hardening CFLAGS='-O0 -g0' CXXFLAGS='-O0 -g0 -I/usr/lib/llvm-16/lib/clang/16/include'
SDK_URL=https://bitcoincore.org/depends-sources/sdks
SHLVL=2
LC_TELEPHONE=de_CH.UTF-8
LOGNAME=drgrid
RUN_TIDY=true
XDG_RUNTIME_DIR=/run/user/1000
CI_IMAGE_NAME_TAG=ubuntu:lunar
PATH=/home/drgrid/bitcoin/ci/retry:/home/drgrid/.cargo/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/drgrid/.rvm/bin
LC_IDENTIFICATION=de_CH.UTF-8
BOOST_TEST_RANDOM=1
CCACHE_TEMPDIR=/tmp/.ccache-temp
TEST_RUNNER_ENV=
BASE_OUTDIR=/home/drgrid/bitcoin/ci/scratch/out/x86_64-pc-linux-gnu
PREVIOUS_RELEASES_DIR=/home/drgrid/bitcoin/releases/x86_64-pc-linux-gnu
NO_DEPENDS=1
_=/usr/bin/python3
Creating ubuntu:lunar container to run in
[+] Building 130.9s (9/9) FINISHED
=> [internal] load build definition from test_imagefile 0.1s
=> => transferring dockerfile: 404B 0.0s
=> [internal] load .dockerignore 0.1s
=> => transferring context: 2B 0.0s
=> [internal] load metadata for docker.io/library/ubuntu:lunar 1.5s
=> [1/4] FROM docker.io/library/ubuntu:lunar@sha256:db2764b64a490a9ce44a7ce0c7ba7cc6d0472c8c44684960dbca7a06525eb597 1.3s
=> => resolve docker.io/library/ubuntu:lunar@sha256:db2764b64a490a9ce44a7ce0c7ba7cc6d0472c8c44684960dbca7a06525eb597 0.0s
=> => sha256:db2764b64a490a9ce44a7ce0c7ba7cc6d0472c8c44684960dbca7a06525eb597 1.13kB / 1.13kB 0.0s
=> => sha256:ce7f6664be1081be78dfcf319cb11d7bceeef17b32df373282a66fe940e47f6d 424B / 424B 0.0s
=> => sha256:0120ea4307c597784895e9f539a67d88239d2c71ebfa830195fa2a650e4f3b5d 2.30kB / 2.30kB 0.0s
=> => sha256:cc1fe050fbd8eee8b0e2ef684c783d2c269fb7f61a0df0dee4639340bf612925 26.83MB / 26.83MB 0.7s
=> => extracting sha256:cc1fe050fbd8eee8b0e2ef684c783d2c269fb7f61a0df0dee4639340bf612925 0.5s
=> [internal] load build context 0.0s
=> => transferring context: 12.78kB 0.0s
=> [2/4] COPY ./ci/retry/retry /usr/bin/retry 0.1s
=> [3/4] COPY ./ci/test/00_setup_env.sh ././ci/test/00_setup_env_native_tidy.sh ./ci/test/01_base_install.sh /ci_base_install/ci/test/ 0.2s
=> [4/4] RUN ["bash", "-c", "cd /ci_base_install/ && set -o errexit && source ./ci/test/00_setup_env.sh && ./ci/test/01_base_install.sh"] 117.2s
=> exporting to image 10.4s
=> => exporting layers 10.4s
=> => writing image sha256:345b719a8a0223b71ba2bb051960ff42e89017ae86b39f4791ebdb771611a3a6 0.0s
=> => naming to docker.io/library/ci_native_tidy 0.0s
ci_native_tidy_ccache
ci_native_tidy_depends
ci_native_tidy_previous_releases
Number of files: 979 (reg: 918, dir: 61) sent 74.44M bytes received 17.79K bytes 148.91M bytes/sec |
This will lead to a duplicate install, see bitcoin#27739 (comment)
maflcko
force-pushed
the
2305-ci-bash-sob-
branch
from
fa534d7
to
fa0d36f
Compare
|
Thanks, the last commit should fix your bug. I wonder if env's like USER(NAME) and PATH should also be excluded, but this can be done later? |
There was a problem hiding this comment.
Otherwise errors are silently ignored
Should we do the same in ci/test/00_setup_env.sh?
https://cirrus-ci.com/task/5206141060251648:
./ci/test/00_setup_env.sh: line 33: /ci_base_install/depends/config.guess: No such file or directory
Also, set -x for easier debugging. Also, do the same for ci/test/00_setup_env.sh
It is unclear what the point is of maintaining a "default", the meaning of which is unclear.
This will lead to a duplicate install, see bitcoin#27739 (comment)
maflcko
force-pushed
the
2305-ci-bash-sob-
branch
from
fa0d36f
to
fa12558
Compare
Thank you for digging into this, seems like everything is working as it should now. |
fa12558 ci: Avoid leaking HOME var into CI pod (MarcoFalke) aaaa432 ci: Remove "default" test env (MarcoFalke) fa7a87b ci: Add missing set -e to 01_base_install.sh (MarcoFalke) Pull request description: Otherwise errors are silently ignored ACKs for top commit: TheCharlatan: ACK [fa12558](bitcoin@fa12558) hebasto: ACK fa12558 Tree-SHA512: dbf3f16302c83973b78f3a5e7793090bc9ac44fdf20d51a26b30a99a97369971661e9aed1cd810d80d49d60009651ca0a8aeb2bdc24198a143bf4fff0ec89901
There are two options:
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -70,7 +70,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
ninja -C "${BASE_SCRATCH_DIR}"/msan/cxx_build/ "$MAKEJOBS"
fi
-if [[ "${RUN_TIDY}" == "true" ]]; then
+if [[ $RUN_TIDY == true ]]; then
git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"or
--- a/ci/test/01_base_install.sh
+++ b/ci/test/01_base_install.sh
@@ -70,7 +70,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
ninja -C "${BASE_SCRATCH_DIR}"/msan/cxx_build/ "$MAKEJOBS"
fi
-if [[ "${RUN_TIDY}" == "true" ]]; then
+if [ "${RUN_TIDY}" = "true" ]; then
git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"I'd prefer the former option because Anyway, the current usage of |
fad0b67 ci: Use qemu-user through container engine (MarcoFalke) Pull request description: Currently the CI containers always run on the host architecture, and only wrap `bitcoind` into `qemu-user` when needed. This has many issues: * The `i386` tasks can not be run on non-x86 hosts. * `config.guess` isn't present when building the CI image, which is fine. But it prints a warning, see bitcoin/bitcoin#27739 (review) * The python tests are run on the host architecture, making it harder to find architecture specific bugs. See for example bitcoin/bitcoin#27529 (comment) * All modern container engines support automatic dispatch to qemu-user, so it seems redundant to re-invent the wheel. Fix all issues by: * removing `HOST` from `ci/test/00_setup_env.sh`. * removing `QEMU_USER_CMD` and `ci/test/wrap-qemu.sh`. * removing `DPKG_ADD_ARCH` where possible, and pruning `PACKAGES` where possible. * specifying the architecture in `CI_IMAGE_NAME_TAG` to be used by the container engine. ACKs for top commit: fanquake: ACK fad0b67 - this seems ok to me, and removes complexity from our CI system. Tree-SHA512: 85e79f9f570e292d70a629d112fd4a6e6217d96226a1b665ed13485f616d84720ad2126b7d4b22fc603049f72fa7f2163b56a6bc276319fcd8b0496304ea4157
fa15f7e ci: Remove no longer applicable section (MarcoFalke) fa378be ci: Start with clean env (MarcoFalke) fa8c250 ci: Limit scope of some env vars (MarcoFalke) Pull request description: Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in bitcoin/bitcoin#27739 (comment) ACKs for top commit: dergoegge: utACK fa15f7e Tree-SHA512: 716b264217557b6524dab92d5a2a8d61ecb982dff475bd0cf5a763070b4c5916cd5995e764eb5d67d9cf2428c29d5fc2f42b32941b54c7c3053123ce448171e5
fa15f7e ci: Remove no longer applicable section (MarcoFalke) fa378be ci: Start with clean env (MarcoFalke) fa8c250 ci: Limit scope of some env vars (MarcoFalke) Pull request description: Starting with a clean `env` should help to avoid non-determinism, such as the one fixed in bitcoin#27739 (comment) ACKs for top commit: dergoegge: utACK fa15f7e Tree-SHA512: 716b264217557b6524dab92d5a2a8d61ecb982dff475bd0cf5a763070b4c5916cd5995e764eb5d67d9cf2428c29d5fc2f42b32941b54c7c3053123ce448171e5
This will lead to a duplicate install, see bitcoin/bitcoin#27739 (comment)
Otherwise errors are silently ignored