Skip to content

autotools: move checking for zmq library to common area in configure.ac - #6743

Merged
laanwj merged 1 commit into
bitcoin:masterfrom
jmcorgan:fixes/6679
Oct 6, 2015
Merged

autotools: move checking for zmq library to common area in configure.ac#6743
laanwj merged 1 commit into
bitcoin:masterfrom
jmcorgan:fixes/6679

Conversation

@jmcorgan

@jmcorgan jmcorgan commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

Fixes #6679

@jmcorgan

jmcorgan commented Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

Looks like Windows builds are failing, but probably not due to Windows per se. I suspect the new configure.ac logic is not properly shutting off ENABLE_ZMQ when the library is not found.

@jonasschnelli

Copy link
Copy Markdown
Contributor

I think you need to keep the else with something similar to AC_DEFINE_UNQUOTED([ENABLE_ZMQ],[0],[Define to 1 to enable ZMQ functions]).
Travis Win32/64 does not have a libzmq installed and therefore should not compile the zmq features.

@jmcorgan

jmcorgan commented Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

Ah, missed that, thanks.

@jmcorgan

jmcorgan commented Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

That didn't work. Calling @theuni ...

@jmcorgan

jmcorgan commented Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

The fix required implementing the logic to test for zmq or not when use_pkgconfig was set to no.

@theuni

theuni commented Oct 2, 2015

Copy link
Copy Markdown
Member

@jmcorgan yep, that was the right fix. Although, the non-pkg-config check isn't testing versions. Can you change the AC_CHECK_LIB to test for something new in v4? If there's not a new symbol to check for, we'll have to resort to a quick compile test.

Edit: If this is to be believed, a check for zmq_ctx_shutdown should do the trick.

@theuni

theuni commented Oct 2, 2015

Copy link
Copy Markdown
Member

Nit: Please move the new check up above the protobuf check, so it's not mixed in with the qt stuff.

@jmcorgan

jmcorgan commented Oct 2, 2015

Copy link
Copy Markdown
Contributor Author

Understood about check for a 4.x only function in the non-pkgconfig section, but I'm confused by your request for moving the checks to a different place. Can you be more specific about which lines of code you want to move and to which location?

@theuni

theuni commented Oct 2, 2015

Copy link
Copy Markdown
Member
  if test "x$use_zmq" = "xyes"; then
     AC_CHECK_HEADER([zmq.h],

That whole block Up about 5 lines, so it's before BITCOIN_QT_CHECK...

@jmcorgan

jmcorgan commented Oct 3, 2015

Copy link
Copy Markdown
Contributor Author

Done.

* Fixes bitcoin#6679

* Tested with --disable-zmq
* Tested with and without pkgconfig
* Tested with and without zmq installed

Signed-off-by: Johnathan Corgan <johnathan@corganlabs.com>
@jmcorgan

jmcorgan commented Oct 3, 2015

Copy link
Copy Markdown
Contributor Author

@theuni This should all be good to go now.

@theuni

theuni commented Oct 5, 2015

Copy link
Copy Markdown
Member

Looks good, ut ACK

@laanwj
laanwj merged commit dd28089 into bitcoin:master Oct 6, 2015
laanwj added a commit that referenced this pull request Oct 6, 2015
dd28089 autotools: move checking for zmq library to common area in configure.ac (Johnathan Corgan)
@jmcorgan
jmcorgan deleted the fixes/6679 branch October 6, 2015 18:53
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
zkbot added a commit to zcash/zcash that referenced this pull request Feb 9, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ZMQ: configury should not unconditionally use pkg-config

4 participants