build: Make sqlite support optional (compile-time) #20156
Conversation
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Concept ACK.
Sqlite is already optional in depends using NO_SQLITE=1, but configure will still look for it (without this PR).
I prefer to disable the creation of descriptor wallets if this flag is used (createwallet RPC and gui). That gives us the flexibility to use native Sqlite stuff for those wallets in the future, without having to worry about BDB upgrade paths.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Making bdb optional would be more interesting looking forward. But no problem with this either, concept ACK. |
|
Improved configure help per request, and added nicer behaviour in RPC/GUI around descriptor wallet stuff. (A future PR can remove descriptor wallet code entirely if desired.) |
|
tACK f9b93a0 on macOS (though I couldn't test the scenario of not having a sqlite library at all on the system) |
|
Concept ACK. |
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
Approach ACK f9b93a0053476055759124160efa87978d6b635f
…te support not compiled in
… and sqlite support not compiled in
|
Assert added, checkbox forced off, and help message made to match others |
|
Concept ACK – however, I agree with @laanwj that making BerkleyDB optional could be more interesting for many users. |
|
@jonasschnelli I don't disagree, it's just outside the scope of this PR. Hopefully once it's merged, someone (maybe me) will extend it to BDB and we can discuss if it belongs in 0.21 or 0.22. :) |
|
Would detecting an sqlite wallet (look for the sqlite magic) make sense to give more specific errors in case someone tries to load a sqlite wallet with a non-sqlite-supporting bitcoind/qt? (could be a follow up PR is complicated). Current error in the deamon... and the GUI: |
|
Tested ACK bbb42a6 |
|
@jonasschnelli Added nicer error messages to the possible followup list |
|
ACK bbb42a6 |
|
re-utACK bbb42a6 |
Guix builds
|
laanwj
changed the title
bbb42a6 RPC: createwallet: Nicer error message if descriptor wallet requested and sqlite support not compiled in (Luke Dashjr) 6608fec GUI: Create Wallet: Nicely disable descriptor wallet checkbox if sqlite support not compiled in (Luke Dashjr) 7b54d76 Make sqlite support optional (compile-time) (Luke Dashjr) Pull request description: As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21. Potential follow-up PRs after this: * Make BDB support optional * Nicer error messages when user tries to load an unsupported wallet * Don't compile descriptor wallet code if sqlite disabled ACKs for top commit: jonasschnelli: Tested ACK bbb42a6 achow101: ACK bbb42a6 Sjors: re-utACK bbb42a6 hebasto: ACK bbb42a6, tested on Linux Mint 20 (x86_64, Qt 5.12.8). Tree-SHA512: 500209dd1971310fab8ae51543343ce0ba91f088ccccff6109b4cc27547cd5532289dca6cb7dac2a7d7c59cdf3c8f5aacc31e9b0f912e38cea52ec26b97100bd
7411876 Ensure a legacy wallet for BDB format check (Andrew Chow) 5866403 Skip --descriptor tests if sqlite is not compiled (Andrew Chow) Pull request description: #20156 allows sqlite to not be compiled by configuring `--without-sqlite`. However doing so and then running the test runner will result in all of the `--descriptor` tests to fail. We should be skipping those tests if sqlite was not compiled. ACKs for top commit: practicalswift: ACK 7411876: patch looks correct Sjors: tACK 7411876 ryanofsky: Code review ACK 7411876 hebasto: ACK 7411876, tested on Linux Mint 20 (x86_64), tests pass for binaries compiled with: Tree-SHA512: 1d635a66d2b7bb865300144dfefcfdaf86133aaaa020c8f440a471476ac1205d32f2df704906ce6c2ea48ddf791c3c95055f6291340b4f7b353c1b02cab5cabe
d52f502 Fix mock SQLiteDatabases (Andrew Chow) 99309ab Allow disabling BDB in configure with --without-bdb (Andrew Chow) ee47f11 GUI: Force descriptor wallets when BDB is not compiled (Andrew Chow) 71e40b3 RPC: Require descriptors=True for createwallet when BDB is not compiled (Andrew Chow) 6ebc41b Enforce salvage is only for BDB wallets (Andrew Chow) a58b719 Do not compile BDB things when USE_BDB is defined (Andrew Chow) b33af48 Include wallet/bdb.h where it is actually being used (Andrew Chow) Pull request description: Adds a `--without-bdb` option to `configure` which disables the compilation of the BDB stuff. Legacy wallets will not be created when BDB is not compiled. A legacy-sqlite wallet can be loaded, but we will not create them. Based on #20156 to resolve the situation where both `--without-sqlite` and `--without-bdb` are provided. In that case, the wallet is disabled and `--disable-wallet` is effectively set. ACKs for top commit: laanwj: Code review ACK d52f502 Tree-SHA512: 5a92ba7a542acc2e27003e9d4e5940e0d02d5c1f110db06cdcab831372bfd83e8d89c269caff31dd5bff062c1cf5f04683becff12bd23a33be731676f346553d
9 participants
As a new requirement, sqlite support should be optional. This PR aims to be only minimum/blocker changes for 0.21.
Potential follow-up PRs after this: