Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Jan 8, 2015

Conversation

luke-jr
Copy link
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
Contributor

jgarzik commented Dec 30, 2014

lightly tested ACK

@laanwj
Copy link
Member

laanwj commented Jan 3, 2015

@theuni can you take a look here?

@theuni
Copy link
Member

theuni commented Jan 5, 2015

Looking into this.

@theuni
Copy link
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
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
@@ -500,6 +520,14 @@ if test x$use_upnp != xno; then
)
fi

if test x$build_bitcoin_utils$build_bitcoind$bitcoin_enable_qt$use_tests = xnonono; then
Copy link
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
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
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
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
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.

None yet

4 participants