-
-
Notifications
You must be signed in to change notification settings - Fork 799
Fixes for the Linux version of GRDB #1824
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
Closed
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nsion, as it compiles fine under swift 6
…tforms. Fix this.
… code. This should be only Linux, because other platforms may provide a custom build. This should be fixed in another way later because we may want to add custom sqlite builds for Linux later.
…throw an XCTSkip for these test cases. This gives a better impression how many tests are skipped on Linux/non-Darwin platforms.
…rwin platforms. Skip testNSErrorBridging on these platforms.
Author
|
@groue I'm seeing one merge conflict. I will investigate how to solve these in a fork (I don't want to edit them in the GitHub editor). |
Author
|
@groue I'm seeing one merge conflict. I will investigate how to solve these in the fork (I don't want to edit them in the GitHub editor). Any tips how to do this in a handy way? I think I will just close the PR now and send another one! |
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes for the Linux version of GRDB
This PR contains some fixes for the Linux version of GRDB, mainly these fixes include:
Foundationon Linux and Apple platformsCombineorNSFileCoordinator, etc.Some of these fixes are inspired by #1708 by @marcprux. This PR attemps to make these fixes in
a platform-agnostic way so that
GRDBwill build withSPMon more platforms than Linux alone,for instance, on Android and Windows. These patches fix the build of
GRDBonSwift 6.1and later.There are still some issues building on
Swift 6.0, which I will attempt to fix later.Fixing the GRDB Build
Most of GRDB actually builds on Linux just fine. The only change is that I needed to do to make it build was adding a
to handle non-Sendable protocol conformance in
GRDB/Core/DispatchQueueActor.swift. Not sure if that's the right approach but it made everything build!Fixing GRDB linking on Linux
The main issue with
GRDBon Linux is actually a linker error. The standardlibsqlite3-devinstalled withaptordnfdoesn't have WAL enabled and when trying to useGRDBin another swift package or app in Linux, you get a linker error. Functions likesqlite3_snapshot_get,sqlite3_snapshot_openare not found by the linker,and therefore the package isn't usable.
There would be two ways around it I think, either:
sqlitebuild forGRDBon Linux, orI chose the latter for now as it is a more easy fix. I updated the following conditional compilation directive:
#if SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER)to:
#if (SQLITE_ENABLE_SNAPSHOT || (!GRDBCUSTOMSQLITE && !GRDBCIPHER)) && !os(Linux)This is a quick fix, but it would be better to change the
SQLITE_ENABLE_SNAPSHOTto returnfalseon Linux.Note this should be better handled as a fix to
SQLITE_ENABLE_SNAPSHOTas later on Linux may also providea custom sqlite build to enable WAL. In those cases, the code inside these blocks should be compiled.
This compiler directive is present (and updated) in the following files:
GRDB/Core/DatabasePool.swiftGRDB/Core/DatabaseSnapshotPool.swiftGRDB/Core/WALSnapshot.swiftGRDB/Core/WALSnapshotTransaction.swiftGRDB/ValueObservation/Observers/ValueConcurrentObserver.swiftUpdating the compiler directive as above will fix the build with the standard
libsqlite3-devon Linux. If that's fine wecan keep it at that. I'm not completely sure yet what WAL does but if I understand correctly, using it is mostly a
performance consideration. If it would be a nice to have on Linux too, I can research adding a custom
sqlitebuild forLinux with WAL enabled. I wonder why it is not included in the standard
libsqlite3-devthough? Adding the changes abovewill fix linking of the
GRDBcode.Fixes for open source Foundation versions
Open source versions of
Foundationare slightly different than theFoundationApple bundles withXCode. Also, somepackages, like for instance
CoreGraphics, are not available on non-Apple platforms. Some primitive types likeCGFloatare available on non-Apple platforms and are defined in
Foundation. In some cases, there are also differences where ainitializer is defined as
convenience initin open sourceFoundationand as a normalinitin AppleFoundation. Thiscauses some problems with Apple specific code in some cases; it will fail to build on open source
Foundation. InGRDBinmany cases this code is surrounded by a
#if !os(Linux)compiler directive. The fixes below try to solve the build issues onLinux and try to get rid of these compiler directives. Also fixing the unit tests at the same time.
CGFloat.swift
CoreGraphicsis not available on Linux. ButCGFloatis available viaFoundation. Changed the compiler directive inCGFloat.swiftand its test from:#if canImport(CoreGraphics)to:
The same fix should be added to
GRDBTests/CGFloatTests.swift.Decimal.swift
Removed the
#if !os(Linux)compiler directive in theDecimalextension, as it compiles fine under swift 6. As discussedwith @groue the minimum version for
GRDBisswift 6.0. I tested these changes withswift 6.1andswift 6.2and they work fine.Note: swift 6.0 doesn't build, we should look into fixing the build for this version.
NSString, NSDate, Date
Some foundation
NS*types (NSString, NSData, Date, etc.) were excluding theDatabaseValueConvertibleextensions on Linux but build fine onswift 6.1andswift 6.2.Removed
#if !os(Linux)in:NSString.swiftNSData.swiftDate.swiftNSNumber
There's a problem with the following code in
NSNumber.swifton Linux:both the
.int64and.doublecase above do not compile. The error is the following:The
NSNumber(value: Int64)andNSNumber(value: Double)are marked asconvenienceinit on Linux. This may cause the build to fail. The issue with this approach is that it doesn't work forNSDecimalNumber, which is a subclass ofNSNumber.It looks like the main issue to begin with is that
NSNumberdefinesNSNumber(value: Int64)andNSNumber(value: Doubleas convenience init, whereas theirNSDecimalNumberequivalents are non-convenience. I could solve it by changing the codeabove like this:
This is a lot more verbose than the existing code. Also, this code may fail if there are other subclasses of
NSNumberthat need to be supported. For each of these classes we should add a case for
.int64anddouble. I think an alternativeapproach would be better, but I don't have one at the moment.
All tests in:
Tests/GRDBTests/FoundationNSNumberTests.swiftTests/GRDBTests/FoundationNSDecimalNumberTests.swiftsucceeded with these changes.
URL.swift
The
NSURLis a specific case. Apparently, theNSURLabsoluteStringproperty is non-optional on Linux and optional on Apple platforms. The fix is the following:/// Returns a TEXT database value containing the absolute URL. public var databaseValue: DatabaseValue { #if !os(Darwin) absoluteString.databaseValue #else absoluteString?.databaseValue ?? .null #endif }UUID.swift
UUID.swiftcontains a compiler directive excluding theDatabaseValueConvertiblefrom buildingon Linux and Windows. On Linux the problem is the following:
The
NSUUID(uuidBytes:)constructor takes an optionalUnsafePointer<UInt8>, see here, whereas this constructor on Linux takes a non-optional.This could be fixed with a guard statement. However, similarly to the case for
NSNumberin addition, theself.init(uuidString: string)doesn't compile on Linux. The error is:Added the following fix:
This also makes all unittests succeed. It should be checked whethere we would want to support any subclass of
NSUUID.If so, a similar solution as for
NSNumberabove can be used.Use of DispatchQueue private APIs
In some of the tests for the
DispatchQueueAPIs, some privateDispatchQueueapis are used to get the label of thecurrent
DispatchQueue. These APIs are unavailable on non-Darwin platforms. It's mainly about some tests in for exampleDatabasePoolConcurrencyTests.swift. For instance, the following test:I understood from @groue that the goal of those tests is to make sure that the Xcode, LLDB, and crash reports mention the label of the current database in the name of the current DispatchQueue. This is a nicety that aims at helping debugging, and worth a test. The label of the current dispatch queue can only be tested with the private
__dispatch_queue_get_labelapi, which is only available on Darwin systems.Those tests can be avoided on non-Darwin platforms, as @marcprux did in #1708:
I implemented this approach for:
Utils.swiftDatabasePoolConcurrencyTests.swiftDatabaseQueueTests.swiftDatabaseSnapshotTests.swiftNSError
Bridging of
DatabaseErrortoNSErrordoes not seem to work on Linux. ThetestNSErrorBridgingthereforefails, we should probably ignore the test on non-Darwin platforms as NSError is mostly a left-over from
ObjectiveCtimesand APIs.
Fixing Unit tests
Tests using NSFileCoordinator
There are some tests using
NSFileCoordinatorwhich is not available on non-Darwin platforms. Most notablytestConcurrentOpeninginDatabasePoolConcurrencyTests.swiftcontributes 400+ test failures when run onLinux.
testConcurrentOpening, that runs about 500 tests (a for loop of 50 iterations runs 10 times some things in parallelwith
DispatchQueue.concurrentPerform. The test uses theNSFileCoordinatorwhich isn't available on Linux.These tests are ignored because they test whether
NSFileCoordinatorworks withGRDB. This isDarwinspecific functionality:Arguably
DispatchQueue.concurrentPerformcould be replaced with any other modern technique that guarantees a minimum level of parallelism, such aswithTaskGroup.We should check whether we need to test similar behaviour on Linux. For now we just ignore the test.
@marcprux solved it like this:
We solved it similarly.
Other Unit tests
XCTIssueis not available on non-Darwin platforms, it's used in:GRDBTests/FailureTestCase.swiftignored this test completely with#if os(Darwin)on non-Darwin platformsGRDBTests/ValueObservationRecorderTests.swiftignored this test also.Support.swiftactually needsCombine, so all tests that use it needCombineas well:DatabaseMigratorTests.swifthas several tests using theTestclass defined inSupport.swift; used an#if !canImport(Combine)clause to skip several tests in this test suite.ValueObservationTests.swiftalso uses thisTestclassDatabaseRegionObservationTests.swifttests someCombinefunctionality intestAnyDatabaseWriter; also wrapped them in a#if canImport(Combine)TODOs
swift 6.0XCTIssue__dispatch_queue_get_labelon LinuxCI.ymlpipeline to build the Linux version of the package automatically and run the unit testsWith this we: