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

[depends] zeromq 4.2.3 #11986

Merged
merged 2 commits into from Mar 6, 2018

Conversation

Projects
None yet
6 participants
@fanquake
Member

fanquake commented Dec 22, 2017

This is a followup to #9254 and #11981. Zeromq 4.2.3 was released just after #9254 was merged, and contains a years worth of improvements/bug fixes. See the release notes here.

Todo:

  • Add zeromq-4.2.3.tar.gz to /depends-sources on bitcoincore.org
  • Verify gitian builds are still OK
  • Check: zeromq/libzmq#2787

@fanquake fanquake requested review from jonasschnelli and theuni Dec 22, 2017

@da2ce7

This comment has been minimized.

da2ce7 commented Dec 24, 2017

@fanquake Hello.

With all the dependencies that are linked in; as I understand zeromq isn't turned on used by default, however it is linked by default.

My main concern is that in dependencies there is some reflective code that has been designed to compromise Bitcoin Core. Is this a realistic concern?; how much dew-diligence do we need to audit the history of linked libs to make sure there isn't any nastyness?

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 4, 2018

@da2ce7:
zmq will be enabled by default if present on the system,... and therefore through "depends/" builds (gitian, "official binaries"), it's enabled.

AFAIK even when zmq has not been enabled via -zmqpubhashblock (or similar), the code calls zmq_init(1);.

So... yeah. Unsure.

EDIT:
However, this is unrelated to this PR.

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 5, 2018

Linux build failed (haven't checked why exactly).
https://bitcoin.jonasschnelli.ch/build/446

@jonasschnelli

This comment has been minimized.

Member

jonasschnelli commented Jan 5, 2018

qt/bitcoin-qt: symbol pthread_setname_np from unsupported version GLIBC_2.12
Seems to be related to this PR.

@fanquake

This comment has been minimized.

Member

fanquake commented Feb 16, 2018

The issue here is that ZeroMQ started using pthread_set_name_np to name their background threads, see zeromq/libzmq#2367.

pthread_set_name_np was introduced in GLIB 2.12, and the max GLIB version allowed by symbol-check.py is 2.11.

We are currently using pthread_set_name_np for thread naming in util.cpp. Might be time to consider increasing the max allowed GLIB version in symbol-check.py ?

@laanwj

This comment has been minimized.

Member

laanwj commented Feb 22, 2018

Might be time to consider increasing the max allowed GLIB version in symbol-check.py ?

This is likely going to cause complaints, because it means the binary won't run on some distributions anymore. Not sure what the minimum glibc in commonly used (stable) distributions is, though.

We could open a separate issue for that.

@laanwj

This comment has been minimized.

Member

laanwj commented Mar 5, 2018

utACK 13a399a

@theuni

theuni approved these changes Mar 5, 2018

utACK 13a399a

@laanwj laanwj merged commit 13a399a into bitcoin:master Mar 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 6, 2018

Merge #11986: [depends] zeromq 4.2.3
13a399a depends: patch pthread_set_name_np out of zeromq (Cory Fields)
8f79226 depends: zeromq 4.2.3 (fanquake)

Pull request description:

  This is a followup to #9254 and #11981. Zeromq 4.2.3 was released just after #9254 was merged, and contains a years worth of improvements/bug fixes. See the release notes [here](https://github.com/zeromq/libzmq/releases/tag/v4.2.3).

  Todo:
  - [ ] Add zeromq-4.2.3.tar.gz to /depends-sources on bitcoincore.org
  - [ ] Verify gitian builds are still OK
  - [ ] Check: zeromq/libzmq#2787

Tree-SHA512: 85e06f47be3e1fdedcee50ce90e3391d69df2ea1c167472ffc3126d8970d418eb75141b970e422eb2fda9a8cad00e6ba5b36afa53565171a9ebaa152a9dc9b60

@sickpig sickpig referenced this pull request Jun 5, 2018

Merged

[PORT] Depends cherries #1120

@greenaddress

This comment has been minimized.

Contributor

greenaddress commented Jul 17, 2018

@fanquake Was the autogen.sh step removal intentional? Seems to break things (I apply this zeromq/libzmq@58d1339 to libzmq for ndk builds)

@theuni

This comment has been minimized.

Member

theuni commented Jul 17, 2018

@greenaddress The autogen.sh is only needed if you're not building from their bootstrapped release tarball. If you're building from some git commit, you'll need to add it back.

@greenaddress

This comment has been minimized.

Contributor

greenaddress commented Jul 17, 2018

@theuni fair enough. I'm doing a PR to backport a zmq upstream but unmerged patch zeromq/libzmq@58d1339
in (preparing for PR) https://github.com/greenaddress/bitcoin/commits/zmq_clang_depends and it needs autogen.sh otherwise fails with aclocal issues ( https://travis-ci.org/greenaddress/bitcoin/jobs/404902106 )

More context: #11844 (comment)

I'm aware of #13578 so i also backported it in case that gets merged first https://github.com/greenaddress/bitcoin/commits/zmq-upgrade-mruddy-patched

@greenaddress

This comment has been minimized.

Contributor

greenaddress commented Jul 17, 2018

@fanquake fanquake deleted the fanquake:zeromq-4-2-3 branch Jul 18, 2018

MarcoFalke added a commit that referenced this pull request Jul 19, 2018

Merge #13689: depends: disable Werror when building zmq
a4ba238 depends: disable Werror for zmqlib release, causes ndk build to break (Lawrence Nahum)

Pull request description:

  This PR ~~backports this libzmq commit zeromq/libzmq@58d1339 from this merged upstream PR (but unreleased as of today) zeromq/libzmq#3140 as a patch to our zmq 4.2.3 version.~~ passes --disable-Werror when we build zmq.

  For reference see #11844 (comment)

  ~~This patch~~ A similar patch is already in use in abcore/bitcoin_ndk and needed to build bitcoind for android using NDK or some versions of clang 6+ during cross compilation.

  This patch won't be necessary once zmqlib releases a 4.2.6+ version including the fix and bitcoin core updates to it but it may be good to [have](#13689 (comment))

  ~~It also reintroduces autogen.sh as it is necessary for the patch to work, otherwise you'll have aclocal issues (see https://travis-ci.org/greenaddress/bitcoin/jobs/404902106). autogen,sh was removed in master only #11986 as non-necessary but otherwise unreleased.~~

  In the likely case #13578 is merged first I rebased the patch for the newer zmq version (4.2.5) in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched and i can update the PR accordingly or close and open a new one.

Tree-SHA512: ba8780c84b4ac4ead5605c0305827431a85c1c8503b7ed255a552469b22bdb4ca018449282f738794f573f93caed95c14ede7caadb3a3f14989f57cb74501f00

laanwj added a commit that referenced this pull request Sep 17, 2018

Merge #13578: [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid d…
…eprecated zeromq api functions

f1bd03e [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions (mruddy)

Pull request description:

  Upgrade the ZeroMQ dependency from version 4.2.3 to the latest stable version 4.2.5.

  This PR Follows the lead of #11986.

  I upgraded both patch files to correspond to the version `4.2.5` libzmq files.
  I assume doing so is still necessary and correct.

  Without updating the patch line numbers, things appear to work, but you get extra log messages while building `depends` because things don't exactly match, e.g.:
  ```
  /bitcoin/depends> make zeromq
  Extracting zeromq...
  /bitcoin/depends/sources/zeromq-4.2.5.tar.gz: OK
  Preprocessing zeromq...
  patching file src/windows.hpp
  Hunk #1 succeeded at 58 (offset 3 lines).
  patching file src/thread.cpp
  Hunk #1 succeeded at 307 with fuzz 2 (offset 87 lines).
  Hunk #2 succeeded at 323 with fuzz 2 (offset 90 lines).
  ```
  Updating the patches seemed cleaner, so I did it. Note that libzmq had some whitespace changes, so that's why the updated patches do too.

  More info: https://github.com/zeromq/libzmq/releases/tag/v4.2.5

  tags: libzmq, zmq, 0mq

Tree-SHA512: 78659dd276b5311e40634b1bbebb802ddd6b69662ba3c84995ef1e3795c49a78b1635112c7fd72a405ea36e2cc3bdeb84e6d00d4e491a349bba1dafff50e2fa5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment