-
-
Notifications
You must be signed in to change notification settings - Fork 790
Support SQLCipher using package traits #1708
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
base: development
Are you sure you want to change the base?
Conversation
Update: this was simply because I wasn't building in release mode. See comment below, where the SQLCipher build is actually 10% faster. |
@marcprux the Android NDK doesn't seem to bundle sqlite3.h, whereas GRDB and your PR do rely on that being available. How are you providing that right now when building and testing GRDB? |
It is included with the SQLCipher source module: https://github.com/skiptools/swift-sqlcipher/tree/main/Sources/SQLCipher/sqlite |
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.
Hello @marcprux,
I like that very much. This pull request is a great gift 🎁! I'll shortly play with the new targets.
I made a first review and preliminary comments. I hope we agree on the broad outlines!
Isn't it just the cipher? (EDIT: I'm wrong because performance tests don't run on encrypted databases, see comment below.) SQLCipher can also be extremely slow when it opens a connection. Some people have reported up to 0.5 seconds (😱). To the point I had to ship #1350. |
I ran the same performance tests, but this time in release configuration:
This time, SQLCipher tests are faster. |
Agreed, but the performance hit should only occur when encryption is actually enabled on the database, which shouldn't affect any of the perf tests.
Of course! Foolish of me to not do that. Otherwise, it is comparing the debug build of the sqlcipher/sqlite source build against the release build of the vendored sqlite. It is heartening to see that superior performance can be squeezed out of a source build. I wonder how much more might be managed with strategic performance-related flags (e.g., sbooth/CSQLite#59 (comment)). |
…abilites on non-Dawin platforms missing XCTIssue
… rather than disabiling them altogether
Apologies for the long delay in addressing the feedback (I was traveling). I believe I've handled each of the requested changes. Let me know if I can do anything else to help get this PR over the finish line – it has grown quite large, so I'm nervous that rebasing is going to be difficult if it starts to become stale. |
We've been using this branch successfully. |
I'll follow up on the list of needs at https://mastodon.social/@groue@hachyderm.io/113990450491505387 here, just to keep it in the PR record.
This should work out of the box for anyone that just changes: dependencies: [
.package(url: "https://github.com/groue/GRDB.swift.git", from: "7.0.0"),
] to the (theoretical) locally-maintained fork: dependencies: [
.package(url: "https://github.com/groue/GRDB-SQLCipher.swift.git", from: "7.0.0"),
]
You are welcome to relocate https://github.com/skiptools/swift-sqlcipher.git into the
This isn't part of this PR, but it would only be a couple of lines in the README.
This is part of the PR. The new
The tests do run against SQLCipher, but against an unencrypted database. I didn't see how this is being done with SQLCipher for CocoaPods, but I suppose we could just do the same setup here (perhaps just based on checking the
This shouldn't be an issue. Just activate (i.e., un-comment) the
For the SQLCipher dependency, you just need to make a new amalgamated sqlite.c from sqlcipher, commit it, and then tag a release. This is done with the script: https://github.com/skiptools/swift-sqlcipher/blob/main/scripts/build_sqlcipher.sh An example of this sort of update commit is the bump to sqlite 3.46.1 and sqlcipher 4.6.1: skiptools/swift-sqlcipher@5ea2f8b
Update: the fork is no longer necessary when using package traits (see comment below) |
I'm sorry I did not answer yet. I had to cope with the problems created with Xcode 16.3 beta. I'll get back to you soon. |
This PR saved me today while integrating a closed source framework which links SQLCipher while we were already extensively using grdb. |
I've updated this PR to use the new package traits in Swift 6.1, which seem designed for a scenario exactly like this. Now, rather than having to create and maintain a custom GRDB-SQLCipher fork that enables GRDBCIPHER, a consumer of this package can simply enable the
Adding the trait to the dependency will activate the For the purpose of testing and CI, the "GRDBCIPHER" environment variable is still checked, but only to see whether to enable the trait by default in the
The performance test environment can also still be used, which demonstrates that the custom SQLCipher build continues to be ~10% faster than the vendored SQLite3, at least on my machine:
The only drawback is that since traits are simple booleans, you can no longer specify a custom repository with the "GRDBCIPHER" environment variable, so you need to depend on a specific repository. But an advantage is that the |
@groue Hoping you can take another look at this PR with the new Package traits dependency. I think it really improves on the original idea. |
We need this for our work, so I'll maintain GRDB with SQLCipher support in our fork at https://github.com/swift-everywhere/grdb-sqlcipher.git and track upstream releases, starting with v7.5.0. |
Hello, sorry for being late to the party. I'm slow on those topics because I'm really not excited by build topics. Let's discuss traits! The Trait unification chapter of the proposal says:
A SQLCipher trait can not be 100% compatible with the SQLite version that ships on Apple operating systems (the default trait on Apple OS). We just have to wait for the two SQLite version to ship with a different set of compile options, or for the operating system to ship a newer SQLite version with more features. In case the problem is not clear, we just need:
With bad luck, API X does not work with SQLCipher, and has no replacement (linker error, runtime error, whatever, SOME ERROR). The application owner complains to the Dependency1 maintainers, who say they're not responsible (they are correct). Owner complains to Dependency2, who say they require SQLCipher (perfectly legit). What are the options of the application owner? All painful. They are entitled to turn back to this repository, complaining that this is a misuse of package traits (and they would be correct: GRDB should not have exposed SQLCipher as a trait, to reuse the emphasized terminology of the proposal). And I'm not even talking about the Thoughts? |
A diamond dependency pattern does indeed have the potential to introduce merged trait conflicts. But I envision that the preference for which SQLite runtime to use (vendored SQLite3 vs. custom SQLCipher) would be something generally decided at the application level, and so the trait declarations would be pushed up as high as possible. Dependency1 and Dependency2 ought not mandate any particular compile options, but instead adapt to the SQLite runtime in use, such as by introspecting on func encryptDatabase() throws {
if try exec("pragma COMPILE_OPTIONS").contains("SQLCIPHER") == false {
throw RuntimeError("Unsupported database configuration…")
}
} I'm not sure how many middleware packages like this are out there currently, other than sharing-grdb. If you know of any, it would be interesting to examine them to see if they presume any particular compile options. Also, I'll note that the potential issue of compile option mismatch already exists on Linux: the
Yes, well, that would have to be discouraged. If GRDB were to re-export the SQLite3 or SQLCipher module, then a dependent package ought to just be able to pick up the symbols from there without needing to ever |
Thanks @marcprux for your careful consideration. This is well appreciated. What I envision is that users wildly exploit any possible technique. That's my rule of thumb, and it's been quite efficient so far. I do not assume that the preference will be decided at the application level. It is reasonable to imagine a third-party library that encrypts its private data and sets the SQLCipher trait. We do not have to wonder if conflicts can happen: they will. And runtime checks can't fix linker errors. The correct attitude is vigilant optimism - and probably a fair amount of advice and empathy in the documentation. On the topic of exposing the C SQLite APIs, which is an important GRDB feature - several people depend on it:
That's what GRDB was doing until GRDB 7. There were too many reports of build errors, so I shipped #1600, forcing users to write explicit imports of their desired SQLite flavor. Since then, the flow of build error reports has nearly stopped. As far as I can tell, the root cause is probably an Xcode bug. What I mean here is that I can't just re-export and call it a day. In an ideal world, this would work. But it does not, for reasons I can't address, and I'm unable to report (the error does happen, I saw it, but it is very difficult to reproduce, and it's not only a matter of cleaning caches). Finally, there's the "official" SQLCipher package, on which you have made several comments. If I understand well, the big problem is that a binary distribution would not work on all platforms, and that's why you suggest a source distribution. I agree with you. Why did you pick https://github.com/skiptools/swift-sqlcipher? What are the other options? If there's a bias due to your focus on Android, would you please make it explicit, and describe the consequences of this choice for other platforms? So… Using package traits for SQLCipher will create a new flow of issues. And this is not a subject I know enough about. I do not want open issues to pile up, especially when the issue lies elsewhere, and I'm unable to prove it. I could move all those issues to discussions (some repos do that in order to keep the number of issues small), but that does not address anything. Finally I'm slow to address those topics, as everybody can see. In the interest of all, I wish someone would volunteer for the position of “build support” in GRDB. |
I picked it simply because that is a package that I had already put together to work on Android. Any buildable SQLCipher amalgamation would work, and as I've mentioned before, you could simply pull it into the GRDB package as a C module in order to eliminate the need for an external dependency (which I think would be the ideal solution). I would be happy to expand this PR to provide that option, but it would introduce the potential maintenance burden of keeping up with subsequent SQLCipher releases, which I sense you aren't interested in.
You would be correct in thinking that the two issues (SQLCipher support and Android support) are orthogonal. They just happen to both be solvable through this PR. My ultimate goal is to bring this support to all platforms, including Android. GRDB currently doesn't work at all on Android due to there not being any linkable sqlite library on the platform (unlike Linux), so a source build is currently the only solution for that platform (irrespective of whether the source build is sqlcipher or just a stock sqlite package like https://github.com/sbooth/CSQLite or https://github.com/swiftlang/swift-toolchain-sqlite). As to your other concerns, I feel like they can all be mitigated by simply stating that this support is experimental and optional (just like the current Linux support). Nothing this PR does changes how GRDB functions on Apple platforms by default. It is only when opting-into the SQLCipher trait that the new build behavior would be activated. I'm happy to continue iterating on the PR or provide alternate suggestions/solutions, but I also understand if you would prefer GRDB to stick to narrow path of supporting only the vendored-SQLite on Apple platforms. |
All right, @marcprux, we are converging. I agree that we can just flag SQLCipher support as experimental. I'll have a closer look at the PR. |
(EDIT: in response to a comment that was deleted by its author) I'm not sure the meaning of "experimental" was correctly understood. Expressions such as "huge deal" and "intolerable" can only be supported by actual contributions. Il n’y a pas d’amour, il n’y a que des preuves d’amour (the proof is in the pudding). EDIT: Given the complete lack of mention of GRDB in the SQLiteData README, I would advise commenters to choose their arguments with more tact and discernement than some of their favorite developers. |
This was an honest mistake on our part and we saw your comment on Hacker News and immediately corrected it. We took for granted the obviousness of GRDB’s presence in the library in the name alone, and so forgot to update the readme with the 1.0 release. We are very sorry for that. |
XCTFail("Expected DatabaseError") | ||
} catch let error as DatabaseError { | ||
XCTAssertEqual(error.resultCode, .SQLITE_ERROR) | ||
#if canImport(ObjectiveC) // non-Darwin platforms bridge NSError differently and do not set the message the same |
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.
This PR merges two concerns here: SQLCipher and Android.
NSError is available on non-Darwin platforms, so people can use it. I'd be curious to know if this test can be restored. When a custom SQL function raises an error, it is important that the host application can catch a message that preserves as much as possible from the original error.
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.
What's the specific issue here? This test does pass on Linux, as the DatabaseError
is used here directly. I don't need the compiler directive here. Is this different on Android?
|
||
func testNSErrorBridging() throws { | ||
#if !canImport(ObjectiveC) | ||
throw XCTSkip("NSError bridging works differently on non-Darwin platforms") |
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.
Same question as above
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.
Again, I'm having a similar issue on Linux. The DatabaseError
in this test is thrown correctly but casting to an NSError
in the catch
clause fails. I guess this is either a bug in Swift on non-Darwin platforms or not supported. Why would we need NSError
in this case instead of the Swift Error
protocol? To preserve the error information in DatabaseError
?
func testEmptyMigratorPublisher() throws { | ||
func test(writer: some DatabaseWriter) throws { | ||
let migrator = DatabaseMigrator() | ||
#if !canImport(Combine) |
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.
I'd move this check at the beginning of the test method (as all similar checks below).
// > because DISPATCH_QUEUE_OVERCOMMIT is not a public API. I don't | ||
// > know of a way to get a reference to the overcommit queue using | ||
// > only public APIs. | ||
#if !canImport(Darwin) |
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.
Should be moved at the beginning of the test function
/// A XCTestCase subclass that can test its own failures. | ||
class FailureTestCase: XCTestCase { | ||
private struct Failure: Hashable { | ||
#if canImport(Darwin) |
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.
Instead of turning regular methods into throwing methods, as below, in order to be able to throw XCTSkip, I'd rather remove the whole FailureTestCase
from non-Darwin platforms (or Android only, if this limitation is specific to Android). I hope Swift Testing makes it possible to run tests that catch test failures.
|
||
func testNSDecimalNumberPreservesIntegerValues() throws { | ||
#if !canImport(ObjectiveC) | ||
throw XCTSkip("NSDecimalNumber unavailable") |
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.
NSDecimalNumber is part of the GRDB support for decimals.
I find mentions of NSDecimalNumber in https://github.com/swiftlang/swift-corelibs-foundation and https://github.com/swiftlang/swift-foundation.
Is this lack of NSDecimalNumber temporary, or Android specific? Shouldn't we use #if os(android)
instead of #if !canImport(ObjectiveC)
?
Finally, shouldn't we disable the whole test class, since it is named FoundationNSDecimalNumberTests?
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.
I'm having a similar issue on Linux as mentioned here The issue is not the availability of NSDecimalNumber
but a problem with two constructors which are used in converting int64
and double
values from a DatabaseValue
to an NSNumber
. I have solved it similarly by ignoring these tests on Linux and Windows platforms. Not sure why these constructors are giving problems. Maybe they are not available? In any case it needs more research!
func testNSNumberDatabaseValueToSwiftType() { | ||
func testNSNumberDatabaseValueToSwiftType() throws { | ||
#if !canImport(ObjectiveC) | ||
throw XCTSkip("NSNumber unavailable") |
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.
Same question as above about NSDecimalNumber.
|
||
func testNSURLDatabaseRoundTrip() throws { | ||
#if !canImport(ObjectiveC) | ||
throw XCTSkip("NSURL unavailable") |
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.
NSURL
is less useful than NSNumber
/NSDecimalNumber
, so here I'll just recommend that the whole test class is disabled on platforms that do not support NSURL. Or to throw XCTSkip from setUp()
so that we can see this test case in the test log.
(I admit I don't quite understand why we can keep on compiling this test, and yet do not compile the NSURL support for GRDB)
|
||
private func assertRoundTrip(_ uuid: UUID) throws { | ||
#if !canImport(ObjectiveC) | ||
throw XCTSkip("NSUUID unavailable") |
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.
Same request as in FoundationNSURLTests
above.
XCTAssertNil(URL.fromDatabaseValue(databaseValue_Int64)) | ||
XCTAssertNil(URL.fromDatabaseValue(databaseValue_Double)) | ||
XCTAssertEqual(URL.fromDatabaseValue(databaseValue_Blob)!.absoluteString, "bar") | ||
XCTAssertEqual(URL.fromDatabaseValue(databaseValue_Blob)?.absoluteString, "bar") |
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.
It looks like a change that is not strictly necessary.
} | ||
|
||
|
||
#if canImport(Darwin) // @MainActor test cases don't compile non-Darwin platforms: "call to main actor-isolated instance method 'test_mainActor_observation()' in a synchronous nonisolated(unsafe) context" |
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.
This needs a link to at least one bug report in the Swift repository.
@@ -1,4 +1,4 @@ | |||
// swift-tools-version:6.0 | |||
// swift-tools-version:6.1 |
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.
Not required in this pull request, but some cleanup is needed:
git grep -i 'version.*\b6\.0'
git grep 'compiler('
README.md
GRDB.swift.podspec
.library(name: "GRDB-dynamic", type: .dynamic, targets: ["GRDB"]), | ||
], | ||
traits: [ | ||
.trait(name: "GRDBCIPHER", description: "Use the SQLCipher library rather than the vendored SQLite"), |
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.
After reading the naming concerns expressed in #1816, and the CamelCase
trait names used in the proposal SE-0450, I suggest using the name SkipSQLCipher
or SkipToolsSQLCipher
, and a description that mentions the repository URL https://github.com/skiptools/swift-sqlcipher
I assume, @sjlombardo, that the names I suggested are acceptable for Zetetic.
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.
We're about to have multiple SQLCipher versions: the one in this pull request, the one from #1816, and the CocoaPods one (even if it's high time I completely removed it).
To simplify the compiler flags dance, I start working on replacing most uses of #if GRDBCIPHER
with #if SQLITE_HAS_CODEC
. That last flag ought to be set by all GRDB+SQLCipher flavors. See #1819
cc @R4N
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.
@groue That sounds good to us. We were initially re-using the GRDBCIPHER
flag, but using SQLITE_HAS_CODEC
should work better for supporting both flavors. We'll switch over to that.
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.
@groue thanks, the suggested names look good to us, things will be very clear with the prefixing.
This is a less ambitious alternative to #1701 that enables adding a SQLCipher source dependency to the Package.swift based on the
GRDBCIPHER
environment variable. Tests can be run with the dependency with a command like:Running it without the environment variable will leave things exactly as they are. The majority of changes in this PR are simply re-ordering the import header checks for
GRDBCIPHER
andSWIFT_PACKAGE
, since we would need the former to take precedence of the latter.An advantage of doing it this way is that it facilitates creating a(this is no longer needed when using package traits; see comment below)GRDB-SQLCipher
fork that can follow the releases of the upstream. The only change in such a fork would be to hardwire theGRDBCIPHER
setting in Package.swift to some SQLCipher source repository (either mine, or one that you maintain). When a client package wants to switch between GRDB and GRDB-SQLCipher, they would just need to swap their dependency URL.This PR also adds in support building and testing against Android, mostly by stubbing out the unit tests that can't compile due to missing
NSFileCoordinator
,Combine
, and the like. Android tests can be run with skip:Most of the tests pass, but there are a few that need to be investigated. I'll look into them next.
Closes #1701