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

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:201129-sqlite changing 2 files +33 −11
  1. hebasto commented at 11:39 am on November 29, 2020: member

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

    This PR fixes this issue and update macOS build docs.

    Closes #20498.

  2. fanquake added the label Build system on Nov 29, 2020
  3. hebasto commented at 11:40 am on November 29, 2020: member
    Mind considering backporting into 0.21?
  4. DrahtBot commented at 1:45 pm on November 29, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #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.

  5. DrahtBot commented at 7:03 pm on November 30, 2020: member

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

  6. jonasschnelli commented at 10:11 am on December 1, 2020: contributor
    utACK ef7cd61a401f0c51afa0cb8c46271e2d7e9e9424
  7. jonasschnelli commented at 10:28 am on December 1, 2020: contributor
    Needs rebase (due to #20478)
  8. DrahtBot added the label Needs rebase on Dec 1, 2020
  9. hebasto force-pushed on Dec 1, 2020
  10. hebasto commented at 8:31 pm on December 1, 2020: member
    Rebased ef7cd61a401f0c51afa0cb8c46271e2d7e9e9424 -> 0479464ac938b3d51f7f38b19a20ed2a5dcf9c60 (pr20527.01 -> pr20527.02) due to the conflict with #20478.
  11. DrahtBot removed the label Needs rebase on Dec 1, 2020
  12. willcl-ark changes_requested
  13. willcl-ark commented at 12:42 pm on December 3, 2020: member

    Tested 0479464ac938b3d51f7f38b19a20ed2a5dcf9c60 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

  14. in doc/build-osx.md:25 in 0479464ac9 outdated 
    18@@ -19,7 +19,7 @@ Then install [Homebrew](https://brew.sh).
19 
20 ## Dependencies
21 ```shell
22-brew install automake berkeley-db4 libtool boost miniupnpc pkg-config python qt libevent qrencode sqlite
23+brew install automake libtool boost miniupnpc pkg-config python qt libevent qrencode
24 ```
25 
26 If you run into issues, check [Homebrew's troubleshooting page](https://docs.brew.sh/Troubleshooting).
    laanwj commented at 12:52 pm on December 3, 2020:
    It might be helpful to add a general note here that wallet support requires one or both of the dependencies in the sections below.
    hebasto commented at 3:23 pm on December 3, 2020:
    Thanks! Updated.
  15. hebasto commented at 2:47 pm on December 3, 2020: member

    @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?

  16. willcl-ark commented at 2:55 pm on December 3, 2020: member

    @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.

  17. willcl-ark approved
  18. hebasto force-pushed on Dec 3, 2020
  19. hebasto commented at 3:23 pm on December 3, 2020: member

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

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

  20. jonasschnelli approved
  21. jonasschnelli commented at 8:53 am on December 4, 2020: contributor
    code review ACK 7bf449ee5c4fb0748370981f1205c356162e58b4
  22. DrahtBot added the label Needs rebase on Dec 4, 2020
  23. build, refactor: Check that Homebrew's qt5 package is actually installed 
    This change unifies Homebrew packages workflow, and does not change
behavior.
    c96d1f65a5
  24. hebasto force-pushed on Dec 4, 2020
  25. hebasto commented at 11:19 am on December 4, 2020: member

    Updated 7bf449ee5c4fb0748370981f1205c356162e58b4 -> 03c65e4f704c4baa9bd72a56e8afdb92ec5d4d33 (pr20527.03 -> pr20527.04):

    • rebased due to the conflict with #20563
    • used uniform approach to check if Homebrew’s package is actually installed, that effectively implements @willcl-ark’s suggestion
  26. DrahtBot removed the label Needs rebase on Dec 4, 2020
  27. willcl-ark commented at 9:54 am on December 5, 2020: member

    tACK 03c65e4f704c4baa9bd72a56e8afdb92ec5d4d33

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

    0SQLITE_CFLAGS=''
1SQLITE_LIBS='-lsqlite3'

    vs building with brew sqlite3:

    0SQLITE_CFLAGS='-I/usr/local/Cellar/sqlite/3.33.0/include'
1SQLITE_LIBS='-L/usr/local/Cellar/sqlite/3.33.0/lib -lsqlite3'
  28. in configure.ac:652 in 03c65e4f70 outdated 
    651          fi
652-         if test x$qt5_prefix != x; then
653-           PKG_CONFIG_PATH="$qt5_prefix/lib/pkgconfig:$PKG_CONFIG_PATH"
654-           export PKG_CONFIG_PATH
655+
656+         if $BREW list --versions sqlite3 >/dev/null; then
    laanwj commented at 10:01 am on December 7, 2020:

    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.

    hebasto commented at 11:32 am on December 7, 2020:
    Thanks! Updated.
  29. build: Use Homebrew's sqlite package if it is available ee7b84e63c
  30. doc: Update wallet database installation guide for macOS c932e0d67e
  31. hebasto force-pushed on Dec 7, 2020
  32. hebasto commented at 11:31 am on December 7, 2020: member

    Updated 03c65e4f704c4baa9bd72a56e8afdb92ec5d4d33 -> c932e0d67e4b369e4265267da6c8bebac2b6fb53 (pr20527.04 -> pr20527.05, diff):

    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
  33. willcl-ark commented at 2:10 pm on December 7, 2020: member

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

    05390: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 c932e0d67e4b369e4265267da6c8bebac2b6fb53

  34. hebasto commented at 2:47 pm on December 7, 2020: member

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

    05390: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?

  35. willcl-ark commented at 8:13 pm on December 7, 2020: member

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

    05390: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!

  36. laanwj added the label Needs backport (0.21) on Dec 10, 2020
  37. laanwj commented at 12:04 pm on December 10, 2020: member
    Code review ACK c932e0d67e4b369e4265267da6c8bebac2b6fb53
  38. jonasschnelli approved
  39. jonasschnelli commented at 12:18 pm on December 10, 2020: contributor
    code review re-ACK c932e0d67e4b369e4265267da6c8bebac2b6fb53
  40. jonasschnelli merged this on Dec 10, 2020
  41. jonasschnelli closed this on Dec 10, 2020
  42. hebasto deleted the branch on Dec 10, 2020
  43. MarcoFalke referenced this in commit 48f8929aad on Dec 10, 2020
  44. MarcoFalke referenced this in commit f51e1cb291 on Dec 10, 2020
  45. MarcoFalke referenced this in commit 48134a09ad on Dec 10, 2020
  46. MarcoFalke commented at 1:14 pm on December 10, 2020: member
    Backported in #20612 because this has been marked for backport
  47. MarcoFalke removed the label Needs backport (0.21) on Dec 10, 2020
  48. sidhujag referenced this in commit 5a29c8ab68 on Dec 10, 2020
  49. DrahtBot locked this on Feb 15, 2022


hebasto DrahtBot jonasschnelli willcl-ark laanwj MarcoFalke

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me