Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: bump libevent to 2.1.11 in depends #17008

Merged
merged 1 commit into from Nov 20, 2019
Merged

Conversation

@stefanwouldgo
Copy link
Contributor

stefanwouldgo commented Oct 1, 2019

this doesn't need patches on Android anymore like 2.1.8 did.

@laanwj laanwj changed the title bump libevent to 2.1.11 in depends build: bump libevent to 2.1.11 in depends Oct 1, 2019
@dongcarl

This comment has been minimized.

Copy link
Contributor

dongcarl commented Oct 1, 2019

I don't know about the Android part, but I did confirm that the ./configure --help outputs are identical modulo the version number.

Copy link
Member

fanquake left a comment

Can you mention which Android patches are no-longer required? As if we don't actually want to bump libevent, maybe we could move them into depends.

2.1.11 changelog is available here.

This PR would also need to update the libevent entry in doc/dependencies.md.

Diff of the configure output comparing master (1f40a91) and this PR (51794d9) on macOS:

--- master.txt
+++ pr.txt
@@ -129,14 +129,15 @@
 checking for WIN32... no
+checking for MIDIPIX... no
 checking for CYGWIN... no
@@ -163,14 +164,17 @@
 checking netinet/tcp.h presence... yes
 checking for netinet/tcp.h... yes
+checking sys/un.h usability... yes
+checking sys/un.h presence... yes
+checking for sys/un.h... yes
 checking poll.h usability... yes
 checking poll.h presence... yes
@@ -243,14 +247,15 @@
 checking for arc4random_buf... yes
+checking for arc4random_addrandom... yes
 checking for eventfd... no
@@ -285,15 +290,14 @@
 checking for working kqueue... yes
 checking for epoll_ctl... no
-checking waitpid support WNOWAIT... no
 checking for port_create... no
 checking for pid_t... yes
@@ -303,28 +307,31 @@
 checking size of void *... 8
 checking size of off_t... 8
+checking size of time_t... 8
 checking for struct in6_addr... yes
 checking for struct sockaddr_in6... yes
+checking for struct sockaddr_un... yes
 checking for sa_family_t... yes
 checking for struct addrinfo... yes
 checking for struct sockaddr_storage... yes
 checking for struct in6_addr.s6_addr32... no
 checking for struct in6_addr.s6_addr16... no
 checking for struct sockaddr_in.sin_len... yes
 checking for struct sockaddr_in6.sin6_len... yes
 checking for struct sockaddr_storage.ss_family... yes
 checking for struct sockaddr_storage.__ss_family... no
-checking for struct so_linger... no
+checking for struct linger... yes
 checking for socklen_t... yes
 checking whether our compiler supports __func__... yes
+checking whether our compiler supports __FUNCTION__... yes
 checking for the pthreads library -lpthreads... no
 checking whether pthreads work without any flags... yes
 checking for joinable pthread attribute... PTHREAD_CREATE_JOINABLE
 checking if more special flags are required for pthreads... -D_THREAD_SAFE
 checking size of pthread_t... 8
 checking that generated files are newer than configure... done
 configure: creating ./config.status

As per the release notes bufferevent_ssl.h is no longer installed. I have a depends diff of master vs this PR here.

Which operating systems have you tested building / running Bitcoin Core on with the new libevent?

depends/packages/libevent.mk Show resolved Hide resolved
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 2, 2019

+checking for struct sockaddr_un... yes

That looks vaguely promising. Is there better UNIX socket support ?
Seems so: libevent/libevent@737d1be
Can't find anything similar for the HTTP client though.

@stefanwouldgo

This comment has been minimized.

Copy link
Contributor Author

stefanwouldgo commented Oct 2, 2019

Can you mention which Android patches are no-longer required? As if we don't actually want to bump libevent, maybe we could move them into depends.

This patch from @greenaddress for missing arc4random_addrandom. It is now handled in upstream.

This PR would also need to update the libevent entry in doc/dependencies.md.

Good point. Done.

Which operating systems have you tested building / running Bitcoin Core on with the new libevent?

termux on Android-5 (ndk 20).

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 3, 2019

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

Conflicts

No conflicts as of last run.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 5, 2019

ACK after squashing the 968ae2f commit

@laanwj laanwj added the Android label Oct 5, 2019
@stefanwouldgo stefanwouldgo force-pushed the bitcoinprivacy:master branch from 968ae2f to 3346b02 Oct 7, 2019
@stefanwouldgo

This comment has been minimized.

Copy link
Contributor Author

stefanwouldgo commented Oct 7, 2019

ACK after squashing the 968ae2f commit

Done.

@kristapsk

This comment has been minimized.

Copy link
Contributor

kristapsk commented Oct 13, 2019

Don't know about other Linux distros, but on Gentoo 2.1.8 is latest unmasked (stable) version, 2.1.11 is still masked.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 17, 2019

Don't know about other Linux distros, but on Gentoo 2.1.8 is latest unmasked (stable) version, 2.1.11 is still masked.

FWIW, what distributions use is not important for what is used with depends. The binaries are statically linked to libevent and this doesn't change the minimum supported version for building from source (that's still 2.0.22).

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 17, 2019

I think the concern was the we don't want to be the bleeding edge testers of new releases, but rather want to wait for others to test new software and only then pull it in.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Oct 17, 2019

At least it is in debian experimental: https://packages.debian.org/experimental/libevent-dev 😬

@bitcoin bitcoin deleted a comment from DrahtBot Oct 17, 2019
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 25, 2019

Well, 0.20.0 is still more than 6 months away so by that time it surely won't be bleeding edge anymore.

I think better android support is a good reason to merge this. Would be nice to make some progress in that regard.

@bitcoin bitcoin deleted a comment from DrahtBot Nov 4, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 4, 2019

Please remove the patch after rebase, now that it is no longer needed.

@stefanwouldgo

This comment has been minimized.

Copy link
Contributor Author

stefanwouldgo commented Nov 4, 2019

Please remove the patch after rebase, now that it is no longer needed.

The patch isn't in this repo AFAIK.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 4, 2019

@stefanwouldgo Please rebase on current master first

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 4, 2019

Could squash the two commits into one?

@stefanwouldgo stefanwouldgo force-pushed the bitcoinprivacy:master branch from b7790ad to 8d87e90 Nov 4, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 4, 2019


ifneq (,$(findstring android,$(host)))
define $(package)_preprocess_cmds
./autogen.sh && patch -p1 < $($(package)_patch_dir)/fix_android_arc4random_addrandom.patch

This comment has been minimized.

Copy link
@icota

icota Nov 4, 2019

Contributor

I haven't tested the build with this change but since the fix_android_arc4random_addrandom.patch is no longer used it's probably best to remove the file itself and its libevent.mk presence as well.

This comment has been minimized.

Copy link
@stefanwouldgo

stefanwouldgo Nov 4, 2019

Author Contributor

it's probably best to remove the file itself

Sorry, fixed.

and its libevent.mk presence as well.

I don't see any further presence. Did I overlook anything?

This comment has been minimized.

Copy link
@icota

icota Nov 4, 2019

Contributor

In (package)_patches?

This comment has been minimized.

Copy link
@icota

icota Nov 4, 2019

Contributor

Sorry, you've removed it. My bad.

this doesn't need patches on Android anymore like 2.1.8 did.
@stefanwouldgo stefanwouldgo force-pushed the bitcoinprivacy:master branch from 8d87e90 to 02ac445 Nov 4, 2019
@icota
icota approved these changes Nov 4, 2019
@icota

This comment has been minimized.

Copy link
Contributor

icota commented Nov 4, 2019

utACK

@DrahtBot DrahtBot removed the Needs rebase label Nov 4, 2019
@dongcarl

This comment has been minimized.

Copy link
Contributor

dongcarl commented Nov 4, 2019

Context for other reviewers:

Here are the upstream equivalent patches:
libevent/libevent@6541168, which introduced a regression tracked in libevent/libevent#488, later fixed in libevent/libevent@266f43a

Our current patch:
https://github.com/bitcoin/bitcoin/blob/bbc9e4133cc5e2c970a557f7c2368e04911aec43/depends/patches/libevent/fix_android_arc4random_addrandom.patch

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 6, 2019

Gitian builds

File commit bdda137
(master)
commit 6ee0f55
(master and this pull)
bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz 338c2453e92576d6... 963f3e507ac8729d...
bitcoin-0.19.99-aarch64-linux-gnu.tar.gz ebfe02ebb327a5fd... 4be3551897128785...
bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz 10685a8364a42850... ac03b9d7cd8ecb30...
bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz 5debad8ecf03f614... 9fae2275536d122a...
bitcoin-0.19.99-i686-pc-linux-gnu-debug.tar.gz c94274278265de7d... b990b189199a6e0d...
bitcoin-0.19.99-i686-pc-linux-gnu.tar.gz 9199c2a5e431eb9a... a00e4dd540407d3f...
bitcoin-0.19.99-osx-unsigned.dmg 7da5a7f569a4318d... e068a60e7382af35...
bitcoin-0.19.99-osx64.tar.gz a459a9173951314d... 645f28435a9918bd...
bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz 660347859b4073ee... f306e77ef800b045...
bitcoin-0.19.99-riscv64-linux-gnu.tar.gz feb185a593b038d9... f86f2afcccaaad0d...
bitcoin-0.19.99-win64-debug.zip a58bd2fba33f4e15... 4164fe781db238a4...
bitcoin-0.19.99-win64-setup-unsigned.exe 501af3958d794c3d... 22a66849e817d991...
bitcoin-0.19.99-win64.zip fb49f8e495bd1bdc... 727f6d0df8f69fdf...
bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz 8b1f655dfa7aab99... 4c6b26fb59920f2c...
bitcoin-0.19.99-x86_64-linux-gnu.tar.gz 21b36fd6521700d8... 0b9b8a73e8b67803...
bitcoin-0.19.99.tar.gz 96c42231845ab16d... 500b4050b258dab7...
bitcoin-core-linux-0.20-res.yml 5fa050a02e3cb95c... a7d650248d5044fe...
bitcoin-core-osx-0.20-res.yml d0d8fb2e491f47a0... 0c3664b345167297...
bitcoin-core-win-0.20-res.yml 6f9be4b4073760a5... 3af3c33b05d19d79...
linux-build.log b31ba24d0a233210... 1e0a3261d66d7119...
osx-build.log eaeccfa77df8d98a... ee93a0513a6e88da...
win-build.log b9e547f8f55648b3... 04bc3c43537c5bb8...
bitcoin-core-linux-0.20-res.yml.diff b9f6b3a9aacdeaad...
bitcoin-core-osx-0.20-res.yml.diff 85ba9dd065ecdeb9...
bitcoin-core-win-0.20-res.yml.diff ab11b240b77b69bd...
linux-build.log.diff 22b209015dfec5ab...
osx-build.log.diff f41fee0ac527a72d...
win-build.log.diff f5518a36a880d9a8...
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 6, 2019

ACK 02ac445

laanwj added a commit that referenced this pull request Nov 20, 2019
02ac445 bump libevent to 2.1.11 in depends (stefanwouldgo)

Pull request description:

  this doesn't need patches on Android anymore like 2.1.8 did.

ACKs for top commit:
  laanwj:
    ACK 02ac445

Tree-SHA512: 1fbfe342ee15fa4c5cb417979bd6c443f7c7aa40a489accf8ccd7c919e5b08e859b3da6edeee3de484f6f156b35dd4e97c7e2c7971b59fc31029865585ccb296
@laanwj laanwj merged commit 02ac445 into bitcoin:master Nov 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 20, 2019

Ah, looks like it made it into bullseye: https://packages.debian.org/bullseye/libevent-dev

sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 20, 2019
02ac445 bump libevent to 2.1.11 in depends (stefanwouldgo)

Pull request description:

  this doesn't need patches on Android anymore like 2.1.8 did.

ACKs for top commit:
  laanwj:
    ACK 02ac445

Tree-SHA512: 1fbfe342ee15fa4c5cb417979bd6c443f7c7aa40a489accf8ccd7c919e5b08e859b3da6edeee3de484f6f156b35dd4e97c7e2c7971b59fc31029865585ccb296
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.