/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Build against system UniValue when available #7349

Merged
merged 7 commits into from Feb 4, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
6 participants
@luke-jr @jonasschnelli @jgarzik @laanwj @theuni @gmaxwell
@luke-jr
Member

luke-jr commented Jan 15, 2016

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 luke-jr referenced this pull request Jan 15, 2016

Closed

Replace univalue subtree with proper dependency on external UniValue #7340

@jonasschnelli
Member

jonasschnelli commented Jan 15, 2016

This is nice.
Concept ACK.

@jonasschnelli jonasschnelli added the Build system label Jan 15, 2016

@jgarzik
Contributor

jgarzik commented Jan 15, 2016

ut ACK
@laanwj
Owner

laanwj commented Jan 18, 2016

utACK
@laanwj
Owner

laanwj commented Jan 20, 2016

Ping @theuni
@theuni
Member

theuni commented Jan 22, 2016

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.

@theuni theuni commented on an outdated diff Jan 22, 2016

View full outdated diff
src/Makefile.am
AM_LDFLAGS = $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS)
AM_CXXFLAGS = $(HARDENED_CXXFLAGS)
AM_CPPFLAGS = $(HARDENED_CPPFLAGS)
+if EMBEDDED_UNIVALUE
+DIST_SUBDIRS += univalue
@theuni

theuni Jan 22, 2016

Member

always add to dist, regardless of the config.

@theuni theuni commented on an outdated diff Jan 22, 2016

View full outdated diff
src/Makefile.bench.include
@@ -14,7 +14,7 @@ bench_bench_bitcoin_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
bench_bench_bitcoin_LDADD = \
$(LIBBITCOIN_SERVER) \
$(LIBBITCOIN_COMMON) \
- $(LIBBITCOIN_UNIVALUE) \
+ $(LIBUNIVALUE) \
@theuni

theuni Jan 22, 2016

Member

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
Member

luke-jr commented Jan 23, 2016

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
Member

theuni commented Jan 25, 2016

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
Member

luke-jr commented Jan 25, 2016

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
Owner

laanwj commented Jan 26, 2016

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
Member

gmaxwell commented Jan 27, 2016

I agree with @theuni.
@luke-jr
Member

luke-jr commented Jan 28, 2016

I think defaulting to "no" is still very unreasonable considering @jgarzik is the maintainer, but changed for now.
@laanwj
Owner

laanwj commented Jan 28, 2016

Ok, thanks, utACK after other two @theuni's nits solved
@luke-jr
Member

luke-jr commented Jan 31, 2016

@theuni 's nits taken care of

@laanwj laanwj and 1 other commented on an outdated diff Feb 1, 2016

View full outdated diff
doc/build-unix.md
@@ -46,6 +46,7 @@ Optional dependencies:
qt | GUI | GUI toolkit (only needed when GUI enabled)
protobuf | Payments in GUI | Data interchange format used for payment protocol (only needed when GUI enabled)
libqrencode | QR codes in GUI | Optional for generating QR codes (only needed when GUI enabled)
+ univalue | Utility | JSON parsing and encoding (if missing, bundled version will be used)
@laanwj

laanwj Feb 1, 2016

Owner

Nit: should specify that bundled version is used except if --with-system-univalue

@luke-jr

luke-jr Feb 1, 2016

Member

Fixed
@theuni
Member

theuni commented Feb 1, 2016

Thanks, ut ack.

@laanwj laanwj merged commit 42407ed into bitcoin:master Feb 4, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 4, 2016

@laanwj
Merge #7349: Build against system UniValue when available 
42407ed build-unix: Update UniValue build conditions (Luke Dashjr)
cdcad9f LDADD dependency order shuffling (Luke Dashjr)
62f7f2e Bugfix: Always include univalue in DIST_SUBDIRS (Luke Dashjr)
2356515 Change default configure option --with-system-univalue to "no" (Luke Dashjr)
5d3b29b doc: Add UniValue to build instructions (Luke Dashjr)
ab22705 Build against system UniValue when available (Luke Dashjr)
2adf7e2 Bugfix: The var is LIBUNIVALUE,not LIBBITCOIN_UNIVALUE (Luke Dashjr)
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 74810B012346C9A6 Learn about signing commits
152a821

This was referenced Feb 8, 2016

Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment