SQLiteData linux fixes#459
Conversation
… the fixes to make GRDB build again on Linux. Also wrap the CloudKit specific tests in a #if canImport(CloudKit) to ignore these tests on Linux
… SyncEngine. Ignore them also by adding an #if canImport(CloudKit) block.
| dependencies: [ | ||
| .package(url: "https://github.com/apple/swift-collections", from: "1.0.0"), | ||
| .package(url: "https://github.com/groue/GRDB.swift", from: "7.6.0"), | ||
| .package(url: "https://github.com/groue/GRDB.swift", from: "7.10.0"), |
There was a problem hiding this comment.
Would it be possible to keep this relaxed to just 7.6 and then anyone that wants linux support for GRDB can explicitly depend on 7.10+?
There was a problem hiding this comment.
@mbrandonw I now tried reverting this line to 7.6.0 and the code builds. I guess it picks the correct version on Linux then anyway (i.e. 7.10.0). Not sure why it wasn't happening earlier. I cleaned it with swift package clean and also tried manually deleting the Package.resolved and all builds work. I've reverted and pushed on this branch.
Just for my curiosity (and to learn) why would you want to keep this on an earlier version?
| // TODO quick fix, may need to improve this bit | ||
| #if canImport(CloudKit) |
There was a problem hiding this comment.
Can you explain what's the problem with this file? are there a mixture of CloudKit and non-cloudkit tests in here? I haven't looked through the file myself yet.
There was a problem hiding this comment.
Just saw your comments over here on this. It doesn't seem importing SQLite3 is actually necessary. Does removing that import help you?
These tests are technically most relevant to CloudKit because the whole reason we provided this tool is to help people migrate from auto incrementing IDs to UUIDs for the sync engine. But it doesn't actually use anything from CloudKit and theoretically it could be helpful to people on non-Apple platforms.
There was a problem hiding this comment.
@mbrandonw There are two errors. The first is that the type SQLiteSchema is not found. I was under the impression that this was related to the import not being available. However, upon further investigation I found out that SQLiteSchema is a SQLiteData type. The type is wrapped in an #if canImport(CloudKit) block. I removed this and that part of the code now builds fine.
The second is that two tests are using the SyncEngine which is of course not available (yet) on Linux. I've wrapped these tests now in an #if canImport(CloudKit) block. But after that I'm getting the next errors:
PrimaryKeyMigrationTests.swift:207:25: error: argument passed to call that takes no arguments
205 | }
206 |
207 | try migrate(tables: Parent.self)
| `- error: argument passed to call that takes no arguments
208 |
209 | assertQuery(Parent.select { ($0.rowid, $0) }, database: database) {This happens at several lines for several types. I saw this was caused because I put the whole migrate helper function in an #if canImport(CloudKit). After only putting the SyncEngine bit in such a block the tests compile, but of course fail. All tests in PrimaryKeyMigrationTests fail. So unless we can enable part of the SyncEngine on Linux, I'm not sure if these tests than make sense on Linux. What do you think?
There was a problem hiding this comment.
I added (and updated) the Linux part of CI.yml as well. Now it runs the build and the unit tests. The build step passes and the unit tests now correctly fail.
There was a problem hiding this comment.
@mbrandonw I've found a way to make the tests in PrimaryKeyMigrationTests pass. I analyzed the code and checked which dependencies were marked unnecessarily with an #if canImport(CloudKit) and remove them. Then let Codex generate a version of the SyncEngine.swift file that would also contain a minimal stub that would compile on Linux. That just generated an #if canImport(CloudKit) #else block that contained some duplicated code in the #else block. I reverted this code and tried to see what I could come up with manually. Which code I could get to compile gradually adding #if canImport(CloudKit) statements. Not sure if this a lot better than what Codex produced 😆 So this probably needs so deliberation.
I also wonder whether these (large number of) fixes are justified only to fix PrimaryKeyMigrationTests. It's a trade-off of course. Some changes, like removing a #if canImport statements for code that will compile on Linux and other platforms probably are worth it. However, it may mean adding "dead" code that is only used on Apple platforms. I do not have a good enough mental model to judge this. I will submit submitted the changes soon to this branch to let you get an idea.
Note the SyncEngine initializer for platforms that cannot import CloudKit is the only AI generated code here. I do not have time right now to review it in more depth (will do that later), but at least all tests pass now.
…e to build for platforms that don't have CloudKit. It's also used heavily in PrimaryKeyMigrationTests. I adapted these tests to compile on Linux as well. However, they now all fail.
…ep to the Linux container. Tests revealed the linker error with GRDB on Linux and therefore should be run when building.
SQLiteData Package fixes
GRDBto 7.10.0 inPackage.swiftthat contains the fixes we made to makeGRDBbuild on Linux again. No further changes were needed. By using thisGRDBversionSQLiteDatabuilds under LinuxChanges to Unit Tests
After the above changes, the unit tests still fail to build. I changed the following to make them build:
#if canImport(CloudKit)block asCloudKitis not available on Linux:UnattachedSyncEngineTests.swiftSyncEngineTestsCloudKitTestHelpers.swiftSchema.swiftthat usesCloudKit/SyncEngineUserDatabaseTestsPrimaryKeyMigrationTests, but not completely sure this was the right way to go. The tests fail to build becauseSQlite3is not found on Linux. When wrapped in acanImportblock several other constructs are not found on Linux. Either they are available in theSQLite3package and we could find an equivalent on Linux to fix these tests. However, if they are also related toSyncEnginethey should be ignored on Linux. I can't judge at this moment if this was the case.After these changes all tests that are not ignored by these changes pass. I want to also look into bringing the CI back for Linux. @stephencelis already send me some info on that. Let me know if these changes are acceptable and whether I can improve some tests to make them work on Linux as well!