ci: Allow PIP_PACKAGES on centos

MarcoFalke commented at 10:22 am on October 3, 2022: member

This was added in 7fc5e865b93af59364e9c8bf75ec68b4decc7e5d but I can’t see a reason why this should be forbidden.

This is also needed for other changes (bumping the minimum python version).

MarcoFalke force-pushed on Oct 3, 2022

DrahtBot added the label Tests on Oct 3, 2022

in ci/test/00_setup_env_i686_centos.sh:13 in fa684bcde9 outdated

 8@@ -9,7 +9,8 @@ export LC_ALL=C.UTF-8
 9 export HOST=i686-pc-linux-gnu
10 export CONTAINER_NAME=ci_i686_centos
11 export DOCKER_NAME_TAG=quay.io/centos/centos:stream8
12-export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-zmq which patch lbzip2 xz procps-ng dash rsync coreutils bison"
13+export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-pip zeromq which patch lbzip2 xz procps-ng dash rsync coreutils bison"
14+export PIP_PACKAGES="pyzmq"

hebasto commented at 11:54 am on October 3, 2022:

How does this PR change the “32-bit + dash [gui] [CentOS 8]” CI task?

Asking because the interface_zmq.py functional test passes on the current master branch.

hebasto commented at 11:57 am on October 3, 2022:

From https://github.com/bitcoin/bitcoin/tree/master/test:

The ZMQ functional test requires a python ZMQ library. To install it:

on Unix, run sudo apt-get install python3-zmq

on mac OS, run pip3 install pyzmq

MarcoFalke commented at 11:58 am on October 3, 2022:

How does this PR change the “32-bit + dash [gui] [CentOS 8]” CI task?

It shouldn’t. All it does is replace the python3-zmq system package, which has wrapped the pyzmq from pip and zeromq. It is replaced by an explicit installation of both “sub”-packages.

hebasto commented at 11:59 am on October 3, 2022:

Then, probably, the relevant docs should be adjusted as well?

hebasto commented at 12:01 pm on October 3, 2022:

All it does is replace the python3-zmq system package…

Other tasks still use python3-zmq.

MarcoFalke commented at 12:08 pm on October 3, 2022:

apt-get install python3-zmq

centos is not using apt-get, so any doc update seems unrelated. If your system provides python3-zmq you can use it, however, you can also use pip3 install pyzmq, which should work everywhere.

hebasto commented at 12:17 pm on October 3, 2022:

centos is not using apt-get, so any doc update seems unrelated

That is why docs need to be updated like that or similar:

0The ZMQ functional test requires a python ZMQ library. To install it:
1- on Debian/Ubuntu, run `sudo apt-get install python3-zmq`
2- on other systems, run `pip3 install pyzmq`

But it is unrelated to this PR CI changes themselves.

All it does is replace the python3-zmq system package, which has wrapped the pyzmq from pip and zeromq. It is replaced by an explicit installation of both “sub”-packages.

And what are benefits of that?

MarcoFalke commented at 12:26 pm on October 3, 2022:

And what are benefits of that?

It demonstrates that the goal of this pull request is achieved. Also, it is needed for other changes (bumping the minimum python version).

hebasto commented at 12:41 pm on October 3, 2022:

Also, it is needed for other changes (bumping the minimum python version).

Perhaps, mention this in the PR description?

MarcoFalke commented at 12:44 pm on October 3, 2022:

Perhaps, mention this in the PR description?

Thx, done

in ci/test/04_install.sh:84 in fa684bcde9 outdated

81+    # shellcheck disable=SC2086
82+    IN_GETOPT_BIN="/usr/local/opt/gnu-getopt/bin/getopt" ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES
83+  else
84     # shellcheck disable=SC2086
85-    ${CI_RETRY_EXE} pip3 install --user $PIP_PACKAGES
86+    ${CI_RETRY_EXE} CI_EXEC pip3 install --user $PIP_PACKAGES

MarcoFalke commented at 12:17 pm on October 3, 2022:

Review note: Adding CI_EXEC is required, otherwise it would result in a failure.

To reproduce the failure:

0$ RESTART_CI_DOCKER_BEFORE_RUN=1 FILE_ENV="./ci/test/00_setup_env_i686_centos.sh" ./ci/test_run_all.sh
1...
2bash: line 1: pip3: command not found

MarcoFalke commented at 12:24 pm on October 3, 2022:

This also fixes a bug on historic commits. To reproduce the bug on the historic commits:

0$ git checkout 3c3bd9022026a75e15492ba9cf8bf3b72866b9a9~ && RESTART_CI_DOCKER_BEFORE_RUN=1 FILE_ENV="./ci/test/00_setup_env_i686_multiprocess.sh" ./ci/test_run_all.sh
1...
2bash: line 1: pip3: command not found

hebasto approved

hebasto commented at 12:41 pm on October 3, 2022: member

ACK fa684bcde97e7ed5de102429e1e656c6d091a547, I have reviewed the code and it looks OK, I agree it can be merged.

MarcoFalke commented at 12:47 pm on October 3, 2022: member

cc @fanquake (author of 7fc5e865b93af59364e9c8bf75ec68b4decc7e5d)

MarcoFalke requested review from fanquake on Oct 3, 2022

fanquake approved

fanquake commented at 12:59 pm on October 3, 2022: member

ACK fa684bcde97e7ed5de102429e1e656c6d091a547 - I see lief still being installed.

DrahtBot commented at 11:41 pm on October 3, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26226 (Bump minimum python version to 3.7 by MarcoFalke)
#25900 (ci: run docker wrapper with a non-root user by josibake)
#24798 ([POC] build: Hello Qt 6 by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ci: Remove unused package

Address feedback from https://github.com/bitcoin/bitcoin/pull/24561/files#r985719812

fac085a05c

ci: Allow PIP_PACKAGES on centos

This was added in 7fc5e865b93af59364e9c8bf75ec68b4decc7e5d but I can't
see a reason why this should be forbidden.

fa6054e952

in ci/test/00_setup_env_i686_centos.sh:12 in fa684bcde9 outdated

 8@@ -9,7 +9,8 @@ export LC_ALL=C.UTF-8
 9 export HOST=i686-pc-linux-gnu
10 export CONTAINER_NAME=ci_i686_centos
11 export DOCKER_NAME_TAG=quay.io/centos/centos:stream8
12-export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-zmq which patch lbzip2 xz procps-ng dash rsync coreutils bison"
13+export DOCKER_PACKAGES="gcc-c++ glibc-devel.x86_64 libstdc++-devel.x86_64 glibc-devel.i686 libstdc++-devel.i686 ccache libtool make git python3 python3-pip zeromq which patch lbzip2 xz procps-ng dash rsync coreutils bison"

MarcoFalke commented at 9:56 am on October 4, 2022:

Actually I think that zermomq shouldn’t be needed at all. depends should be statically linking the 32-bit lib into the binary already

MarcoFalke commented at 10:22 am on October 4, 2022:

(fixed in last force push)

MarcoFalke force-pushed on Oct 4, 2022

MarcoFalke commented at 10:52 am on October 4, 2022: member

For reference, interface_zmq.py:

on master macos: https://cirrus-ci.com/task/6731452615229440?logs=ci#L2918
on master centos: https://cirrus-ci.com/task/5464815220031488?logs=ci#L3322
here macos: https://cirrus-ci.com/task/5487962711392256?logs=ci#L2918
here centos: https://cirrus-ci.com/task/6121281408991232?logs=ci#L3303

hebasto approved

hebasto commented at 12:52 pm on October 4, 2022: member

re-ACK fa6054e952f4522b98dc89609033950a3cbfd06c

fanquake merged this on Oct 4, 2022

fanquake closed this on Oct 4, 2022

MarcoFalke deleted the branch on Oct 5, 2022

sidhujag referenced this in commit cb725b6690 on Oct 5, 2022

fanquake referenced this in commit 0a369b58d5 on Jan 12, 2023

fanquake referenced this in commit 28b9c2f6a6 on Jan 12, 2023

fanquake referenced this in commit d5ba043a6c on Jan 12, 2023

fanquake referenced this in commit a57248a814 on Jan 12, 2023

fanquake referenced this in commit 360ff52aa3 on Jan 12, 2023

MarcoFalke referenced this in commit 644c0304f5 on Jan 12, 2023

MarcoFalke referenced this in commit ce2a072ba8 on Jan 12, 2023

fanquake referenced this in commit 047cae3f80 on Jan 12, 2023

fanquake referenced this in commit fd70fecbe6 on Jan 12, 2023

fanquake referenced this in commit 46d99d5b81 on Jan 12, 2023

jarolrod referenced this in commit 4ee5dbe7fb on Jan 13, 2023

fanquake referenced this in commit c2ee70a89b on Jan 13, 2023

fanquake referenced this in commit 821b454abb on Jan 13, 2023

bitcoin locked this on Oct 5, 2023

ci: Allow PIP_PACKAGES on centos #26234

Conflicts