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

Require ZMQ version 4.x and update/cleanup zmq docs #6736

Merged
merged 2 commits into from
Sep 29, 2015

Conversation

jmcorgan
Copy link
Contributor

This PR changes the configure.ac to require ZMQ version 4.x or newer, which is provided on Debian and derivatives as package libzmq3-dev. It also updates the release notes, build-unix.md, and zmq.md docs to reflect this change and add missing documentation.

(Whitespace changes are purely trimming of extraneous previous whitespace by my editor.)

Signed-off-by: Johnathan Corgan <johnathan@corganlabs.com>
Signed-off-by: Johnathan Corgan <johnathan@corganlabs.com>
@jmcorgan jmcorgan changed the title Update zmq docs Require ZMQ version 4.x and update/cleanup zmq docs Sep 29, 2015
@laanwj laanwj added the Docs label Sep 29, 2015
@@ -157,11 +157,11 @@ if test "x$enable_debug" = xyes; then
if test "x$GCC" = xyes; then
CFLAGS="$CFLAGS -g3 -O0"
fi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed on IRC, I'd prefer to not have the unnecessary whitespace changes here in sections you're otherwise not touching

@laanwj
Copy link
Member

laanwj commented Sep 29, 2015

utACK apart from unnecessary space changes in build system

@gavinandresen
Copy link
Contributor

utACK.

Suggested addition, add zeromq to the build-osx.md dependencies:

diff --git a/doc/build-osx.md b/doc/build-osx.md
index 201fe95..af0d2c4 100644
--- a/doc/build-osx.md
+++ b/doc/build-osx.md
@@ -32,7 +32,7 @@ Instructions: Homebrew

 #### Install dependencies using Homebrew

-        brew install autoconf automake berkeley-db4 libtool boost miniupnpc openssl pkg-config protobuf qt5 libevent
+        brew install autoconf automake berkeley-db4 libtool boost miniupnpc openssl pkg-config protobuf qt5 libevent zeromq

 NOTE: Building with Qt4 is still supported, however, could result in a broken UI. As such, building with Qt5 is recommended.

(installs ZMQ version 4.1.3 right now)

@jmcorgan
Copy link
Contributor Author

Those changes all remove excess or invisible whitespace, which my editor does as a way of not introducing them in the first place. I'd rather not turn this feature off only when editing bitcoin source code.

@jmcorgan
Copy link
Contributor Author

@gavinandresen I think there is an existing PR for that, I'll check.

@laanwj
Copy link
Member

laanwj commented Sep 29, 2015

Those changes all remove excess or invisible whitespace, which my editor does as a way of not introducing them in the first place. I'd rather not turn this feature off only when editing bitcoin source code.

@jmcorgan I understand it's annoying to have to change editor settings per project. But unnecessary whitespace changes create more merge conflicts/rebases. This is acceptable for relatively seldom changing files like the documentation, but not for e.g. configure.ac. Changes to code should be as localized as possible.

@theuni
Copy link
Member

theuni commented Sep 29, 2015

tangentially related: I have a fixed version of #6686 coming up, just doing a few tests first.

@theuni
Copy link
Member

theuni commented Sep 29, 2015

There are docs illustrating how to remain compatible with the 2.x api here: http://zeromq.org/docs:3-1-upgrade

Since there are obviously lots of 2.x/3.x libs still in the wild, I think it makes sense to try to support them instead.

@jmcorgan
Copy link
Contributor Author

I'll leave it to the core-devs to decide what compatibility they want to introduce here. If previous API compatibility is desired, there will need to be code changes, and we'll start this process all over again. Alternatively, it could be accepted as-is, and if it turns out users want that, it could be changed at that point.

@theuni
Copy link
Member

theuni commented Sep 29, 2015

@jmcorgan What do you mean by starting the process all over again?

I've pushed a change here that fixes build with >=libzmq 2.2.0: theuni@a89a62f

Verified that the rpc tests are successful.

I also went through the rest of the API migration guide and verified that we don't violate any rules for API compatibility, so this really is just a matter of fixing builds.

@jmcorgan
Copy link
Contributor Author

I mean code review, testing, etc. I've done it for ZMQ 4.x, but not the earlier ones. If the rpc tests are sufficient, then that's great.

@jgarzik
Copy link
Contributor

jgarzik commented Sep 29, 2015

ut ACK

@ghost ghost mentioned this pull request Sep 29, 2015
@ghost
Copy link

ghost commented Sep 29, 2015

ut ACK. Interesting that build notes slipped through.

@laanwj laanwj merged commit ab0b8be into bitcoin:master Sep 29, 2015
laanwj added a commit that referenced this pull request Sep 29, 2015
ab0b8be zmq: update and cleanup build-unix, release-notes, and zmq docs (Johnathan Corgan)
6cebd5d zmq: require version 4.x or newer of libzmq (Johnathan Corgan)
@laanwj
Copy link
Member

laanwj commented Sep 29, 2015

2.x support is something for a different pull and shouldn't hold up getting the documentation in order.

I'm personally not sure it's worth it - zmq is an optional dependency so people that want to use it can make sure they have the right version installed. Supporting 2.x-4.x extends the spectrum over which testing has to be done (once you promise a version will work, users expect it to keep working), and if we ever decide we want to use more of the zmq API it's a self-imposed pain.

@ghost
Copy link

ghost commented Sep 29, 2015

Agreed with the PR split. May want to reconsider the setting, as being enabled by default might be construed as a "promise a version will work."

@jmcorgan jmcorgan deleted the update-zmq-docs branch October 1, 2015 15:42
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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants