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

[depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions #13578

Merged
merged 1 commit into from Sep 17, 2018

Conversation

Projects
None yet
8 participants
@mruddy
Copy link
Contributor

commented Jun 30, 2018

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

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

@mruddy What's the motivation for bumping; is there a specific change/bugfix included in 4.2.3->4.2.5 that you require?

I've had a look at the patches, updating line numbers and formatting from upstream is fine.

Looks like we still also require the postprocess sed, as Libs.private: -lstdc++ is still included in 4.2.5's libzmq.pc.

Which platforms have you tested depends builds/gitian building on? Last time we updated zeromq we were stung with a sneaky change that broke gitian building, hence the need of the sed above.

@@ -1,17 +1,17 @@
From 1a159c128c69a42d90819375c06a39994f3fbfc1 Mon Sep 17 00:00:00 2001

This comment has been minimized.

Copy link
@fanquake

fanquake Jul 1, 2018

Member

Given that these are just being updated for upstream, you could leave the original From/Date

This comment has been minimized.

Copy link
@mruddy

mruddy Jul 1, 2018

Author Contributor

the date didn't change. the thing that changed was the hex value, which is only the local commit made to my libzmq repo so that i could run git format-patch master --stdout to get a patch that had most of the same formatting as the original one. it's a pretty meaningless line. i was actually thinking of just using the git diff output for the patch file contents as that would make these diffs easier in the future, but then you lose the commit comment being in the patch file.

@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

@fanquake My primary motivation is that my dev machine is Ubuntu 18.04 LTS and its libzmq5 package is already v4.2.5. Without building against depends, I was already using 4.2.5. Since I did the looking to figure that out, I figured that I'd make this update. This is the only machine where I tested the depends build as well.

From the 4.2.4 release notes, this one looked a little interesting. Although, I did NOT try to use it to make my node crash, so I don't know if it actually applies: "ZMQ_PUB crash when due to high volume of subscribe and unsubscribe messages, an unmatched unsubscribe message is received in certain conditions".

@mruddy mruddy force-pushed the mruddy:zmq-upgrade branch Jul 6, 2018

@mruddy mruddy changed the title [depends] zeromq 4.2.5 [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions Jul 6, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2018

Combined #13596 into this PR.
In summary:

  • Upgrade ZeroMQ from 4.2.3 to 4.2.5 (latest stable).
  • Centralize ZeroMQ version documentation into dependencies.md.
  • Note the already existing minimum ZMQ version requirement of 4.0.0 (enforced by configure.ac) which already supports switching to use the non-deprecated API functions.
  • Log what version of the ZMQ lib (libzmq) is used by the node in the zmq debug log messages.
@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

Just noting that I think this PR is ready to merge. The current latest commit 473a48cae5d63f5c0baee4990756d4a9b18c4b5f covers all feedback received.

@jmcorgan

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

utACK

@greenaddress

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

I know is late for this but it would be great if we could also add this commit as a patch zeromq/libzmq@58d1339

If you want to squash it in your PR i rebased it against yours in https://github.com/greenaddress/bitcoin/tree/zmq-upgrade-mruddy-patched

I also prepared a separate PR in #13689

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

utACK 473a48cae5d63f5c0baee4990756d4a9b18c4b5f

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

I think in general, though, it makes sense to avoid small version bumps for libraries such as zeromq unless there's a strong motivation to do so (such as a bug that affects us). This PR is good, but let's not make it a habit.

@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2018

@laanwj OK and thanks for reviewing this.

@greenaddress Just wanted to let you know that I'm not ignoring you, I've just been following the conversation over at #13689. I think I'd rather just leave them separate since this PR already has some ACKs and it looks like you're pretty well done with your changes in the other PR too. Thanks for your review too.

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
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

No more conflicts as of last run.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

Needs rebase

@mruddy mruddy force-pushed the mruddy:zmq-upgrade branch Aug 6, 2018

@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

rebased

@mruddy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

This is ready to merge.

@ken2812221
Copy link
Member

left a comment

Maybe you did something wrong when you rebase this. Otherwise utACK

doc/build-unix.md Outdated
@@ -47,7 +47,7 @@ Optional dependencies:
protobuf | Payments in GUI | Data interchange format used for payment protocol (only needed when GUI enabled)
libqrencode | QR codes in GUI | Optional for generating QR codes (only needed when GUI enabled)
univalue | Utility | JSON parsing and encoding (bundled version will be used unless --with-system-univalue passed to configure)
libzmq3 | ZMQ notification | Optional, allows generating ZMQ notifications (requires ZMQ version >= 4.x)
libzmq3 | ZMQ notification | Optional, allows generating ZMQ notifications

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 23, 2018

Member

Why change this?

This comment has been minimized.

Copy link
@mruddy

mruddy Aug 23, 2018

Author Contributor

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

This comment has been minimized.

Copy link
@laanwj

laanwj Aug 31, 2018

Member

I'm not convinced that removing this information everywhere is a good idea.
Yes, you are correct that the information is in dependencies.md, however I still think, for users actually reading this documentation, it is useful to mention the major version requirement in other places too.

This comment has been minimized.

Copy link
@mruddy

mruddy Sep 11, 2018

Author Contributor

ok, i've mostly re-duplicated the version documentation and rebased and squashed it all again.

@@ -93,7 +93,7 @@ Optional (see --with-miniupnpc and --enable-upnp-default):

sudo apt-get install libminiupnpc-dev

ZMQ dependencies (provides ZMQ API 4.x):
ZMQ dependencies (provides ZMQ API):

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 23, 2018

Member

Why change this?

This comment has been minimized.

Copy link
@mruddy

mruddy Aug 23, 2018

Author Contributor

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

doc/zmq.md Outdated
@@ -33,8 +33,10 @@ buffering or reassembly.

## Prerequisites

The ZeroMQ feature in Bitcoin Core requires ZeroMQ API version 4.x or
newer. Typically, it is packaged by distributions as something like
The ZeroMQ feature in Bitcoin Core requires the ZeroMQ API

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 23, 2018

Member

Why get rid of version limit?

This comment has been minimized.

Copy link
@mruddy

mruddy Aug 23, 2018

Author Contributor

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

doc/zmq.md Outdated
@@ -78,7 +80,7 @@ bytes).
These options can also be provided in bitcoin.conf.

ZeroMQ endpoint specifiers for TCP (and others) are documented in the
[ZeroMQ API](http://api.zeromq.org/4-0:_start).
[ZeroMQ API](http://api.zeromq.org/).

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Aug 23, 2018

Member

Why change this?

This comment has been minimized.

Copy link
@mruddy

mruddy Aug 23, 2018

Author Contributor

it's already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.

@mruddy mruddy force-pushed the mruddy:zmq-upgrade branch to f1bd03e Sep 11, 2018

@ken2812221

This comment has been minimized.

Copy link
Member

commented Sep 17, 2018

utACK f1bd03e

@laanwj laanwj merged commit f1bd03e into bitcoin:master Sep 17, 2018

2 checks passed

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

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

@mruddy mruddy deleted the mruddy:zmq-upgrade branch Sep 17, 2018

@bitcoin bitcoin deleted a comment from DrahtBot Sep 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.