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

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:zmq-upgrade changing 7 files +33 −27
  1. mruddy commented at 2:54 pm on June 30, 2018: contributor

    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.:

    0/bitcoin/depends> make zeromq
1Extracting zeromq...
2/bitcoin/depends/sources/zeromq-4.2.5.tar.gz: OK
3Preprocessing zeromq...
4patching file src/windows.hpp
5Hunk [#1](/bitcoin-bitcoin/1/) succeeded at 58 (offset 3 lines).
6patching file src/thread.cpp
7Hunk [#1](/bitcoin-bitcoin/1/) succeeded at 307 with fuzz 2 (offset 87 lines).
8Hunk [#2](/bitcoin-bitcoin/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

  2. fanquake added the label Build system on Jul 1, 2018
  3. fanquake commented at 4:37 am on July 1, 2018: member

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

  4. in depends/patches/zeromq/0001-fix-build-with-older-mingw64.patch:1 in db8096aa8e outdated 
    0@@ -1,17 +1,17 @@
1-From 1a159c128c69a42d90819375c06a39994f3fbfc1 Mon Sep 17 00:00:00 2001
    fanquake commented at 4:38 am on July 1, 2018:
    Given that these are just being updated for upstream, you could leave the original From/Date
    mruddy commented at 12:47 pm on July 1, 2018:
    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.
  5. mruddy commented at 12:36 pm on July 1, 2018: contributor

    @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”.

  6. mruddy force-pushed on Jul 6, 2018
  7. mruddy renamed this:
    [depends] zeromq 4.2.5
    [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions
    on Jul 6, 2018
  8. mruddy commented at 3:28 am on July 6, 2018: contributor

    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.
  9. mruddy commented at 8:51 pm on July 16, 2018: contributor
    Just noting that I think this PR is ready to merge. The current latest commit 473a48cae5d63f5c0baee4990756d4a9b18c4b5f covers all feedback received.
  10. jmcorgan commented at 9:50 pm on July 16, 2018: contributor
    utACK
  11. greenaddress commented at 3:32 pm on July 17, 2018: contributor

    I know is late for this but it would be great if we could also add this commit as a patch https://github.com/zeromq/libzmq/pull/3140/commits/58d13395ece1fa0dd9b0583d736af4ac342c1267

    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 https://github.com/bitcoin/bitcoin/pull/13689

  12. laanwj commented at 2:17 pm on July 18, 2018: member
    utACK 473a48cae5d63f5c0baee4990756d4a9b18c4b5f
  13. laanwj commented at 2:19 pm on July 18, 2018: member
    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.
  14. mruddy commented at 9:22 pm on July 18, 2018: contributor
    @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.
  15. MarcoFalke referenced this in commit 8c36432791 on Jul 19, 2018
  16. DrahtBot commented at 4:08 pm on August 2, 2018: member
  17. DrahtBot added the label Needs rebase on Aug 6, 2018
  18. DrahtBot commented at 4:10 pm on August 6, 2018: member
  19. mruddy force-pushed on Aug 6, 2018
  20. mruddy commented at 9:14 pm on August 6, 2018: contributor
    rebased
  21. fanquake removed the label Needs rebase on Aug 6, 2018
  22. MarcoFalke added the label Needs gitian build on Aug 16, 2018
  23. MarcoFalke removed the label Needs gitian build on Aug 16, 2018
  24. MarcoFalke added the label Needs gitian build on Aug 17, 2018
  25. MarcoFalke removed the label Needs gitian build on Aug 18, 2018
  26. MarcoFalke added the label Needs gitian build on Aug 18, 2018
  27. DrahtBot removed the label Needs gitian build on Aug 22, 2018
  28. mruddy commented at 11:59 am on August 23, 2018: contributor
    This is ready to merge.
  29. in doc/build-unix.md:50 in 33d997d667 outdated 
    46@@ -47,7 +47,7 @@ Optional dependencies:
47  protobuf    | Payments in GUI  | Data interchange format used for payment protocol (only needed when GUI enabled)
48  libqrencode | QR codes in GUI  | Optional for generating QR codes (only needed when GUI enabled)
49  univalue    | Utility          | JSON parsing and encoding (bundled version will be used unless --with-system-univalue passed to configure)
50- libzmq3     | ZMQ notification | Optional, allows generating ZMQ notifications (requires ZMQ version >= 4.x)
51+ libzmq3     | ZMQ notification | Optional, allows generating ZMQ notifications
    ken2812221 commented at 12:57 pm on August 23, 2018:
    Why change this?
    mruddy commented at 4:15 pm on August 23, 2018:
    it’s already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.
    laanwj commented at 12:56 pm on August 31, 2018:
    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.
    mruddy commented at 8:37 pm on September 11, 2018:
    ok, i’ve mostly re-duplicated the version documentation and rebased and squashed it all again.
  30. in doc/build-unix.md:96 in 33d997d667 outdated 
    92@@ -93,7 +93,7 @@ Optional (see --with-miniupnpc and --enable-upnp-default):
93 
94     sudo apt-get install libminiupnpc-dev
95 
96-ZMQ dependencies (provides ZMQ API 4.x):
97+ZMQ dependencies (provides ZMQ API):
    ken2812221 commented at 12:58 pm on August 23, 2018:
    Why change this?
    mruddy commented at 4:15 pm on August 23, 2018:
    it’s already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.
  31. in doc/zmq.md:36 in 33d997d667 outdated 
    32@@ -33,8 +33,10 @@ buffering or reassembly.
33 
34 ## Prerequisites
35 
36-The ZeroMQ feature in Bitcoin Core requires ZeroMQ API version 4.x or
37-newer. Typically, it is packaged by distributions as something like
38+The ZeroMQ feature in Bitcoin Core requires the ZeroMQ API
    ken2812221 commented at 1:00 pm on August 23, 2018:
    Why get rid of version limit?
    mruddy commented at 4:15 pm on August 23, 2018:
    it’s already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.
  32. in doc/zmq.md:83 in 33d997d667 outdated 
    79@@ -78,7 +80,7 @@ bytes).
80 These options can also be provided in bitcoin.conf.
81 
82 ZeroMQ endpoint specifiers for TCP (and others) are documented in the
83-[ZeroMQ API](http://api.zeromq.org/4-0:_start).
84+[ZeroMQ API](http://api.zeromq.org/).
    ken2812221 commented at 1:01 pm on August 23, 2018:
    Why change this?
    mruddy commented at 4:15 pm on August 23, 2018:
    it’s already centrally located in doc/dependencies.md. this removes redundant info that can get out of synch later.
  33. ken2812221 changes_requested
  34. ken2812221 commented at 1:03 pm on August 23, 2018: contributor
    Maybe you did something wrong when you rebase this. Otherwise utACK
  35. [depends, zmq, doc] upgrade zeromq to 4.2.5 and avoid deprecated zeromq api functions f1bd03eb01
  36. mruddy force-pushed on Sep 11, 2018
  37. ken2812221 commented at 0:04 am on September 17, 2018: contributor
    utACK f1bd03e
  38. laanwj merged this on Sep 17, 2018
  39. laanwj closed this on Sep 17, 2018
  40. laanwj referenced this in commit 2d4749b366 on Sep 17, 2018
  41. mruddy deleted the branch on Sep 17, 2018
  42. MarcoFalke deleted a comment on Sep 17, 2018
  43. deadalnix referenced this in commit e2dd836231 on Mar 31, 2020
  44. PastaPastaPasta referenced this in commit bcdd83da7c on Jun 9, 2020
  45. PastaPastaPasta referenced this in commit 274911cbb9 on Jun 9, 2020
  46. PastaPastaPasta referenced this in commit 7b0df1dd89 on Jun 10, 2020
  47. PastaPastaPasta referenced this in commit e9d78ed017 on Jun 10, 2020
  48. PastaPastaPasta referenced this in commit cd96e52b4d on Jun 11, 2020
  49. PastaPastaPasta referenced this in commit 1ee1074e22 on Jun 11, 2020
  50. PastaPastaPasta referenced this in commit b31cb3a365 on Jun 12, 2020
  51. PastaPastaPasta referenced this in commit e84e74cab5 on Jun 14, 2020
  52. str4d referenced this in commit edfb4d98e7 on Oct 5, 2020
  53. zkbot referenced this in commit e3d5ddbef4 on Oct 8, 2020
  54. zkbot referenced this in commit 6e4090d840 on Oct 8, 2020
  55. gades referenced this in commit 25565e3b1e on Jun 24, 2021
  56. CryptoCentric referenced this in commit 7d0e2fe99b on Jul 2, 2021
  57. ftrader referenced this in commit 323a004923 on Aug 13, 2021
  58. dzutto referenced this in commit 6390d05eaf on Sep 7, 2021
  59. MarcoFalke locked this on Sep 8, 2021


mruddy fanquake jmcorgan greenaddress laanwj DrahtBot ken2812221

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me