Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 24, 2022
Merged

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

merged 2 commits into from
Oct 24, 2022

Conversation

fanquake
Copy link
Member

Backport of #25985 to the 22.x branch.

@fanquake
Revert "build: Use Homebrew's sqlite package if it is available"
bf42d7d 
This reverts ee7b84e from bitcoin#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 bitcoin#25724.

Resolves the "build from source" portion of bitcoin#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
bitcoin#25724 (comment),
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: bitcoin#25985
Rebased-From: d216d71
@fanquake fanquake mentioned this pull request Oct 20, 2022
Merged
0xB10C
0xB10C reviewed Oct 21, 2022
View reviewed changes
configure.ac
@@ -696,10 +696,6 @@ case $host in
BDB_LIBS="-L$bdb_prefix/lib -ldb_cxx-4.8"
fi

if test "x$use_sqlite" != xno && $BREW list --versions sqlite3 >/dev/null; then
Copy link
Contributor

@0xB10C 0xB10C Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

note: this is different from #25985 as fanquake@d6d402b had removed the x-prefix in the meantime

@0xB10C
Copy link
Contributor

0xB10C commented Oct 21, 2022

ACK bf42d7d

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

@hebasto
Copy link
Member

hebasto commented Oct 21, 2022 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Member

hebasto commented Oct 21, 2022 •
edited
Loading

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
Copy link
Member Author

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
Copy link
Member

ACK 8995df8

stickies-v
stickies-v approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK bf42d7d - 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 ee7b84e. 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:

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

On this PR, it outputs:

S["SQLITE_LIBS"]="-lsqlite3"

On 22.x, it outputs:

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

@stickies-v stickies-v mentioned this pull request Oct 21, 2022
Merged
@maflcko maflcko added this to the 22.1 milestone Oct 21, 2022
@fanquake fanquake force-pushed the revert_slow_macos_sqlite_22_x branch from 8995df8 to 63d2ee9 Compare October 24, 2022 01:36
hebasto
hebasto approved these changes Oct 24, 2022
View reviewed changes
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 63d2ee9, I have reviewed the code and it looks OK, I agree it can be merged.

stickies-v
stickies-v approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

re-ACK 63d2ee9

@maflcko maflcko merged commit 2d3161f into bitcoin:22.x Oct 24, 2022
@fanquake fanquake deleted the revert_slow_macos_sqlite_22_x branch October 24, 2022 10:03
@bitcoin bitcoin locked and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@0xB10C 0xB10C 0xB10C left review comments

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Backport Build system
Projects
None yet
Milestone
22.1
Development

Successfully merging this pull request may close these issues.
6 participants
@fanquake @0xB10C @hebasto @dergoegge @stickies-v @maflcko