Skip to content
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

Add support for modernc.org/sqlite #608

Closed
wants to merge 4 commits into from
Closed

Conversation

cugu
Copy link

@cugu cugu commented Dec 31, 2024

This PR adds a new dbmate driver for https://modernc.org/sqlite.

Some Go projects like sqlc moved away from github.com/mattn/go-sqlite3 to modernc.org/sqlite which offers sqlite support without cgo.

This PR adds a new dbmate driver for modernc.org/sqlite with the following changes:

  • The new package pkg/driver/sqlite/internal contains the common sqlite code for both sqlite drivers. This is mostly just moved over from the previous pkg/driver/sqlite package, not sure why git did not pick this up.
  • The new pkg/driver/sqlite/modernc contains a dbmate driver for modernc.org/sqlite. It imports modernc.org/sqlite and registers the appropriate dbmate drivers. Also contains the tests for this dbmate driver.
  • The updated pkg/driver/sqlite package still imports github.com/mattn/go-sqlite3 and registers the appropriate dbmate drivers. This is a backwards compatible wrapper of the internal package now and still contains tests for this driver.
  • main_non_cgo.go imports the new modernc driver, so dbmate can be build without cgo now and still has sqlite support.

@amacneil
Copy link
Owner

Thanks for the PR

  • Could you explain more about why this matters for your workflow? Most people are using a compiled binary of dbmate so the fact that we use cgo is irrelevant
  • What is modernc/sqlite? The website you linked is very light on details and just says "CGo-free port of the C SQLite3 library". What does that mean? Does it mean they ported sqlite to go? Or this is somehow a precompiled sqlite?
  • If we did move to "modernc" sqlite, why keep the existing sqlite driver at all? Are they incompatible?

@cugu
Copy link
Author

cugu commented Jan 11, 2025

Could you explain more about why this matters for your workflow? Most people are using a compiled binary of dbmate so the fact that we use cgo is irrelevant

I am using dbmate as a library in some projects, here is an open source example if you are interested. I prefer to avoid cgo, to make (cross-) compilation easier. I adapted the dbmate sqlite driver to use modernc.org/sqlite: cugu/fomo/…/dbmate_sqlite.go and wanted to share it here in case others want to use it as well.

What is modernc/sqlite? The website you linked is very light on details and just says "CGo-free port of the C SQLite3 library". What does that mean? Does it mean they ported sqlite to go? Or this is somehow a precompiled sqlite?

modernc.org/sqlite is created by translating the sqlite C code to Go code using https://modernc.org/cc. So the sqlite is native Go code then, but autogenerated and not pleasant to read. It is precompiled in the sense that the C code is compiled to Go, but it does not contain any sqlite binary.

If we did move to "modernc" sqlite, why keep the existing sqlite driver at all? Are they incompatible?

The existing driver, using github.com/mattn/go-sqlite3, might be faster and for other projects that use it anyway it might make sense to not add both sqlite drivers as dependencies.

Also for fully moving to modernc.org/sqlite I felt this would be a more disruptive change here and I tried to not break anything. It would make (cross-) compilation of dbmate easier though as well.

@amacneil
Copy link
Owner

amacneil commented Jan 14, 2025

Thanks for the explanation. I've put some thought into this and don't think it is a good feature to merge into core dbmate.

It would be better for you to maintain separately (as you stated, this version of sqlite is only helpful when using dbmate as a library, and when using dbmate as a library you can always import your own custom driver while still using our upstream core packages).

  • Dbmate's philosophy is to keep things simple, and having multiple sqlite drivers goes against that. People would need to know which driver to use and why. If there was a better sqlite driver available then we could switch wholesale but I don't like the idea of having two drivers.
  • Transpiling C to Go sounds error prone to me, meaning we will get esoteric bug reports that are going to end up being due to this driver (it already happens even with the mainstream drivers we use, so this can only make things worse)
  • Our users depend on us to make good security choices for them since they connect us to their production infrastructure, and this modernc project is lacking even a website and who is behind the project. Since they use a custom domain to host the go source code, it could easily be changed out from under us without the audit trail of a github-hosted dependency. While I can't review every line of code from every dependency we include, we try to use very popular dependencies which means more eyes from the community have reviewed them.
  • Most importantly, dbmate is a side project, and we are responsible for maintaining and fixing bugs for anything that merges into core, and I don't think the additional maintenance burden is worth it here

@amacneil amacneil closed this Jan 14, 2025
@cugu cugu deleted the modernc_support branch January 14, 2025 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants