build: Fix search for brew-installed BDB 4 on OS X #19356

gwillen commented at 2:42 AM on June 23, 2020: contributor

NOTE: This PR contains one important fix that I need (to make Bitcoin Core build cleanly on my system without shenanigans), plus some related general cleanup that is not really necessary, and could be annoying. (I am prepared to defend my argument that BDB_CFLAGS is wrong here, and BDB_CPPFLAGS is right, but this could bite anybody who has gotten in the habit of -- or scripted -- setting the former.)

Ok, I have been convinced that I was too clever with the refactor and I have removed it. Now it's just the tiny change to fix the build on my local machine.

On OS X, when searching Homebrew keg-only packages for BDB 4.8, if we find it, use BDB_CPPFLAGS and BDB_LIBS instead of CFLAGS and LIBS for the result. This is (1) more correct, and (2) necessary in order to give this location priority over other directories in the include search path, which may include system include directories with other versions of BDB.

fanquake added the label Build system on Jun 23, 2020

gwillen commented at 3:04 AM on June 23, 2020: contributor

(It looks like the use of BDB_CFLAGS for overrides was introduced in #9705, switching from CPPFLAGS. @laanwj, is there a reason you preferred BDB_CFLAGS over BDB_CPPFLAGS here, or that was just how things happened to land?)

DrahtBot commented at 4:51 AM on June 23, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Sjors commented at 9:02 AM on June 23, 2020: member

As suggested by @DrahtBot, #18870 is related, though it goes much further.

You may also want to rebase now that #19240 landed.

I have both berkeley-db@4 and berkeley-db 18.1.32 installed via homebrew. With or without this change, it picks up 4.8. I'm not sure what to test for.

gwillen commented at 10:05 AM on June 23, 2020: contributor

@Sjors: Thanks, per discussion with @theuni on IRC I'm going to rework the way I am doing this a bit anyway, so I'll rebase (thought it doesn't appear to conflict with #19240, fortunately.) I've been convinced that changing BDB_CFLAGS to BDB_CPPFLAGS, while tantalizing, is not actually correct.

As for testing -- in my case, I had bdb@4 unlinked, bdb 18 linked, and the result was that anything that touched the wallet would segfault. It was picking up bdb 18 headers from /usr/local/include, but bdb 4 libraries from /usr/local/opt/blahblahblah. After my change it correctly picks up bdb 4 only (for me.)

gwillen force-pushed on Jun 23, 2020

gwillen renamed this:
~~build: Fix & improve the search for BDB~~
build: Fix search for brew-installed BDB 4 on OS X
on Jun 23, 2020

gwillen commented at 11:47 AM on June 23, 2020: contributor

Ok, removed the refactor of BDB_CFLAGS, now it's just the tiny change to rearrange the include order so that we pick up the unlinked brew-installed BDB 4 consistently if present.

This does interact with #18870 a bit, but I think they're mostly orthogonal -- that one affects what we're looking for, whereas this one affects what we do with it if we find it.

in configure.ac:638 in 96ed2c7722 outdated

 634 | @@ -635,9 +635,12 @@ case $host in
 635 |  
 636 |           bdb_prefix=$($BREW --prefix berkeley-db4 2>/dev/null)
 637 |           qt5_prefix=$($BREW --prefix qt5 2>/dev/null)
 638 | -         if test x$bdb_prefix != x; then
 639 | -           CPPFLAGS="$CPPFLAGS -I$bdb_prefix/include"
 640 | -           LIBS="$LIBS -L$bdb_prefix/lib"
 641 | +         if test x$bdb_prefix != x && test "x$BDB_CFLAGS" = "x"; then

theuni commented at 1:42 PM on June 23, 2020:

This means that the user can't pass in a BDB_CFLAGS and also have the brew path appended. I guess that's fine though, I can't imagine a use-cases that would require both.

Though for completeness, we should make sure that BDB_LIBS is unset as well. Otherwise with something like "./configure BDB_LIBS=/foo/bar -ldb_cxx-4.8", the BDB_LIBS is completely ignored.

gwillen commented at 9:39 PM on June 23, 2020:

Yeah, that's true re: libs. I think it's incoherent to specify one and not the other, but in that case better for us to not be clever, and let the specified options go through. Will fix.

I think $DEPENDENCY_CFLAGS has no other obvious purpose than specifying include paths, in which case it wouldn't make sense for someone to specify it and also want to pick up the brew path -- you'd want the user path to override.

in configure.ac:642 in 96ed2c7722 outdated

 640 | -           LIBS="$LIBS -L$bdb_prefix/lib"
 641 | +         if test x$bdb_prefix != x && test "x$BDB_CFLAGS" = "x"; then
 642 | +           dnl This must precede the call to BITCOIN_FIND_BDB48 below.
 643 | +           BDB_CFLAGS="-I$bdb_prefix/include"
 644 | +           BDB_LIBS="-L$bdb_prefix/lib -ldb_cxx-4.8"
 645 | +           AC_SUBST(BDB_CFLAGS)

theuni commented at 1:45 PM on June 23, 2020:

The AC_SUBST's here shouldn't be necessary. We're switching to BDB_CFLAGS and BDB_LIBS specifically to match the vars in the m4. The AC_ARG_VAR's in there will take care of AC_SUBST.

gwillen commented at 9:39 PM on June 23, 2020:

Good point. I was originally trying to keep this standalone and effectively independent of BITCOIN_FIND_BDB48, but now that this is not possible, the SUBSTs are redundant. Will remove.

theuni changes_requested

theuni commented at 8:15 PM on June 23, 2020: member

Concept ACK. Requested a few little changes.

build: Fix search for brew-installed BDB 4 on OS X

On OS X, when searching Homebrew keg-only packages for BDB 4.8, if we find it,
use BDB_CPPFLAGS and BDB_LIBS instead of CFLAGS and LIBS for the result. This
is (1) more correct, and (2) necessary in order to give this location
priority over other directories in the include search path, which may include
system include directories with other versions of BDB.

8578c6fccd

gwillen force-pushed on Jun 24, 2020

gwillen commented at 5:09 AM on June 24, 2020: contributor

@theuni Comments addressed!

laanwj commented at 1:17 PM on June 24, 2020: member

(It looks like the use of BDB_CFLAGS for overrides was introduced in #9705, switching from CPPFLAGS. @laanwj, is there a reason you preferred BDB_CFLAGS over BDB_CPPFLAGS here, or that was just how things happened to land?)

I based it on some existing conventions in the build sytem. See ./configure --help. All the additional library-specific flags have _CFLAGS and _LIBS pairs. None has _CPPFLAGS at least.

theuni approved

theuni commented at 3:37 PM on June 24, 2020: member

Thanks for the quick fixups.

ACK 8578c6fccd11404412d2c60f9bede311b79ca0d0.

gwillen commented at 5:14 PM on June 24, 2020: contributor

(It looks like the use of BDB_CFLAGS for overrides was introduced in #9705, switching from CPPFLAGS. @laanwj, is there a reason you preferred BDB_CFLAGS over BDB_CPPFLAGS here, or that was just how things happened to land?)

I based it on some existing conventions in the build sytem. See ./configure --help. All the additional library-specific flags have _CFLAGS and _LIBS pairs. None has _CPPFLAGS at least.

Yeah, some IRC conversation after my initial PR convinced me that you were in fact correct to do so after all.

fanquake merged this on Jun 29, 2020

fanquake closed this on Jun 29, 2020

stevenroose referenced this in commit 78d4ea802a on Jul 6, 2020

DrahtBot locked this on Feb 15, 2022