Clean up internal library dependencies so we can avoid building unused code #5542

luke-jr commented at 11:56 AM on December 25, 2014: member

Without this, we unnecessarily compile libbitcoin_server for utils-only configurations.

luke-jr force-pushed on Dec 27, 2014

jgarzik commented at 3:56 PM on December 30, 2014: contributor

lightly tested ACK

jgarzik added the label Build system on Dec 30, 2014

laanwj commented at 9:35 AM on January 3, 2015: member

@theuni can you take a look here?

theuni commented at 7:09 PM on January 5, 2015: member

Looking into this.

theuni commented at 8:34 PM on January 5, 2015: member

Concept ACK, but I think this approach is overly-complicated. Since the binaries know their own deps, there should be no need to explicitly tell the convenience libs to build or not build. The current problem is that noinst_LIBRARIES are always built, whether or not they're needed. Instead, let's just let Make decide if/when they bulld. EXTRA_LIBRARIES can be used for exactly that.

This should be the only change needed:

diff --git a/src/Makefile.am b/src/Makefile.am
index d6ac6e1..81b16d1 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -37,7 +37,7 @@ $(LIBSECP256K1): $(wildcard secp256k1/src/*) $(wildcard secp256k1/include/*)

 # Make is not made aware of per-object dependencies to avoid limiting building parallelization
 # But to build the less dependent modules first, we manually select their order here:
-noinst_LIBRARIES = \
+EXTRA_LIBRARIES = \
   crypto/libbitcoin_crypto.a \
   libbitcoin_util.a \
   libbitcoin_common.a \
@@ -46,7 +46,7 @@ noinst_LIBRARIES = \
   libbitcoin_cli.a
 if ENABLE_WALLET
 BITCOIN_INCLUDES += $(BDB_CPPFLAGS)
-noinst_LIBRARIES += libbitcoin_wallet.a
+EXTRA_LIBRARIES += libbitcoin_wallet.a
 endif

 if BUILD_BITCOIN_LIBS
diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 1192b7b..cdd8f8d 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -1,5 +1,5 @@
 bin_PROGRAMS += qt/bitcoin-qt
-noinst_LIBRARIES += qt/libbitcoinqt.a
+EXTRA_LIBRARIES += qt/libbitcoinqt.a

 # bitcoin qt core #
 QT_TS = \

With this change, building with --without-gui --without-daemon --disable-tests, the server lib is not built. Likewise, with --without-gui --without-daemon --disable-tests --without-utils, only the consensus lib is built.

This is significantly easier to maintain, since we don't have to keep up with the tangled web of convenience lib dependencies.

luke-jr force-pushed on Jan 6, 2015

luke-jr commented at 1:41 PM on January 6, 2015: member

Rewrote based on @theuni 's version (nice!), but also fixed configure to tolerate missing boost for lib-only builds, and check for openssl/ec.h (missing on Gentoo if OpenSSL is built with "bindist" USE flag).

luke-jr force-pushed on Jan 6, 2015

Use EXTRA_LIBRARIES instead of noinst_LIBRARIES so we can avoid building unused code fe925e221f

luke-jr force-pushed on Jan 6, 2015

in configure.ac:None in d7c9c92289 outdated

 519 | @@ -500,6 +520,14 @@ if test x$use_upnp != xno; then
 520 |    )
 521 |  fi
 522 |  
 523 | +if test x$build_bitcoin_utils$build_bitcoind$bitcoin_enable_qt$use_tests = xnonono; then

theuni commented at 3:05 AM on January 7, 2015:

BITCOIN_QT_CONFIGURE needs to be run before we know about bitcoin_enable_qt.

luke-jr commented at 12:06 PM on January 7, 2015:

fixed

luke-jr force-pushed on Jan 7, 2015

laanwj commented at 1:00 PM on January 7, 2015: member

Looks good to me. Tested ACK (for 0.10)

laanwj added this to the milestone 0.10.0 on Jan 7, 2015

luke-jr force-pushed on Jan 7, 2015

Bugfix: configure: Check for openssl/ec.h a19eeaced8

luke-jr force-pushed on Jan 7, 2015

Bugfix: Only check for boost when building code that requires it b7a4ecc153

luke-jr force-pushed on Jan 7, 2015

Bugfix: configure: Correctly detect "nothing to build" condition 2ecd2941ed

luke-jr commented at 8:24 PM on January 7, 2015: member

Added another bugfix and retested here (incl boost-less and ec-less systems). I think this is finally done.

theuni commented at 9:11 PM on January 7, 2015: member

Thanks for this! ut ACK if travis is happy.

laanwj merged this on Jan 8, 2015

laanwj closed this on Jan 8, 2015

laanwj referenced this in commit 204d41a821 on Jan 8, 2015

laanwj referenced this in commit 7fdbedcaf8 on Jan 8, 2015

MarcoFalke locked this on Sep 8, 2021