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

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.
luke-jr cross-referenced this on Jan 15, 2016 from issue Replace univalue subtree with proper dependency on external UniValue by luke-jr
luke-jr force-pushed on Jan 15, 2016
luke-jr force-pushed on Jan 15, 2016
Bugfix: The var is LIBUNIVALUE,not LIBBITCOIN_UNIVALUE 2adf7e2c90
Build against system UniValue when available ab22705a7b
doc: Add UniValue to build instructions 5d3b29bc00
luke-jr force-pushed on Jan 15, 2016
jonasschnelli commented at 7:26 AM on January 15, 2016: contributor

This is nice. Concept ACK.
jonasschnelli added the label Build system on Jan 15, 2016
jgarzik commented at 1:55 PM on January 15, 2016: contributor

ut ACK
laanwj commented at 10:58 AM on January 18, 2016: member

utACK
laanwj commented at 2:51 PM on January 20, 2016: member

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

in src/Makefile.am:None 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.

in src/Makefile.bench.include:None 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.

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...
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.
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.
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.
gmaxwell commented at 7:59 PM on January 27, 2016: contributor

I agree with @theuni.
Change default configure option --with-system-univalue to "no" 23565157ba
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.
laanwj commented at 10:44 AM on January 28, 2016: member

Ok, thanks, utACK after other two @theuni's nits solved
Bugfix: Always include univalue in DIST_SUBDIRS 62f7f2ee21
LDADD dependency order shuffling cdcad9fc5f
luke-jr commented at 2:33 AM on January 31, 2016: member

@theuni 's nits taken care of
build-unix: Update UniValue build conditions 42407ed43a

in doc/build-unix.md:None 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

theuni commented at 9:53 PM on February 1, 2016: member

Thanks, ut ack.
laanwj merged this on Feb 4, 2016
laanwj closed this on Feb 4, 2016
laanwj referenced this in commit 152a8216cc on Feb 4, 2016
laanwj cross-referenced this on Feb 8, 2016 from issue Use system univalue by default by luke-jr
OlegGirko cross-referenced this on Jul 3, 2017 from issue Backport Bitcoin PR#7349: Build against system UniValue when available by OlegGirko
MarcoFalke cross-referenced this on Jul 8, 2021 from issue Bugfix: Workaround UniValue push_back(bool) limitation with push_back(UniValue(bool)) by luke-jr
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7340 Replace univalue subtree with proper dependency on external UniValue #7485 Use system univalue by default #22412 Bugfix: Workaround UniValue push_back(bool) limitation with push_back(UniValue(bool))#25369 Unsubtree Univalue