Add SqlCipher - #199
Conversation
mr-smidge
left a comment
mr-smidge
left a comment
There was a problem hiding this comment.
Great initiative - a couple of comments.
| if(SYSTEM_SQLITE) | ||
| message(WARNING "EXPERIMENTAL_ENABLE_SQLCIPHER supersedes SYSTEM_SQLITE; the system SQLite option is ignored") | ||
| endif() |
There was a problem hiding this comment.
My intuition suggests that if we have -DEXPERIMENTAL_ENABLE_SQLCIPHER=ON and -DSYSTEM_SQLITE=ON at the same time, then this would use SQLCipher from the system / package repositories:
| SYSTEM_SQLITE | EXPERIMENTAL_ENABLE_SQLCIPHER | Meaning |
|---|---|---|
| ON | ON | Use system installation of SQLCipher. |
| ON | OFF | Use system installation of vanilla SQLite. |
| OFF | ON | Use vendored amalgamation of SQLCipher (added in this PR). |
| OFF | OFF | Use vendored amalgamation of vanilla SQLite. |
But the code highlighted above adds a different meaning for the ON/ON use case, i.e. it ignores SYSTEM_SQLITE and behaves like the OFF/ON case. I think that's confusing.
If we don't support ON/ON in this PR (which is a legitimate choice), I think an error message saying that the combination is not supported is more clear-cut than quietly changing the behaviour to something else.
| { os: ubuntu-24.04, c_compiler: clang-16, cpp_compiler: clang++-16, cmake_args: "-DEXPERIMENTAL_ENABLE_SQLCIPHER=ON" }, | ||
| { os: ubuntu-24.04, c_compiler: clang-17, cpp_compiler: clang++-17, cmake_args: "-DEXPERIMENTAL_ENABLE_SQLCIPHER=ON" }, | ||
| { os: ubuntu-24.04, c_compiler: gcc-13, cpp_compiler: g++-13, cmake_args: "-DEXPERIMENTAL_ENABLE_SQLCIPHER=ON" }, | ||
| { os: ubuntu-24.04, c_compiler: gcc-14, cpp_compiler: g++-14, cmake_args: "-DEXPERIMENTAL_ENABLE_SQLCIPHER=ON" }, |
There was a problem hiding this comment.
As an experimental feature, I think it might be better to only enable this for a few builds, and to not change all the 'stable' build configs already present.
For example, perhaps we just add one new build config for each OS (latest compiler) that uses SQLCipher for now?
2 participants
Follow up for #191
This PR vendors SqlCipher
4.16.0, updates the CMake build, and enables the flag in CI for linux and windows behind a flagEXPERIMENTAL_ENABLE_SQLCIPHER.