ci: Add missing set -e to 01_base

maflcko commented at 12:50 pm on May 24, 2023: member

Otherwise errors are silently ignored

DrahtBot commented at 12:50 pm on May 24, 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	hebasto, TheCharlatan

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

DrahtBot added the label Tests on May 24, 2023

maflcko commented at 1:03 pm on May 24, 2023: member

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.

fanquake requested review from TheCharlatan on May 26, 2023

TheCharlatan commented at 9:49 am on May 26, 2023: contributor

Still not sure what is going on here:

0MAKEJOBS="-j12" FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
1> ...
2> fatal: destination path '/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use' already exists and is not an empty directory.
3ls home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use
4> ls: cannot access 'home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use': No such file or directory

Command runs fine on master, so likely something that is now surfaced with set -e.

maflcko commented at 10:06 am on May 26, 2023: member

Can you replace set -e with set -ex and show the surrounding line?

maflcko commented at 10:08 am on May 26, 2023: member

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.

TheCharlatan commented at 10:39 am on May 26, 2023: contributor

Re #27739 (comment)

Can you replace set -e with set -ex and show the surrounding line?

00 upgraded, 0 newly installed, 0 to remove and 1 not upgraded.
1+ '[' -n '' ']'
2+ [[ '' == \t\r\u\e ]]
3+ [[ true == \t\r\u\e ]]
4+ git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use
5fatal: destination path '/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use' already exists and is not an empty directory.

Does it reproduce on your system? I tried removing any remnants of the image, but the problem persists.

maflcko commented at 10:43 am on May 26, 2023: member

Can you share the output where it should say “Skip base install”? Because it should never be called twice

maflcko commented at 10:44 am on May 26, 2023: member

+ [[ true == \t\r\u\e ]]

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.

TheCharlatan commented at 11:24 am on May 26, 2023: contributor

Re #27739 (comment)

Can you share the output where it should say “Skip base install”? Because it should never be called twice

ci_native_tidy_ccache ci_native_tidy_depends ci_native_tidy_previous_releases

Number of files: 979 (reg: 918, dir: 61) Number of created files: 973 (reg: 918, dir: 55) Number of deleted files: 0 Number of regular files transferred: 918 Total file size: 74.35M bytes Total transferred file size: 74.35M bytes Literal data: 74.35M bytes Matched data: 0 bytes File list size: 0 File list generation time: 0.001 seconds File list transfer time: 0.000 seconds Total bytes sent: 74.44M Total bytes received: 17.79K

sent 74.44M bytes received 17.79K bytes 148.91M bytes/sec total size is 74.35M speedup is 1.00

CFG_DONE=ci.base-install-done ++ git config –global ci.base-install-done
‘[’ ’’ == true ‘]’
‘[’ -n ’’ ‘]’
[[ ubuntu:lunar == centos ]]
‘[’ ’’ ‘!=’ no ‘]’
[[ -n ’’ ]]
retry – apt-get update Get:1 http://security.ubuntu.com/ubuntu lunar-security InRelease [109 kB] Hit:2 http://archive.ubuntu.com/ubuntu lunar InRelease Get:3 http://archive.ubuntu.com/ubuntu lunar-updates InRelease [109 kB] Get:4 http://archive.ubuntu.com/ubuntu lunar-backports InRelease [99.8 kB] Fetched 317 kB in 3s (123 kB/s) Reading package lists…
retry – bash -c ‘apt-get install –no-install-recommends –no-upgrade -y clang-16 libclang-16-dev llvm-16-dev libomp-16-dev clang-tidy-16 jq bear cmake libevent-dev libboost-dev libminiupnpc-dev libnatpmp-dev libzmq3-dev systemtap-sdt-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libqrencode-dev libsqlite3-dev libdb++-dev build-essential libtool autotools-dev automake pkg-config bsdmainutils curl ca-certificates ccache python3 rsync git procps bison’ Reading package lists… Building dependency tree… Reading state information… Skipping clang-16, it is already installed and upgrade is not set. Skipping libclang-16-dev, it is already installed and upgrade is not set. Skipping llvm-16-dev, it is already installed and upgrade is not set. Skipping libomp-16-dev, it is already installed and upgrade is not set. Skipping clang-tidy-16, it is already installed and upgrade is not set. Skipping jq, it is already installed and upgrade is not set. Skipping bear, it is already installed and upgrade is not set. Skipping cmake, it is already installed and upgrade is not set. Skipping libevent-dev, it is already installed and upgrade is not set. Skipping libboost-dev, it is already installed and upgrade is not set. Skipping libminiupnpc-dev, it is already installed and upgrade is not set. Skipping libnatpmp-dev, it is already installed and upgrade is not set. Skipping libzmq3-dev, it is already installed and upgrade is not set. Skipping systemtap-sdt-dev, it is already installed and upgrade is not set. Skipping libqt5gui5, it is already installed and upgrade is not set. Skipping libqt5core5a, it is already installed and upgrade is not set. Skipping libqt5dbus5, it is already installed and upgrade is not set. Skipping qttools5-dev, it is already installed and upgrade is not set. Skipping qttools5-dev-tools, it is already installed and upgrade is not set. Skipping libqrencode-dev, it is already installed and upgrade is not set. Skipping libsqlite3-dev, it is already installed and upgrade is not set. Skipping libdb++-dev, it is already installed and upgrade is not set. Skipping build-essential, it is already installed and upgrade is not set. Skipping libtool, it is already installed and upgrade is not set. Skipping autotools-dev, it is already installed and upgrade is not set. Skipping automake, it is already installed and upgrade is not set. Skipping pkg-config, it is already installed and upgrade is not set. Skipping bsdmainutils, it is already installed and upgrade is not set. Skipping curl, it is already installed and upgrade is not set. Skipping ca-certificates, it is already installed and upgrade is not set. Skipping ccache, it is already installed and upgrade is not set. Skipping python3, it is already installed and upgrade is not set. Skipping rsync, it is already installed and upgrade is not set. Skipping git, it is already installed and upgrade is not set. Skipping procps, it is already installed and upgrade is not set. Skipping bison, it is already installed and upgrade is not set. 0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded.
‘[’ -n ’’ ‘]’
[[ ’’ == \t\r\u\e ]]
[[ true == \t\r\u\e ]]
git clone –depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use fatal: destination path ‘/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use’ already exists and is not an empty directory.

maflcko commented at 11:51 am on May 26, 2023: member

Ok so git config --global ci.base-install-done evaluates to the empty string? Can you share the details where it is first set?

https://github.com/bitcoin/bitcoin/blob/fa534d780e0bb811b7b762cbd430e30f92dd7d7e/ci/test/01_base_install.sh#L81

(This should happen when building the image)

TheCharlatan commented at 12:34 pm on May 26, 2023: contributor

Re #27739 (comment)

Can you share the details where it is first set?

Here is the entire output I am willing to share (the rest above prints my env, which I don’t want to paste here)

Number of files: 979 (reg: 918, dir: 61) Number of created files: 973 (reg: 918, dir: 55) Number of deleted files: 0 Number of regular files transferred: 918 Total file size: 74.35M bytes Total transferred file size: 74.35M bytes Literal data: 74.35M bytes Matched data: 0 bytes File list size: 0 File list generation time: 0.001 seconds File list transfer time: 0.000 seconds Total bytes sent: 74.44M Total bytes received: 17.78K

sent 74.44M bytes received 17.78K bytes 148.91M bytes/sec total size is 74.35M speedup is 1.00 Hit:1 http://archive.ubuntu.com/ubuntu lunar InRelease Get:2 http://archive.ubuntu.com/ubuntu lunar-updates InRelease [109 kB] Get:3 http://archive.ubuntu.com/ubuntu lunar-backports InRelease [99.8 kB] Get:4 http://security.ubuntu.com/ubuntu lunar-security InRelease [109 kB] Fetched 317 kB in 1s (528 kB/s) Reading package lists… Reading package lists… Building dependency tree… Reading state information… Skipping clang-16, it is already installed and upgrade is not set. Skipping libclang-16-dev, it is already installed and upgrade is not set. Skipping llvm-16-dev, it is already installed and upgrade is not set. Skipping libomp-16-dev, it is already installed and upgrade is not set. Skipping clang-tidy-16, it is already installed and upgrade is not set. Skipping jq, it is already installed and upgrade is not set. Skipping bear, it is already installed and upgrade is not set. Skipping cmake, it is already installed and upgrade is not set. Skipping libevent-dev, it is already installed and upgrade is not set. Skipping libboost-dev, it is already installed and upgrade is not set. Skipping libminiupnpc-dev, it is already installed and upgrade is not set. Skipping libnatpmp-dev, it is already installed and upgrade is not set. Skipping libzmq3-dev, it is already installed and upgrade is not set. Skipping systemtap-sdt-dev, it is already installed and upgrade is not set. Skipping libqt5gui5, it is already installed and upgrade is not set. Skipping libqt5core5a, it is already installed and upgrade is not set. Skipping libqt5dbus5, it is already installed and upgrade is not set. Skipping qttools5-dev, it is already installed and upgrade is not set. Skipping qttools5-dev-tools, it is already installed and upgrade is not set. Skipping libqrencode-dev, it is already installed and upgrade is not set. Skipping libsqlite3-dev, it is already installed and upgrade is not set. Skipping libdb++-dev, it is already installed and upgrade is not set. Skipping build-essential, it is already installed and upgrade is not set. Skipping libtool, it is already installed and upgrade is not set. Skipping autotools-dev, it is already installed and upgrade is not set. Skipping automake, it is already installed and upgrade is not set. Skipping pkg-config, it is already installed and upgrade is not set. Skipping bsdmainutils, it is already installed and upgrade is not set. Skipping curl, it is already installed and upgrade is not set. Skipping ca-certificates, it is already installed and upgrade is not set. Skipping ccache, it is already installed and upgrade is not set. Skipping python3, it is already installed and upgrade is not set. Skipping rsync, it is already installed and upgrade is not set. Skipping git, it is already installed and upgrade is not set. Skipping procps, it is already installed and upgrade is not set. Skipping bison, it is already installed and upgrade is not set. 0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded. fatal: destination path ‘/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use’ already exists and is not an empty directory.

From what I am seeing this is in the image building stage.

maflcko commented at 12:52 pm on May 26, 2023: member

Well, it isn’t building the image, just printing CACHED. I’d need the output from when it was built of the step that ran base_install:

0CACHED [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"]

maflcko commented at 12:53 pm on May 26, 2023: member

the rest above prints my env, which I don’t want to paste here

If you can reproduce on a fresh install of your OS, that’d help too

TheCharlatan commented at 1:18 pm on May 26, 2023: contributor

Re #27739 (comment)

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:

Number of files: 979 (reg: 918, dir: 61) Number of created files: 975 (reg: 918, dir: 57) Number of deleted files: 0 Number of regular files transferred: 918 Total file size: 74.35M bytes Total transferred file size: 74.35M bytes Literal data: 74.35M bytes Matched data: 0 bytes File list size: 0 File list generation time: 0.001 seconds File list transfer time: 0.000 seconds Total bytes sent: 74.44M Total bytes received: 17.79K

sent 74.44M bytes received 17.79K bytes 148.91M bytes/sec total size is 74.35M speedup is 1.00 Hit:1 http://security.ubuntu.com/ubuntu lunar-security InRelease Hit:2 http://archive.ubuntu.com/ubuntu lunar InRelease Hit:3 http://archive.ubuntu.com/ubuntu lunar-updates InRelease Hit:4 http://archive.ubuntu.com/ubuntu lunar-backports InRelease Reading package lists… Reading package lists… Building dependency tree… Reading state information… Skipping clang-16, it is already installed and upgrade is not set. Skipping libclang-16-dev, it is already installed and upgrade is not set. Skipping llvm-16-dev, it is already installed and upgrade is not set. Skipping libomp-16-dev, it is already installed and upgrade is not set. Skipping clang-tidy-16, it is already installed and upgrade is not set. Skipping jq, it is already installed and upgrade is not set. Skipping bear, it is already installed and upgrade is not set. Skipping cmake, it is already installed and upgrade is not set. Skipping libevent-dev, it is already installed and upgrade is not set. Skipping libboost-dev, it is already installed and upgrade is not set. Skipping libminiupnpc-dev, it is already installed and upgrade is not set. Skipping libnatpmp-dev, it is already installed and upgrade is not set. Skipping libzmq3-dev, it is already installed and upgrade is not set. Skipping systemtap-sdt-dev, it is already installed and upgrade is not set. Skipping libqt5gui5, it is already installed and upgrade is not set. Skipping libqt5core5a, it is already installed and upgrade is not set. Skipping libqt5dbus5, it is already installed and upgrade is not set. Skipping qttools5-dev, it is already installed and upgrade is not set. Skipping qttools5-dev-tools, it is already installed and upgrade is not set. Skipping libqrencode-dev, it is already installed and upgrade is not set. Skipping libsqlite3-dev, it is already installed and upgrade is not set. Skipping libdb++-dev, it is already installed and upgrade is not set. Skipping build-essential, it is already installed and upgrade is not set. Skipping libtool, it is already installed and upgrade is not set. Skipping autotools-dev, it is already installed and upgrade is not set. Skipping automake, it is already installed and upgrade is not set. Skipping pkg-config, it is already installed and upgrade is not set. Skipping bsdmainutils, it is already installed and upgrade is not set. Skipping curl, it is already installed and upgrade is not set. Skipping ca-certificates, it is already installed and upgrade is not set. Skipping ccache, it is already installed and upgrade is not set. Skipping python3, it is already installed and upgrade is not set. Skipping rsync, it is already installed and upgrade is not set. Skipping git, it is already installed and upgrade is not set. Skipping procps, it is already installed and upgrade is not set. Skipping bison, it is already installed and upgrade is not set. 0 upgraded, 0 newly installed, 0 to remove and 1 not upgraded. fatal: destination path ‘/home/drgrid/bitcoin/ci/scratch/iwyu/include-what-you-use’ already exists and is not an empty directory.

maflcko referenced this in commit fa0d36ffb5 on May 26, 2023

maflcko force-pushed on May 26, 2023

maflcko commented at 3:03 pm on May 26, 2023: member

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?

in ci/test/01_base_install.sh:9 in fa0d36ffb5 outdated

5@@ -6,6 +6,8 @@
6 
7 export LC_ALL=C.UTF-8
8 
9+set -ex

hebasto commented at 4:30 pm on May 26, 2023:

Mention -x option in the PR description?

maflcko commented at 7:19 am on May 29, 2023:

thx, mentioned in commit description

hebasto commented at 4:35 pm on May 26, 2023: member

Otherwise errors are silently ignored

Should we do the same in ci/test/00_setup_env.sh?

https://cirrus-ci.com/task/5206141060251648:

0./ci/test/00_setup_env.sh: line 33: /ci_base_install/depends/config.guess: No such file or directory

in ci/test/00_setup_env.sh:47 in fa0d36ffb5 outdated

43@@ -44,8 +44,6 @@ export TEST_RUNNER_TIMEOUT_FACTOR=${TEST_RUNNER_TIMEOUT_FACTOR:-40}
44 export TEST_RUNNER_ENV=${TEST_RUNNER_ENV:-}
45 export RUN_FUZZ_TESTS=${RUN_FUZZ_TESTS:-false}
46 
47-export CONTAINER_NAME=${CONTAINER_NAME:-ci_unnamed}

hebasto commented at 4:55 pm on May 26, 2023:

FILE_ENV="./ci/test/00_setup_env_mac_native_arm64.sh" ./ci/test_run_all.sh fails locally for me now with the following error:

0ERROR: invalid tag "": repository name must have at least one component

Also https://github.com/bitcoin/bitcoin/blob/fa0d36ffb5c8348598998eeb3ea6a3ee93bd6350/ci/test/04_install.sh#L37

maflcko commented at 7:12 am on May 29, 2023:

I don’t think running macOS on Linux is in scope for the CI system.

maflcko commented at 7:21 am on May 29, 2023:

Also, this was never supported, so seems unrelated to the changes here?

hebasto commented at 8:13 am on May 29, 2023:

You are right.

ci: Add missing set -e to 01_base_install.sh

Also, set -x for easier debugging.

Also, do the same for ci/test/00_setup_env.sh

fa7a87bc7c

ci: Remove "default" test env

It is unclear what the point is of maintaining a "default", the meaning
of which is unclear.

aaaa432603

ci: Avoid leaking HOME var into CI pod

This will lead to a duplicate install, see https://github.com/bitcoin/bitcoin/pull/27739#issuecomment-1564122573

fa12558d21

maflcko force-pushed on May 29, 2023

hebasto approved

hebasto commented at 8:18 am on May 29, 2023: member

ACK fa12558d21aa03c22407a1458a0345d8a7ab6a4b

TheCharlatan commented at 8:38 am on May 29, 2023: contributor

Re #27739 (comment)

Thanks, the last commit should fix your bug.

Thank you for digging into this, seems like everything is working as it should now. ACK fa12558

DrahtBot removed review request from TheCharlatan on May 29, 2023

TheCharlatan approved

fanquake merged this on May 29, 2023

fanquake closed this on May 29, 2023

maflcko deleted the branch on May 29, 2023

sidhujag referenced this in commit 237f65db60 on May 29, 2023

hebasto commented at 10:57 am on June 14, 2023: member

+ [[ true == \t\r\u\e ]]

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.

There are two options:

remove quoting:

 0--- a/ci/test/01_base_install.sh
 1+++ b/ci/test/01_base_install.sh
 2@@ -70,7 +70,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
 3   ninja -C "${BASE_SCRATCH_DIR}"/msan/cxx_build/ "$MAKEJOBS"
 4 fi
 5 
 6-if [[ "${RUN_TIDY}" == "true" ]]; then
 7+if [[ $RUN_TIDY == true ]]; then
 8   git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
 9   cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
10   make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"

or

use Bash builtin [ command:

 0--- a/ci/test/01_base_install.sh
 1+++ b/ci/test/01_base_install.sh
 2@@ -70,7 +70,7 @@ if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
 3   ninja -C "${BASE_SCRATCH_DIR}"/msan/cxx_build/ "$MAKEJOBS"
 4 fi
 5 
 6-if [[ "${RUN_TIDY}" == "true" ]]; then
 7+if [ "${RUN_TIDY}" = "true" ]; then
 8   git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 "${DIR_IWYU}"/include-what-you-use
 9   cmake -B "${DIR_IWYU}"/build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S "${DIR_IWYU}"/include-what-you-use
10   make -C "${DIR_IWYU}"/build/ install "$MAKEJOBS"

I’d prefer the former option because set -x apply additional quoting for [ and ], which makes the output less readable.

Anyway, the current usage of [...] and [[...]] is quite inconsistent in the CI scripts.

fanquake referenced this in commit 492257019d on Aug 9, 2023

fanquake referenced this in commit cd5d2f5f09 on Aug 24, 2023

Frank-GER referenced this in commit 0ad7e12592 on Sep 8, 2023

janus referenced this in commit 248c77ecbb on Sep 11, 2023

bitcoin locked this on Jun 13, 2024

ci: Add missing set -e to 01_base_install.sh #27739

Reviews