[22.x] Revert "build: Use Homebrew's sqlite package if it is available"

fanquake commented at 6:04 am on October 20, 2022: member

Backport of #25985 to the 22.x branch.

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

This reverts ee7b84e63cbeadd5e680d69ff0548275581e9241 from #20527.
This change was made without any rationale, maybe other than a brew
installed version might be newer, and that's "better". However when
building from source on macOS, it just results in drastically worse
perofrmance, and results in issues / confusions like #25724.

Resolves the "build from source" portion of #25724. Building from
depends is still not ideal, however I have some other changes that might
help improve things in that case.

The difference in performance can be observed using the example from
https://github.com/bitcoin/bitcoin/issues/25724#issuecomment-1213554922,
but minified to only 10 descriptors. i.e:
```bash
time src/bitcoin-cli createwallet speedy true
time src/bitcoin-cli importdescriptors '[
  {"desc":"raw(00145846369f3d6ba366d6f5a903fb5cf4dca3763c0e)#k9wh6v62","timestamp":"now"},
  {"desc":"raw(001420800aabf13f3a4c4ce3ce4c66cecf1d17f21a6e)#6m0hlfh4","timestamp":"now"},
  {"desc":"raw(0014c6bf9715e06d73ebf9b3b02d5cc48d24d8bbabc1)#wyavh36r","timestamp":"now"},
  {"desc":"raw(00141ba7807b3f46af113beaea5c698428ce7138cd8a)#jctdsups","timestamp":"now"},
  {"desc":"raw(00140c1bd27f10fff01b36ddf3c1febaa1acff19b080)#9s6nc3pk","timestamp":"now"},
  {"desc":"raw(00141226e31987e4bc2e63c0ee12908f675e40464b20)#9pp7qm39","timestamp":"now"},
  {"desc":"raw(0014f73f149f7503960a5e849c6ee7a8a8c336f631cb)#qtkxv9fc","timestamp":"now"},
  {"desc":"raw(0014c8ccb4d81ffc769fc5fdd8d7eed69b0e0cae5749)#hn39qayv","timestamp":"now"},
  {"desc":"raw(001498565aead2d67a22a6021d55210f2a917fc22169)#6ar3vwsx","timestamp":"now"},
  {"desc":"raw(001403013248ac0cd9eabe176cad162cda2a19f771e1)#4m47mukd","timestamp":"now"}
]'
```

Running master, when building from souce and using brew installed
sqlite, this takes ~3.4s. With this PR, the same operation takes ~0.3s.

Github-Pull: #25985
Rebased-From: d216d714aae36e6f1c95f82aef81a0be74dee2f3

bf42d7de56

fanquake added the label Build system on Oct 20, 2022

fanquake added the label Backport on Oct 20, 2022

in configure.ac:699 in bf42d7de56 outdated

695@@ -696,10 +696,6 @@ case $host in
696            BDB_LIBS="-L$bdb_prefix/lib -ldb_cxx-4.8"
697          fi
698 
699-         if test "x$use_sqlite" != xno && $BREW list --versions sqlite3 >/dev/null; then

0xB10C commented at 9:39 am on October 21, 2022:

note: this is different from #25985 as https://github.com/fanquake/bitcoin/commit/d6d402bd2b8211e9e111acae433ca4e3cfd8d370 had removed the x-prefix in the meantime

0xB10C commented at 9:40 am on October 21, 2022: contributor

ACK bf42d7d

I’ve checked that this backports the change from #25985 to 22.x as it says it does.

hebasto commented at 9:43 am on October 21, 2022: member

https://github.com/bitcoin/bitcoin/blob/22.x/doc/build-osx.md#descriptor-wallet-support still saying brew install sqlite. With this backport, I believe the docs should be adjusted as well.

fanquake commented at 9:54 am on October 21, 2022: member

I believe the docs should be adjusted as well.

To what? The docs already say “You may not need to install this package.”, and the brew install portion is still correct if someone is on an older system without a useable sqlite, and they are passing *FLAGS through.

hebasto commented at 10:04 am on October 21, 2022: member

if someone is on an older system without a useable sqlite

v22.0.x supports macOS 10.14+, which means the system sqlite is available.

the brew install portion is still correct if … they are passing *FLAGS through.

Exactly. The instruction to brew install sqlite will have no effect without passing *FLAGS. That kind of notice should be mentioned in the docs. Or remove brew install sqlite altogether.

fanquake commented at 10:16 am on October 21, 2022: member

That kind of notice should be mentioned in the docs

There’s no need to mention this in our docs. It’s part of configure help.

Pushed a change up to drop the doc in any case.

dergoegge commented at 10:44 am on October 21, 2022: member

ACK 8995df8918668271592327141b2705a966401052

in doc/build-osx.md:126 in 8995df8918 outdated

121-`sqlite` is required to enable support for descriptor wallets.
122-Skip if you don't intend to use descriptor wallets.
123-
124-``` bash
125-brew install sqlite
126-```

stickies-v commented at 11:40 am on October 21, 2022:

Should we bring this in line with the current description instead?

stickies-v approved

stickies-v commented at 11:41 am on October 21, 2022: contributor

ACK bf42d7de56404b0afe23d81c6b758d1e9cffdf7b - I think maybe the doc commit could be improved as per my comment?

I verified that the change in configure.ac matches what was changed in ee7b84e63cbeadd5e680d69ff0548275581e9241. I also verified that, on macOS with sqlite3 installed with brew, 22.x uses the homebrew version and with this PR it uses the system version.

When running the below command:

0./autogen.sh
1./configure --enable-suppress-external-warnings --disable-bench --disable-tests --with-gui=no
2cat config.status | grep "sqlite"

On this PR, it outputs:

0S["SQLITE_LIBS"]="-lsqlite3"

On 22.x, it outputs:

0S["SQLITE_LIBS"]="-L/opt/homebrew/Cellar/sqlite/3.39.4/lib -lsqlite3"
1S["SQLITE_CFLAGS"]="-I/opt/homebrew/Cellar/sqlite/3.39.4/include"
2S["PKG_CONFIG_PATH"]="/opt/homebrew/opt/qt@5/lib/pkgconfig:/opt/homebrew/opt/sqlite/lib/pkgconfig:"

maflcko added this to the milestone 22.1 on Oct 21, 2022

doc: remove brew install sqlite from macOS docs 63d2ee9a50

fanquake force-pushed on Oct 24, 2022

hebasto approved

hebasto commented at 7:48 am on October 24, 2022: member

ACK 63d2ee9a50c5690df08f25e6840d10be8232f106, I have reviewed the code and it looks OK, I agree it can be merged.

stickies-v approved

stickies-v commented at 9:15 am on October 24, 2022: contributor

re-ACK 63d2ee9

maflcko merged this on Oct 24, 2022

maflcko closed this on Oct 24, 2022

fanquake deleted the branch on Oct 24, 2022

bitcoin locked this on Oct 24, 2023

[22.x] Revert “build: Use Homebrew’s sqlite package if it is available” #26350