Updated Fixes for the Linux version of GRDB#1825
Updated Fixes for the Linux version of GRDB#1825thinkpractice wants to merge 18 commits intogroue:developmentfrom
Conversation
|
I did an initial scan, and my initial takeaways:
|
Thanks!
Actually, I tried to check this with
That would be nice, I discussed this with @groue earlier, but he wanted to wait a bit with this. It would be good in the long run though. If the build fails I could check it and fix it faster. Well let's see 😉 |
|
Hello @thinkpractice, That's huge, and I'm so thankful 🥳 It's a pity that the diff messed up with white space 😬. The git blame will be unusable, and code archeology will become very difficult. I'm very sorry but I have to ask you to redo all your commits and remove all white space changes.
[EDIT] It looks like you actually applied a code formatter. Again, I'm very sorry, but this is not mergeable. And I can not review in correct conditions with so many unrelated changes. I hope VSCode can be configured so that it does not mess up with existing files. |
There was a problem hiding this comment.
Please modify all commits (rewriting history) so that all code formatting and white space changes are removed, and git blame only shows actual changes. If it can help, modify .vscode/settings.json so that VSCode is well behaved and does not take ownership of code formatting.
|
Let's not have code formatting pollute the review. So I looked at |
|
@groue No worries! Ughh this is due to my inexperience contributing to open source projects like this 😞 (and part of my learning curve). It would help me a ton if you give me some pointers as to what your setup according to code formatting is. I think my main mistake is not disabling The other thing is configuring Looking forward to your |
|
The pull request for snapshots: #1826 |
😅 I hope I did not sound too bad at you! As I said above, the real problem is the git blame.
There is no formatting spec such as an My best suggestion is to disable all code formatting tools, and to control what you commit.
I don't think SwiftLint is invoked from SPM. I suppose you can ignore it for now?
I hope #1826 helps! 🤞 |
Nah it was really fine. To be honest I didn't even think about the code formatting being an issue. Then when @marcprux told me yesterday that code reviewing would be a challenge I realised that the git diffs would be messed up because of that. My mistake! If I cannot fix it on this PR then I will just redo the changes and submit it again. Hoping the third time's a charm!
Oh, I just downloaded a vscode EditorConfig extension in the hope that it would take the
I found out that you have |
|
Oh, @thinkpractice, I certainly do not want to sound like I am "ableist" regarding the tooling. If you have difficulties with formatting, this is quite OK: I will perform the necessary cleanup. Please do not feel embarrassed! This PR is such a boon 👏 |
|
I'm mostly trying to get my head around some of the git commands related to PRs. I'm used to using branches but not really use to PR. Trying to figure out how to commit changes to this PR for instance? UPDATE just found out that I can just change my forked branch and the changes will be reflected here DUH. Well that's really awesome! Will go and check how to get the formatting in a good shape again :) |
|
@groue how do you use |
To see your changes, I just use the GitHub interface, that's OK. I do not use
EDIT: So a good practice is to strictly distinguish commits that format from commit that change code meaningfully. This way, archeology is still possible: formatting commits can be ignored, and commits that change code meaningfully have no noise. This rule is not an absolute, of course. But here all thresholds have been crossed ;-) |
|
@groue as I trial I tried to reformat EDIT: I have just discovered the |
|
@groue I found out there's some weird behaviour with my vscode. One time tabs are converted to spaces, and then another indentation which was 4 spaces gets converted back to a tab 😠 Will need to check how to solve this. Already changed my editor settings, so either I missed some setting or it's a bug in vscode.... Found it apparently there is something like |
|
@groue I have been updating some more files to reflect the original formatting and make it easier for me to see my changes. Before these changes would be synced to this PR, but now I'm not seeing them anymore. Is this something that sound familiar to you? I have already tried the solution mentioned here, i.e. updating the base to the same branch and saving it. So far, this hasn't helped, and if I try it again I'm getting an error saying the base is already being updated. I will check later if I can see the changes, but if they don't show up, do you have any tips? UPDATE some of my changes are starting to show up... so I guess I it's working again. Let's see if the other changes I made later will show up as well! |
|
@thinkpractice I have pushed on the groue:grdb_linux_changes branch all changes of your own branch, fully rebased on the latest development branch, with all code formatting removed. I took care that all commits are attributed to you. I had to heavily amend your initial changes, so I hope I haven't broken or forgotten anything. If this branch builds as well as yours on Linux, and if you agree, maybe you could force-push this branch to yours? That will make the review, and future changes, much easier. |
|
@groue did you see I spend some time cleaning up the code as well? Hope you could use that. Thanks for taking care of the rest! Will take care to format my code in the right way on any future changes. I am fine with force pushing the changes to my branch if that makes things easier. I am still traveling so I cannot really test your changes now (only brought my MacBook). I will be home next weekend so then I can test everything. Hope that’s ok! |
Yes, thanks for your efforts! Now starting over from the
I hope
If you look at the new commits, you can see that they perform minimal changes. They do indeed make things much easier. I just don't know if I faithfully reported all your changes. So please take your time, check this branch on your Linux system, and make sure I did not forget anything! 🙏 |
|
@groue checked out your branch and it compiled successfully! Also the test project compiled and ran and we now have: So it looks like we have the build working on Linux again. I will merge the changes you made into my branch and then hopefully we can merge. Do you think whether there would be any additional things I can do to improve the support on Linux? I'm thinking along the lines of adding some equivalent tests for the Darwin specific tests we commented out, adding a custom |
|
Welcome back, @thinkpractice -:
You can be proud :-)
If you do not mind, please force-push my branch to yours instead, so that only "my" commits are in your branch, and only them. This will remove from the git history all the formatting and de-formatting commits. [EDIT]: I'll help you if this is not quite clear - please just tell.
I'm looking forward for all the goos ideas you will suggest :-) For now, let's merge this. In parallel, there was some work in progress regarding SQLCipher, in #1827, in which a serious issue was discovered - it slipped in the I'm really curious to see what will be shipped in the next release :-) |
Thanks! Good to be back :) Although I had a blast at SwiftLeeds and in the UK in general 😄
Thanks! Happy it was relatively easy to get it to run, thanks for all the prior work done 😄
Yeah would prefer some help. So I have your repo as Would this do the job?
I think I will check first if I can get a
That makes two of us! Looks like there are a couple of exciting changes coming up! |
|
@groue just in case you missed it. Is this the right way to apply your changes to my branch? If so then I can merge and fix the remaining issues this week 😄 |
2ddf510 to
3dd3bbf
Compare
|
Hi @thinkpractice, sorry I had a busy weekend. OK so let's go: My current remotes are Let's add a remote for your own fork, where I can find your branch Let's checkout your branch Is it what we expect? Yes it is. These are your latest commits: Let's reset its state to my branch Is it what we expect? Yes it is. These are my latest commits: We can now force-push the content of my branch to yours: And we're done:
On your side, since I did force-push to you branch, you will not be able to pull. You will have to reset your local branch to its new remote state. The latest commit is 3dd3bbf, so you'll write in your terminal: |
|
@groue Thanks so much for the detailed explanation. As you see I'm not that familiar with these specialized git commands, but the above helps me a lot to learn 😄 So I guess the command I showed above would've worked too but was missing the
|
That's fine! I frequently use an LLM to help me, or an app that puts a GUI in front of git. The hardest part is frequently to know what is even possible to do 😅
Yes, please play with it as much as you can - that's not something I can easily do! For the CI part, I'd prefer, if it is possible, that we do not add Linux tests to the |
|
@groue @marcprux I've started adding a CI file for Ubuntu. The idea is that it will download Swiftly and use that to install the appropriate Swift version. I now added a matrix with both UPDATE: it looks like it's working now, but I have the feeling that is sometimes fails for some random reason. Is this to be expected? I also added a batch to Would this be a way forward for the Linux build? As I'm quite new to GitHub Actions and I shamelessly stole some code from another project and adapted it to GRDB I'm looking forward to your feedback 😄 |
marcprux
left a comment
There was a problem hiding this comment.
I think this looks good, pending a couple suggestions! It will be nice to have this extra validation, and also makes a good first step towards Android support.
.github/workflows/ubuntu-ci.yml
Outdated
|
|
||
| concurrency: | ||
| group: ${{ github.ref_name }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
I think this should be false, because it is often useful to see when one variant passes and another fails (e.g., if a change breaks for Swift 6.0 but not 6.1)
There was a problem hiding this comment.
I agree, changed cancel-in-progress to false
| path: ${SWIFTLY_HOME_DIR:-$HOME/.local/share/swiftly} | ||
| key: swiftly-${{ matrix.os }}-${{ matrix.swift }} | ||
|
|
||
| - name: Install Swift Using Swiftly |
There was a problem hiding this comment.
You could alternatively just use uses: swift-actions/setup-swift@next as per swift-actions/setup-swift#710. I've been using it in other actions, and it seems quite stable.
There was a problem hiding this comment.
I did use setup-swift@v2 before but that didn't support swift 6.2. Also, it threw some random errors with the gpg verification (?) of swift, which made the pipeline fail. UPDATE apparently this gpg issue is also mentioned here, i.e. "Investigate GPG issues on linux". That's why I decided to add the swiftly code myself. I agree it would be better to use the setup-swift action in the future. I guess then @next selects the latest v3 version that uses swiftly as well? I saw on its PR that it's not yet merged into main right?
There was a problem hiding this comment.
Oh and I saw there's a possibility to cache the swiftly install but it seems with the current code it doesn't do anything. Not sure whether the path I added (${SWIFTLY_HOME_DIR:-$HOME/.local/share/swiftly}) is evaluated correctly or whether it's an issue with the cache action itself. @marcprux, @groue does any of you have some experience with this?
|
Hello, I did not forget this pull request, but there are a lot of new commits that I did not take the time to review yet. Please hold on. |
|
@groue you've beat me to it 😄 I was going to ask you for the status of this. I will check for any upstream changes and try to merge them! I have been testing a bit with GRDB and Linux and haven't run into any issues yet. However, need to test more. |
|
Hello @thinkpractice, I've merged the initial part of the pull request (3dd3bbf) into the development branch. Thank you! 🙏 This pull request now only contains the CI configuration. It looks like something could be done regarding caching? I'll leave the pull request open for a while, until this question finds a clear answer. |
7b2b8a0 to
1e4d014
Compare
|
Hi @groue, thanks for merging! Just for my understanding, this will then merge all my changes into the development branch? I just had a look at this branch the other day to see if I could merge some new changes you made after december into it. It was made a bit more difficult because the development branch didn't contain any of these changes yet and I didn't have enough time (yet) to give it a thorough look.
You're more than welcome 😄 I will check-out the development branch tomorrow and try to build it on linux, run the tests. Although if you enable the CI it should pickup if things fail to build.
Caching in what regard? Do you mean the toolchain installed with kind regards, Tim |
|
The part that added Linux support is merged in
You tell me ;-) This sentence of yours above raises questions:
Caching speeds up CI. Like everybody else, I hate waiting. If the Linux CI wants me to look at it, it's better be reasonably fast. And it installs a lot of stuff. Marc also suggested some ideas. For me this is not a blocker, but it looks like there's some unfinished business. It's all up to you. If you say "I'm done" I'll be OK with that. |
I will have a look if I can speed it up. I remember updating the CI according to some of the changes @marcprux suggested, but maybe I've missed some. Hope I will be able to look at it this week. |
|
Thank you @thinkpractice! |
|
⛵ The first part of the pull request has shipped in v7.10.0 :-) |
Fixes for the Linux version of GRDB
UPDATE this PR merges some upstream changes into my fork.
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)DatabaseMigratorTests.swiftI tried to merge them but not sure everything went ok. I was struggling a bit integrating them. This file should be checked if all upstream changes have been merged in.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: