Skip to content

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

Merged
laanwj merged 4 commits intobitcoin:masterfrom
luke-jr:ilib_deps
Jan 8, 2015
Merged

Clean up internal library dependencies so we can avoid building unused code#5542
laanwj merged 4 commits intobitcoin:masterfrom
luke-jr:ilib_deps

Conversation

@luke-jr
Copy link
Copy Markdown
Member

@luke-jr luke-jr commented Dec 25, 2014

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

@luke-jr luke-jr force-pushed the ilib_deps branch 2 times, most recently from daa99ae to c936da2 Compare December 27, 2014 21:18
@jgarzik
Copy link
Copy Markdown
Contributor

jgarzik commented Dec 30, 2014

lightly tested ACK

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jan 3, 2015

@theuni can you take a look here?

@theuni
Copy link
Copy Markdown
Member

theuni commented Jan 5, 2015

Looking into this.

@theuni
Copy link
Copy Markdown
Member

theuni commented Jan 5, 2015

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 luke-jr force-pushed the ilib_deps branch 2 times, most recently from a08cf67 to 44f1ff7 Compare January 6, 2015 13:39
@luke-jr
Copy link
Copy Markdown
Member Author

luke-jr commented Jan 6, 2015

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 luke-jr force-pushed the ilib_deps branch 2 times, most recently from e5bfbd0 to 3e477cd Compare January 6, 2015 14:00
configure.ac Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BITCOIN_QT_CONFIGURE needs to be run before we know about bitcoin_enable_qt.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@laanwj
Copy link
Copy Markdown
Member

laanwj commented Jan 7, 2015

Looks good to me.
Tested ACK (for 0.10)

@laanwj laanwj added this to the 0.10.0 milestone Jan 7, 2015
@luke-jr luke-jr force-pushed the ilib_deps branch 3 times, most recently from d36beee to 6526c44 Compare January 7, 2015 15:09
@luke-jr
Copy link
Copy Markdown
Member Author

luke-jr commented Jan 7, 2015

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

@theuni
Copy link
Copy Markdown
Member

theuni commented Jan 7, 2015

Thanks for this! ut ACK if travis is happy.

@laanwj laanwj merged commit 2ecd294 into bitcoin:master Jan 8, 2015
laanwj added a commit that referenced this pull request Jan 8, 2015
2ecd294 Bugfix: configure: Correctly detect "nothing to build" condition (Luke Dashjr)
b7a4ecc Bugfix: Only check for boost when building code that requires it (Luke Dashjr)
a19eeac Bugfix: configure: Check for openssl/ec.h (Luke Dashjr)
fe925e2 Use EXTRA_LIBRARIES instead of noinst_LIBRARIES so we can avoid building unused code (Cory Fields)
laanwj added a commit that referenced this pull request Jan 8, 2015
2ecd294 Bugfix: configure: Correctly detect "nothing to build" condition (Luke Dashjr)
b7a4ecc Bugfix: Only check for boost when building code that requires it (Luke Dashjr)
a19eeac Bugfix: configure: Check for openssl/ec.h (Luke Dashjr)
fe925e2 Use EXTRA_LIBRARIES instead of noinst_LIBRARIES so we can avoid building unused code (Cory Fields)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants