Skip to content

Conversation

@fanquake
Copy link
Member

Move bdb cppflags out of the catch-all BITCOIN_INCLUDES, and pass them
only where they are needed, which is in libbitcoin_node/wallet and the tests.

@hebasto
Copy link
Member

hebasto commented May 30, 2022

Concept ACK.

Is guarding with USE_BDB condition really required? If it fails, the BDB_CPPFLAGS is just unset, no?

@laanwj
Copy link
Member

laanwj commented May 30, 2022

Concept ACK

Move bdb cppflags out of the catch-all BITCOIN_INCLUDES, and pass them
only where they are needed, which is in libbitcoin_node/wallet and the
tests.
@fanquake fanquake force-pushed the use_bdb_cppflags_needed branch from 6fd1f5e to 46a8909 Compare May 31, 2022 05:38
@fanquake
Copy link
Member Author

Is guarding with USE_BDB condition really required?

dropped

@fanquake fanquake requested a review from hebasto June 1, 2022 09:27
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 46a8909

The <db_cxx.h> included in the wallet/bdb.h only. Therefore, BDB_CPPFLAGS are required only for libraries which sources have #include <wallet/bdb.h>.

nit, the following diff

--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -177,6 +177,7 @@ FUZZ_SUITE_LD_COMMON +=\
 
 if USE_BDB
 BITCOIN_TESTS += wallet/test/db_tests.cpp
+test_test_bitcoin_CPPFLAGS += $(BDB_CPPFLAGS)
 endif
 
 FUZZ_WALLET_SRC = \
@@ -201,7 +202,6 @@ test_test_bitcoin_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(TESTDEFS) $(EV
 test_test_bitcoin_LDADD = $(LIBTEST_UTIL)
 if ENABLE_WALLET
 test_test_bitcoin_LDADD += $(LIBBITCOIN_WALLET)
-test_test_bitcoin_CPPFLAGS += $(BDB_CPPFLAGS)
 endif
 
 test_test_bitcoin_LDADD += $(LIBBITCOIN_NODE) $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) \

looks slightly more correct / clear?

@fanquake
Copy link
Member Author

fanquake commented Jun 1, 2022

looks slightly more correct / clear?

Probably not, given that diff wouldn't compile

@fanquake fanquake merged commit b752dad into bitcoin:master Jun 1, 2022
@fanquake fanquake deleted the use_bdb_cppflags_needed branch June 1, 2022 14:58
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jun 1, 2023
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.

3 participants