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

Use sqlite amalgamation for compatibility #50

Open
chippmann opened this issue Jun 28, 2021 · 3 comments
Open

Use sqlite amalgamation for compatibility #50

chippmann opened this issue Jun 28, 2021 · 3 comments

Comments

@chippmann
Copy link
Contributor

chippmann commented Jun 28, 2021

I propose to use the sqlite amalgamation (https://www.sqlite.org/amalgamation.html) rather than linking the libsqlite3 library which is installed on the host.

This has several advantages:

  • It is more portable:
    • as it does not rely on the sqlite version installed on the building host
    • as it does not depend on installed library versions on the building host (like glibc, which was an issue in this PR). This also means that developers on linux can run the tests locally as well even with very recent linux installs
  • Consumer of this library don't have to link sqlite themselves
  • We don't have to link sqlite ourselves
  • It should (at least according to the sqlite docs) give a slight performance boost

While i already have a working prototype branch using sqliter in conjunction with sqldelight one test fails:


Can you @kpgalligan tell me why and what this hardcoded timeout is and why it's set to 1300?
Because with my prototyping branch the actual value is slightly lower (around 1000) and hence I'm unsure if this is a bug in my prototyping branch or not.

@kpgalligan
Copy link
Contributor

The timeout in the config is 1500, so I'm thinking I picked 1300 as a reasonable value that would mean the writer waited for an amount of time. It won't always be "1500" or greater, because we're creating a statement and starting a query. You could rewire that to start the timer before starting the read I guess?

It's a relatively obscure config, as most people would pick WAL. Android, by default, does not use WAL, even though Room does. Were it not for Android support, I would likely only support WAL in the driver.

Anyway, on "amalgamation", I'm not quite sure I understand. If it's just shipping the sqlite binary rather than linking to the one on from the host, I would say that depends on the situation. If we're talking just linux, I don't have much of an opinion, to be honest. If we're talking about also doing that for iOS, while I understand there are some benefits, I would implement it as some kind of extension, or an alternative option. Not as default, for sure. My opinion on this is it's up to the implementer to decide which sqlite to link to. It can be the system one, or one you provide. That is true on iOS now. We only link to the system one for testing purposes.

We do alternate linking for sqlcipher, and I know of at least one implementation that ships with their own sqlite. If "amalgamation" is something else, we can discuss (I'm doing a whole bunch of Monday meetings, so haven't really digested the link).

@kpgalligan
Copy link
Contributor

Having said that, sqldelight adds that linker flag in the plugin, and you need to explicitly tell it not to if you don't want that. Use linkSqlite = false in the db config in gradle. If you do static frameworks, it won't propagate, but dynamic will (IIRC, that is maybe a cocoapods thing instead, but it's been a while since I've had to sort that out in a new config).

@chippmann
Copy link
Contributor Author

I see. Yeah will try that.
Hmm yeah i see your point. And i didn't think of the case where a user of this lib would link to something else.
It's not really "shipping with your binary", sqlite becomes "part of your binary". But still the same regarding our discussion here I guess.

Given your explanation, I definitely agree that this should not be the default then.
If you're open for the idea in general, I'll look into if I find a form of extension or config setup which allows for the needed flexibility and ease of use.

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

No branches or pull requests

2 participants