build: Check libevent minimum version in configure script #18676

hebasto commented at 8:12 PM on April 16, 2020: member

The non-pkg-config path is ignored as there is a hope to get rid of all of them in #18307.

As xenial has libevent 2.0.21 only, the default bionic Docker image is used in the "[no depends, only system libs, sanitizers: thread (TSan), no wallet]" CI test.

luke-jr commented at 8:14 PM on April 16, 2020: member

utACK

laanwj added the label Build system on Apr 16, 2020

laanwj commented at 8:15 PM on April 16, 2020: member

ACK 4a8e07913855da676a0eb13a4dc13d4b9cb77cfb

DrahtBot commented at 10:42 PM on April 16, 2020: member

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

Conflicts

No conflicts as of last run.

in ci/test/00_setup_env_native_tsan.sh:10 in 4a8e079138 outdated

   6 | @@ -7,7 +7,6 @@
   7 |  export LC_ALL=C.UTF-8
   8 |  
   9 |  export CONTAINER_NAME=ci_native_tsan
  10 | -export DOCKER_NAME_TAG=ubuntu:16.04

MarcoFalke commented at 10:55 PM on April 16, 2020:

Please update the travis yaml as well

MarcoFalke commented at 10:56 PM on April 16, 2020:

Also the workaround in line 18 can be removed

MarcoFalke commented at 10:57 PM on April 16, 2020:

Also release notes are needed to say that xenial is no longer supported with system libs

hebasto commented at 11:08 PM on April 16, 2020:

Also release notes are needed to say that xenial is no longer supported with system libs

In which way: wiki or pr?

MarcoFalke commented at 11:10 PM on April 16, 2020:

Building and running Bitcoin Core on xenial works fine today on master and 0.20.0. Your pull is changing that.

hebasto commented at 11:13 PM on April 16, 2020:

I mean: in which way add this info to release notes? :)

MarcoFalke commented at 11:16 PM on April 16, 2020:

I'd speak against backporting this to 0.20.0, so it should go into the release notes for 0.21.0

laanwj commented at 10:20 AM on April 17, 2020: member

If building out of the box on xenial is such a big deal maybe check for 2.0.21 instead, but make sure that Travis also compiles the fuzzing binaries on xenial.

hebasto commented at 10:23 AM on April 17, 2020: member

@laanwj I failed to find the reason why 2.0.22 is the minimal version. Could you shed a light?

laanwj commented at 10:26 AM on April 17, 2020: member

It has been the minimal version ever since libevent was introduced into bitcoin core, it was the version I was using at the time (yes, that long ago).

laanwj commented at 10:29 AM on April 17, 2020: member

If you want to change the minimum version to 2.0.21 that's really fine with me. I do vaguely remember newer versions fixed some issues (such as file descriptor leaks?), but, it will more or less work and I don't know which version fixed what anyhow.

hebasto force-pushed on Apr 17, 2020

build: Set libevent minimum version to 2.0.21 b68e717967

hebasto force-pushed on Apr 17, 2020

hebasto marked this as a draft on Apr 17, 2020

theStack commented at 12:31 PM on April 17, 2020: member

To give some background: I brought up the topic about checking minimum libevent version in IRC because a fuzz test (http_request) using internal libevent functions caused a linking error on my system. I then realized that I was running libevent 2.0.21, when the minimum required version according to doc/dependencies.md is 2.0.22, so I thought a check would make sense.

Unfortunately with 2.0.22, the fuzz test wouldn't compile either, a workaround is needed for all versions < 2.1.1 see PR #18682

Nevertheless checking minimum required version is a good thing, so Concept ACK.

hebasto commented at 12:32 PM on April 17, 2020: member

w.r.t. #18682 I dropped my own workaround in configure.ac. Hence, this PR status updated from "Draft" to "Ready for review".

hebasto marked this as ready for review on Apr 17, 2020

theStack commented at 12:38 PM on April 17, 2020: member

w.r.t. #18682 I dropped my own workaround in configure.ac. Hence, this PR status updated from "Draft" to "Ready for review".

Just out of curiosity, how would a workaround in configure.ac have looked like? There is not much you can do besides of enforcing a minimum version (that would need to be 2.1.1), no?

hebasto commented at 12:54 PM on April 17, 2020: member

@theStack

w.r.t. #18682 I dropped my own workaround in configure.ac. Hence, this PR status updated from "Draft" to "Ready for review".

Just out of curiosity, how would a workaround in configure.ac have looked like? There is not much you can do besides of enforcing a minimum version (that would need to be 2.1.1), no?

smth. like #18358 with a test-based approach.

MarcoFalke added the label Needs Guix build on Apr 17, 2020

luke-jr commented at 5:15 PM on April 17, 2020: member

Looks like Alpine, Arch, FreeBSD, and Ubuntu are at 2.1.11, while CentOS, Debian, Fedora, Gentoo, and openSUSE ship 2.1.8.

Unfortunately, Devuan only ships 2.0.21 still...

I don't see anything in 2.0.21..2.0.22 that should break the build, so I suggest we set the minimum enforced to 2.0.21 for now, require 2.1.8 for the fuzzer to build, and consider bumping the project-wide minimum to 2.1.8 sometime in the near future.

MarcoFalke commented at 5:20 PM on April 17, 2020: member

so I suggest we set the minimum enforced to 2.0.21 for now @luke-jr This is what the pull is doing here

MarcoFalke commented at 5:21 PM on April 17, 2020: member

Concept ACK

luke-jr commented at 7:12 PM on April 17, 2020: member

re-utACK

theStack commented at 1:02 PM on April 19, 2020: member

utACK https://github.com/bitcoin/bitcoin/pull/18676/commits/b68e71796792a9da9daa0a4e759d284d15595230 If anyone knows a way how to test this, I'd be interested. Just tried out with Ubuntu Precise Pangolin (via docker image ubuntu:12.04) that uses libevent 2.0.16, but then autogen.sh is failing as the Autoconf version is too old (<2.69).

hebasto commented at 1:12 PM on April 19, 2020: member

@theStack

If anyone knows a way how to test this, I'd be interested.

For example, on bionic with libevent 2.1.8 on testing purpose you could apply the following patch:

--- a/configure.ac
+++ b/configure.ac
@@ -1265,9 +1265,9 @@ if test x$use_pkgconfig = xyes; then
         BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
       fi
       if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
-        PKG_CHECK_MODULES([EVENT], [libevent >= 2.0.21], [use_libevent=yes], [AC_MSG_ERROR(libevent version 2.0.21 or greater not found.)])
+        PKG_CHECK_MODULES([EVENT], [libevent >= 2.1.9], [use_libevent=yes], [AC_MSG_ERROR(libevent version 2.1.9 or greater not found.)])
         if test x$TARGET_OS != xwindows; then
-          PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads >= 2.0.21],, [AC_MSG_ERROR(libevent_pthreads version 2.0.21 or greater not found.)])
+          PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads >= 2.1.9],, [AC_MSG_ERROR(libevent_pthreads version 2.1.9 or greater not found.)])
         fi
       fi

laanwj commented at 4:42 PM on April 20, 2020: member

ACK b68e71796792a9da9daa0a4e759d284d15595230

laanwj added the label Needs backport (0.19) on Apr 20, 2020

laanwj added the label Needs backport (0.20) on Apr 20, 2020

laanwj merged this on Apr 20, 2020

laanwj closed this on Apr 20, 2020

MarcoFalke removed the label Needs Guix build on Apr 20, 2020

sidhujag referenced this in commit 806c143ad0 on Apr 20, 2020

hebasto deleted the branch on Apr 20, 2020

fanquake referenced this in commit 1d1e3585fe on Apr 23, 2020

fanquake removed the label Needs backport (0.20) on Apr 23, 2020

laanwj referenced this in commit fb5b098598 on May 11, 2020

fanquake referenced this in commit e422f65aee on May 20, 2020

fanquake removed the label Needs backport (0.19) on May 20, 2020

MarcoFalke referenced this in commit 28a9df7d76 on Aug 11, 2020

backpacker69 referenced this in commit 16f8d8f1be on Mar 28, 2021

DrahtBot locked this on Feb 15, 2022