Build against system UniValue when available #7349

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

MarcoFalke locked this on Sep 8, 2021