Build against system UniValue when available #7349

pull luke-jr wants to merge 7 commits into bitcoin:master from luke-jr:sys_univalue_opt changing 5 files +64 −8
  1. luke-jr commented at 3:21 am on January 15, 2016: member

    If UniValue is present on the system, it will be used instead of bundled copy. If not, bundled copy will automatically be built and used statically.

    User can override either way using –with[out]-system-univalue configure option.

  2. luke-jr force-pushed on Jan 15, 2016
  3. luke-jr force-pushed on Jan 15, 2016
  4. Bugfix: The var is LIBUNIVALUE,not LIBBITCOIN_UNIVALUE 2adf7e2c90
  5. Build against system UniValue when available ab22705a7b
  6. doc: Add UniValue to build instructions 5d3b29bc00
  7. luke-jr force-pushed on Jan 15, 2016
  8. jonasschnelli commented at 7:26 am on January 15, 2016: contributor
    This is nice. Concept ACK.
  9. jonasschnelli added the label Build system on Jan 15, 2016
  10. jgarzik commented at 1:55 pm on January 15, 2016: contributor
    ut ACK
  11. laanwj commented at 10:58 am on January 18, 2016: member
    utACK
  12. laanwj commented at 2:51 pm on January 20, 2016: member
    Ping @theuni
  13. theuni commented at 6:23 pm on January 22, 2016: member
    Concept ack, but let’s not default to using the system lib unless –with-system-univalue is specified, please. Otherwise someone (like myself) who has at some point built/installed univalue to system will suddenly find themselves frustrated when their in-tree univalue source changes aren’t reflected in the build.
  14. in src/Makefile.am: in 5d3b29bc00 outdated
    0@@ -1,9 +1,20 @@
    1-DIST_SUBDIRS = secp256k1 univalue
    2+DIST_SUBDIRS = secp256k1
    3 
    4 AM_LDFLAGS = $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS)
    5 AM_CXXFLAGS = $(HARDENED_CXXFLAGS)
    6 AM_CPPFLAGS = $(HARDENED_CPPFLAGS)
    7 
    8+if EMBEDDED_UNIVALUE
    9+DIST_SUBDIRS += univalue
    


    theuni commented at 6:24 pm on January 22, 2016:
    always add to dist, regardless of the config.
  15. in src/Makefile.bench.include: in 5d3b29bc00 outdated
    13@@ -14,7 +14,7 @@ bench_bench_bitcoin_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
    14 bench_bench_bitcoin_LDADD = \
    15   $(LIBBITCOIN_SERVER) \
    16   $(LIBBITCOIN_COMMON) \
    17-  $(LIBBITCOIN_UNIVALUE) \
    18+  $(LIBUNIVALUE) \
    


    theuni commented at 6:26 pm on January 22, 2016:
    Please move univalue to after the internal libs while you’re at it, since it’s not possible for univalue to depend on them.
  16. luke-jr commented at 4:06 am on January 23, 2016: member

    Concept ack, but let’s not default to using the system lib unless –with-system-univalue is specified, please. Otherwise someone (like myself) who has at some point built/installed univalue to system will suddenly find themselves frustrated when their in-tree univalue source changes aren’t reflected in the build.

    Just use –without-system-univalue. Defaults should not be set just for niche scenarios…

  17. theuni commented at 7:16 pm on January 25, 2016: member
    Agreed, and the new (system) scenario is the niche scenario. It should be opt-in as long as we build it in-tree.
  18. luke-jr commented at 9:26 pm on January 25, 2016: member
    Using system libraries when they are available is the standard use case, not the niche. I’m happy to go back to #7340 if that’s preferable though.
  19. laanwj commented at 11:41 am on January 26, 2016: member

    I’m not yet convinced that univalue will be a library found by default on many systems, will be maintained actively, and if so will do proper versioning in lock-step with our requirements.

    So to err on the side of caution I agree with @theuni, defaulting to the in-tree univalue makes sense.

    It can always be reconsidered later when things have matured for a few versions.

  20. gmaxwell commented at 7:59 pm on January 27, 2016: contributor
    I agree with @theuni.
  21. Change default configure option --with-system-univalue to "no" 23565157ba
  22. luke-jr commented at 5:32 am on January 28, 2016: member
    I think defaulting to “no” is still very unreasonable considering @jgarzik is the maintainer, but changed for now.
  23. laanwj commented at 10:44 am on January 28, 2016: member
    Ok, thanks, utACK after other two @theuni’s nits solved
  24. Bugfix: Always include univalue in DIST_SUBDIRS 62f7f2ee21
  25. LDADD dependency order shuffling cdcad9fc5f
  26. luke-jr commented at 2:33 am on January 31, 2016: member
    @theuni ’s nits taken care of
  27. build-unix: Update UniValue build conditions 42407ed43a
  28. in doc/build-unix.md: in cdcad9fc5f outdated
    45@@ -46,6 +46,7 @@ Optional dependencies:
    46  qt          | GUI              | GUI toolkit (only needed when GUI enabled)
    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 (if missing, bundled version will be used)
    


    laanwj commented at 9:50 am on February 1, 2016:
    Nit: should specify that bundled version is used except if --with-system-univalue

    luke-jr commented at 6:49 pm on February 1, 2016:
    Fixed
  29. theuni commented at 9:53 pm on February 1, 2016: member
    Thanks, ut ack.
  30. laanwj merged this on Feb 4, 2016
  31. laanwj closed this on Feb 4, 2016

  32. laanwj referenced this in commit 152a8216cc on Feb 4, 2016
  33. MarcoFalke locked this on Sep 8, 2021

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-24 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me