build: Do not ignore Homebrew's SQLite on macOS #20527

hebasto · 2020-11-29T11:39:34Z

On master (7ae86b3) installed Homebrew sqlite package is ignored during build on macOS.

This PR fixes this issue and update macOS build docs.

Closes #20498.

hebasto · 2020-11-29T11:40:50Z

Mind considering backporting into 0.21?

DrahtBot · 2020-11-29T13:45:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2020-11-30T19:03:20Z

🕵️ @harding @fanquake have been requested to review this pull request as specified in the CODEOWNERS file.

jonasschnelli · 2020-12-01T10:11:02Z

utACK ef7cd61

jonasschnelli · 2020-12-01T10:28:59Z

Needs rebase (due to #20478)

hebasto · 2020-12-01T20:31:56Z

Rebased ef7cd61 -> 0479464 (pr20527.01 -> pr20527.02) due to the conflict with #20478.

willcl-ark

Tested 0479464 on macOS 11.0.1 with Homebrew 2.6.0

Verified that homebrews sqlite is auto-selected after installation and builds sucessfully.

Interestingly, after uninstalling brews sqlite I still found references to ghost homebrew libraries in the Makefile because of the way brew --prefix sqlite3 works: https://github.com/Homebrew/brew/blob/3821d37997a7b385cc97bb51169de56f641d5c39/Library/Homebrew/cmd/--prefix.rb#L23

It means that if brew is installed but brew's sqlite is not, then brew --prefix sqlite will still return where it would install it, and having this path added to the Makefile accordingly.

I have tested that installation does indeed still pass in this case, but it might still be worth testing the sqlite prefix and then the presence of the binary/bin folder itself to avoid a future issue, e.g.:

test ! -d $(brew --prefix sqlite)/bin

doc/build-osx.md

hebasto · 2020-12-03T14:47:45Z

@willcl-ark

It means that if brew is installed but brew's sqlite is not, then brew --prefix sqlite will still return where it would install it, and having this path added to the Makefile accordingly.

I think the code in this PR handles this case fine, as it actually adds a non-existent path to the PKG_CONFIG_PATH, that in turn effectively is noop, right?

willcl-ark · 2020-12-03T14:55:50Z

@willcl-ark

It means that if brew is installed but brew's sqlite is not, then brew --prefix sqlite will still return where it would install it, and having this path added to the Makefile accordingly.

I think the code in this PR handles this case fine, as it actually adds a non-existent path to the PKG_CONFIG_PATH, that in turn effectively is noop, right?

I'm not that experienced with Makefiles' path interpreter but agree that having that extra (non-existant) path in PKG_CONFIG_PATH did not seem to affect the build in any way when brews sqlite3 was not installed.

hebasto · 2020-12-03T15:23:19Z

Updated 0479464 -> 7bf449e (pr20527.02 -> pr20527.03, diff):

addressed @laanwj's comment:

It might be helpful to add a general note here that wallet support requires one or both of the dependencies in the sections below.

jonasschnelli

code review ACK 7bf449e

This change unifies Homebrew packages workflow, and does not change behavior.

hebasto · 2020-12-04T11:19:48Z

Updated 7bf449e -> 03c65e4 (pr20527.03 -> pr20527.04):

rebased due to the conflict with build: Check that Homebrew's berkeley-db4 package is actually installed #20563
used uniform approach to check if Homebrew's package is actually installed, that effectively implements @willcl-ark's suggestion

willcl-ark · 2020-12-05T09:54:55Z

tACK 03c65e4

Building without brew sqlite3 (but with brew installed) now leaves no trace in config.log:

SQLITE_CFLAGS=''
SQLITE_LIBS='-lsqlite3'

vs building with brew sqlite3:

SQLITE_CFLAGS='-I/usr/local/Cellar/sqlite/3.33.0/include'
SQLITE_LIBS='-L/usr/local/Cellar/sqlite/3.33.0/lib -lsqlite3'

configure.ac

hebasto · 2020-12-07T11:31:50Z

Updated 03c65e4 -> c932e0d (pr20527.04 -> pr20527.05, diff):

addressed @laanwj's comment:

Shouldn't this, in principle, be conditional on sqlite3 support actually being requested? (like the use_bdb above, and same for qt5 below)

I'm sure it works but I think it leaves some scope for unexpected differences in behavbior if the build configuration unconditionally depends on the system instead of what the user asks.

(minor) improved robustness of use_bdb check

willcl-ark · 2020-12-07T14:10:37Z

With brew sqlite installed, running ./configure --with-sqlite=no I still see a single artefact of sqlite in config.log:

5390:PKG_CONFIG_PATH='/usr/local/opt/qt/lib/pkgconfig:/usr/local/opt/sqlite/lib/pkgconfig:

It does not appear to impact the build, but I am curious how it's ending up there, as the code would suggest this would only be prepended to PKG_CONFIG_PATH when if test "x$use_sqlite" != xno && $BREW list --versions sqlite3 >/dev/null is satisfied?

That said, another tACK of c932e0d

hebasto · 2020-12-07T14:47:31Z

With brew sqlite installed, running ./configure --with-sqlite=no I still see a single artefact of sqlite in config.log:
5390:PKG_CONFIG_PATH='/usr/local/opt/qt/lib/pkgconfig:/usr/local/opt/sqlite/lib/pkgconfig:
It does not appear to impact the build, but I am curious how it's ending up there, as the code would suggest this would only be prepended to PKG_CONFIG_PATH when if test "x$use_sqlite" != xno && $BREW list --versions sqlite3 >/dev/null is satisfied?

That said, another tACK of c932e0d

Hmm, did you run ./autogen.sh before ./configure?

willcl-ark · 2020-12-07T20:13:13Z

With brew sqlite installed, running ./configure --with-sqlite=no I still see a single artefact of sqlite in config.log:
5390:PKG_CONFIG_PATH='/usr/local/opt/qt/lib/pkgconfig:/usr/local/opt/sqlite/lib/pkgconfig:
It does not appear to impact the build, but I am curious how it's ending up there, as the code would suggest this would only be prepended to PKG_CONFIG_PATH when if test "x$use_sqlite" != xno && $BREW list --versions sqlite3 >/dev/null is satisfied?
That said, another tACK of c932e0d
Hmm, did you run ./autogen.sh before ./configure?

@hebasto you are right, re-running autogen.sh sorted it!

laanwj · 2020-12-10T12:04:49Z

Code review ACK c932e0d

jonasschnelli

code review re-ACK c932e0d

This change unifies Homebrew packages workflow, and does not change behavior. Github-Pull: bitcoin#20527 Rebased-From: c96d1f6

Github-Pull: bitcoin#20527 Rebased-From: ee7b84e

Github-Pull: bitcoin#20527 Rebased-From: c932e0d

maflcko · 2020-12-10T13:14:50Z

Backported in #20612 because this has been marked for backport

fanquake added the Build system label Nov 29, 2020

hebasto mentioned this pull request Nov 29, 2020

Q: Compiling with SQLite on macOS #20498

Closed

DrahtBot mentioned this pull request Nov 29, 2020

Don't set BDB flags when configuring without #20478

Merged

maflcko mentioned this pull request Nov 30, 2020

doc: rename CODEOWNERS to REVIEWERS #20200

Merged

DrahtBot added the Needs rebase label Dec 1, 2020

hebasto force-pushed the 201129-sqlite branch from ef7cd61 to 0479464 Compare December 1, 2020 20:29

DrahtBot removed the Needs rebase label Dec 1, 2020

willcl-ark suggested changes Dec 3, 2020

View reviewed changes

laanwj reviewed Dec 3, 2020

View reviewed changes

doc/build-osx.md Show resolved Hide resolved

willcl-ark approved these changes Dec 3, 2020

View reviewed changes

hebasto force-pushed the 201129-sqlite branch from 0479464 to 7bf449e Compare December 3, 2020 15:19

DrahtBot mentioned this pull request Dec 4, 2020

build: Check that Homebrew's berkeley-db4 package is actually installed #20563

Merged

jonasschnelli approved these changes Dec 4, 2020

View reviewed changes

DrahtBot added the Needs rebase label Dec 4, 2020

build, refactor: Check that Homebrew's qt5 package is actually installed

c96d1f6

This change unifies Homebrew packages workflow, and does not change behavior.

hebasto force-pushed the 201129-sqlite branch from 7bf449e to 03c65e4 Compare December 4, 2020 11:15

DrahtBot removed the Needs rebase label Dec 4, 2020

laanwj reviewed Dec 7, 2020

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

build: Use Homebrew's sqlite package if it is available

ee7b84e

doc: Update wallet database installation guide for macOS

c932e0d

hebasto force-pushed the 201129-sqlite branch from 03c65e4 to c932e0d Compare December 7, 2020 11:27

DrahtBot mentioned this pull request Dec 7, 2020

net: Add NAT-PMP port forwarding support #18077

Merged

laanwj added the Needs backport (0.21) label Dec 10, 2020

jonasschnelli approved these changes Dec 10, 2020

View reviewed changes

jonasschnelli merged commit 86f2007 into bitcoin:master Dec 10, 2020

hebasto deleted the 201129-sqlite branch December 10, 2020 12:24

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 10, 2020

build: Use Homebrew's sqlite package if it is available

f51e1cb

Github-Pull: bitcoin#20527 Rebased-From: ee7b84e

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Dec 10, 2020

doc: Update wallet database installation guide for macOS

48134a0

Github-Pull: bitcoin#20527 Rebased-From: c932e0d

maflcko removed the Needs backport (0.21) label Dec 10, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2020

Merge bitcoin#20527: build: Do not ignore Homebrew's SQLite on macOS

5a29c8a

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

build: Do not ignore Homebrew's SQLite on macOS #20527

build: Do not ignore Homebrew's SQLite on macOS #20527

Uh oh!

Conversation

hebasto commented Nov 29, 2020

Uh oh!

hebasto commented Nov 29, 2020

Uh oh!

DrahtBot commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

DrahtBot commented Nov 30, 2020

Uh oh!

jonasschnelli commented Dec 1, 2020

Uh oh!

jonasschnelli commented Dec 1, 2020

Uh oh!

hebasto commented Dec 1, 2020

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Dec 3, 2020

Uh oh!

willcl-ark commented Dec 3, 2020

Uh oh!

hebasto commented Dec 3, 2020

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Dec 4, 2020

Uh oh!

willcl-ark commented Dec 5, 2020

Uh oh!

Uh oh!

hebasto commented Dec 7, 2020

Uh oh!

willcl-ark commented Dec 7, 2020

Uh oh!

hebasto commented Dec 7, 2020

Uh oh!

willcl-ark commented Dec 7, 2020

Uh oh!

laanwj commented Dec 10, 2020

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 10, 2020

Uh oh!

Uh oh!

DrahtBot commented Nov 29, 2020 •

edited

Loading