Skip to content

Conversation

jmcorgan
Copy link
Contributor

@jmcorgan jmcorgan commented Oct 1, 2015

Fixes #6679

@jmcorgan
Copy link
Contributor Author

jmcorgan commented Oct 1, 2015

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
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
Copy link
Contributor Author

jmcorgan commented Oct 1, 2015

Ah, missed that, thanks.

@jmcorgan
Copy link
Contributor Author

jmcorgan commented Oct 1, 2015

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

@jmcorgan
Copy link
Contributor Author

jmcorgan commented Oct 1, 2015

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

@theuni
Copy link
Member

theuni commented Oct 2, 2015

@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
Copy link
Member

theuni commented Oct 2, 2015

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

@jmcorgan
Copy link
Contributor Author

jmcorgan commented Oct 2, 2015

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
Copy link
Member

theuni commented Oct 2, 2015

  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
Copy link
Contributor Author

jmcorgan commented Oct 3, 2015

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
Copy link
Contributor Author

jmcorgan commented Oct 3, 2015

@theuni This should all be good to go now.

@theuni
Copy link
Member

theuni commented Oct 5, 2015

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