-
Notifications
You must be signed in to change notification settings - Fork 38.7k
[23.x] Revert "build: Use Homebrew's sqlite package if it is available" #26333
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.
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
Conversation
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
|
ACK 419bdc5 |
stickies-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 419bdc5
I verified that the change matches #25985. I also verified that, on macOS with sqlite3 installed with brew, 23.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 23.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:"|
After reviewing #26350, I'm not sure why this backport doesn't do the same docs update? Seems to be like they should be the same? |
|
Did you mean "23.1"? |
hebasto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 7698366, I have reviewed the code and it looks OK, I agree it can be merged.
stickies-v
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK 7698366

Backport of #25985 to the 23.x branch.