refactor: allow specific sqlite engine for OpenBSD

[pacoesteban]

OpenBSD will be removing direct access to syscall(2) soon. Shiori will stop working because of this, as some of its dependencies rely heavily on the use of syscall.Syscall*, which ends up using syscall(2). This commit removes those dependencies by reverting back to use github.com/mattn/go-sqlite3 instead of modernc.org/sqlite to deal with the sqlite database backend.

[fmartingr]

Thanks for looking into this! As we discussed, there were going to be some roadblocks before moving forward and the CI just found the first one. If you give me permissions to your branch I can also work on this when I have some spare time.

That said, expect work on this after 1.6.0 is released. I don't want to include more breaking changes in the same release.

[pacoesteban]

Thanks for looking into this! As we discussed, there were going to be some roadblocks before moving forward and the CI just found the first one. If you give me permissions to your branch I can also work on this when I have some spare time.

Yep, sure. I gave you permissions there.
As I mentioned, we could fix this with conditional compilation, so it does not have any impact on the CI as you do not ship binaries for OpenBSD (and they are kind of pointless too if there's a port), but I don't know how to do it without having duplicated files which would be ugly and difficult to maintain in the future ...

That said, expect work on this after 1.6.0 is released. I don't want to include more breaking changes in the same release.

Sure. No problem.

[fmartingr]

Hey @pacoesteban, I've pushed a draft using build tags and different files. Didn't test it more than running the server locally in my machine, but can't test the openbsd part. It is supposed to build properly for openbsd using a second sqlite implementation file. Can you take a look when you have some time and let me know?

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 30.77%. Comparing base (595cb45) to head (74bbee7).
Report is 21 commits behind head on master.

Files	Patch %	Lines
internal/database/sqlite_noncgo.go	66.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage   25.58%   30.77%   +5.19%     
==========================================
  Files          47       62      +15     
  Lines        5628     4461    -1167     
==========================================
- Hits         1440     1373      -67     
+ Misses       3989     2866    -1123     
- Partials      199      222      +23     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pacoesteban]

Hey @pacoesteban, I've pushed a draft using build tags and different files. Didn't test it more than running the server locally in my machine, but can't test the openbsd part. It is supposed to build properly for openbsd using a second sqlite implementation file. Can you take a look when you have some time and let me know?

I built it with your changes on a recent snapshot of -current, but I still get a segfault at runtime because of modernc.org/libc dependency. Either I'm doing something wrong, the conditional compilation is not taking place or there is something else that is introducing the dependency on the modernc libraries.

I tried to update go module versions as I saw some "downgrades" in the diff between master and this branch, but no luck.

I'll take a deeper look tomorrow.

[fmartingr]

Hey @pacoesteban, I've pushed a draft using build tags and different files. Didn't test it more than running the server locally in my machine, but can't test the openbsd part. It is supposed to build properly for openbsd using a second sqlite implementation file. Can you take a look when you have some time and let me know?

I built it with your changes on a recent snapshot of -current, but I still get a segfault at runtime because of modernc.org/libc dependency. Either I'm doing something wrong, the conditional compilation is not taking place or there is something else that is introducing the dependency on the modernc libraries.

I tried to update go module versions as I saw some "downgrades" in the diff between master and this branch, but no luck.

I'll take a deeper look tomorrow.

Tried real quick to run in a VM but it didn't load the image at all. Haven't used VMs properly in macOS for years so I will need to investigate this a little more. Thing is, I can cross-compile and it gives me no errors so I assumed the go toolchain was smart enough to ignore dependencies that didn't need. I need to investigate more.

[pacoesteban]

Tried real quick to run in a VM but it didn't load the image at all. Haven't used VMs properly in macOS for years so I will need to investigate this a little more. Thing is, I can cross-compile and it gives me no errors so I assumed the go toolchain was smart enough to ignore dependencies that didn't need. I need to investigate more.

The error happens at run time. If I understand correctly, at this stage of the process (syscall elimination I mean), the symbols are there but are just NOOPs. So one can build an link against them but they fail at run time, which is what I'm seeing.

I can bring up again the test machine I prepared and grant you access if you want. Just let me know.

[fmartingr]

I can bring up again the test machine I prepared and grant you access if you want. Just let me know.

I tried some guides around the internet but I had no luck even installing it in a VM under Apple's ARM virtualization layer (and I was testing latest stable release). I don't want to bother you with this and the energy consumption of it considering I'm going to use it only a few moments every now and then. Is there a way you can share a prebuilt VM I can run? I have a Linux laptop I can use.

[fmartingr]

I still need to do proper testing, but it seems that the golang-migrate library uses modernc.org/sqlite underneath as well:

$ ag modernc.org vendor/**/*.go | grep -v vendor/modernc.org
vendor/github.com/golang-migrate/migrate/v4/database/sqlite/sqlite.go:16:       _ "modernc.org/sqlite"

[fmartingr]

PR #876 should be the last thing needed for this.

[fmartingr]

Hey @pacoesteban can you test this? It should be ready to work on OpenBSD. I tried adding custom CI to run the tests on OpenBSD but I couldn't make it work. Is there some way for us to automate or get notified if we break openbsd support? We are only offering it best-effort and making no promises, but it would be good to know early in the process and not when the changes are already released.

[pacoesteban]

Hey @pacoesteban can you test this? It should be ready to work on OpenBSD. I tried adding custom CI to run the tests on OpenBSD but I couldn't make it work. Is there some way for us to automate or get notified if we break openbsd support? We are only offering it best-effort and making no promises, but it would be good to know early in the process and not when the changes are already released.

yep, this seems to work in all my tests.

About the CI, I think it's because -race is not supported on OpenBSD. If I run the unittest Makefile target removing -race from GO_TEST_FLAGS, then tests pass. I don't really use GH actions much outside of work (nor do I want to go anywhere near them if I don't have to), so I have no idea if there will be other problems derived from the way the test machines are built.
Maybe we could override that variable when testing on OpenBSD and give it a try, if you're ok with it.

[fmartingr]

Success? :)