zmq: update to avoid deprecated zeromq api functions and log zmq version used #13596
Conversation
src/zmq/zmqnotificationinterface.cpp
Outdated
There was a problem hiding this comment.
Can be called before context creation?
There was a problem hiding this comment.
Yes, in practice, it seems that it can be. The reason why I still put it after context creation is because of this quote in the API docs at http://api.zeromq.org/master:zmq: Before using any ØMQ library functions you must create a ØMQ context.
edit: I just looked at the code. Yeah, I might have been too pedantic. As long as the API doc is not a contract, it's safe to move the call earlier: https://github.com/zeromq/libzmq/blob/master/src/zmq.cpp#L109-L114
Doesn't matter much to me. If it helps get it accepted, I'll change it.
There was a problem hiding this comment.
It can matter if zmq_ctx_new fails for some reason, then you don't get the version in the logs.
There was a problem hiding this comment.
Makes sense. Updated, thanks!
|
@mruddy ZeroMQ 4.2.3 is currently used in the depends system, but that doesn't necessarily mean it's the minimum version required to build Core. We don't currently have a minimum required version stated in the docs, but I assume the use of these functions should introduce one. Having a quick look at the zmq api docs, it looks like they are both available from 3.2.x onwards. I can't see mention of either in the |
|
@fanquake Good point. Yep, they've been around for a while (since 2012 - 2013). I found a mention in https://github.com/zeromq/libzmq/blob/master/NEWS in section I also found that the commits for |
7a0ac39 to
7d16ac4
Compare
|
In that case, please update dependencies.md with what would be the new minimum required version. I'd also suggest combining these changes into #13578, so that the zmq bump, testing, and any minimum version requirement discussion can happen together. |
|
@fanquake Sure, I'll combine this with #13578. I did more digging. We already require 4.x or later in the configure script. That aligns with Debian and Ubuntu packages for still supported releases (see https://packages.ubuntu.com/search?keywords=libzmq3-dev and https://packages.debian.org/search?keywords=libzmq3-dev). The nice thing is that the ZMQ project has repos for common distros that would make it easier for us to raise the minimum version (see http://zeromq.org/area:download and https://build.opensuse.org/repositories/network:messaging:zeromq:release-stable). Although, from what I've found, the functions that I'm switching to were around before, or with |
According to the libzmq API docs for version 4.2.3 (the version that we currently depend on) and later:
http://api.zeromq.org/master:zmq-init is deprecated by http://api.zeromq.org/master:zmq-ctx-new
http://api.zeromq.org/master:zmq-ctx-destroy is deprecated by http://api.zeromq.org/master:zmq-ctx-term
The one I/O thread set on
zmq_initis the default forzmq_ctx_new, so no further change is necessary.I don't believe anything is changing beyond function naming with
zmq_ctx_newandzmq_ctx_term-- the API docs read basically the same for the before and after functions.I also added a log message to output the exact version of ZMQ being used by the node.