-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added configuration for using specific SQLite versions. #50
base: main
Are you sure you want to change the base?
Conversation
included in the `sqlite` module. A mismatch in the module and the dynamically | ||
loaded library may result in Python failing to load, which may happen if we | ||
use an SQLite version that is older than the system version. | ||
- Debian and Ubuntu use a custom `CFLAGS` variable to compile their distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
SQLite is normally bundled in the Python installation using the version | ||
available on the system where Python is compiled. We use the Python Docker image | ||
based on Debian `bookworm`, which has SQLite 3.40.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated https://code.djangoproject.com/wiki/SupportedDatabaseVersions to better reflect our policy for SQLite.
SQLITE_CFLAGS="-DSQLITE_ENABLE_DESERIALIZE \ | ||
-DSQLITE_ENABLE_JSON1 \ | ||
-DSQLITE_MAX_VARIABLE_NUMBER=32766" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explained this before in more detail in django/django#18899 (comment), but here's a breakdown of each flag:
SQLITE_ENABLE_DESERIALIZE
This flag was not enabled by default until SQLite 3.36. Without this flag, you'll encounter an error like pyenv/pyenv#2625.
- This is because Python checks for the availability of the deserialize functions at compile time.
- If we don't want to add this flag, we can work around this by using an older version of Python that didn't have support for
serialize()
(< 3.11), or by usingbullseye
(which ships with 3.34.1 and thus Python 3.12 wasn't built to have SQLite deserialize functions). Or, of course, compiling Python from source ourselves.
SQLITE_ENABLE_JSON1
This flag was not enabled by default until SQLite 3.38.
We have the following test that fails if you run it with a database backend that does not support JSONField
, because it's missing the @skipUnlessDBFeature
decorator:
You can verify this by setting SQLITE_VERSION
to a version < 3.38 and removing -DSQLITE_ENABLE_JSON1
from SQLITE_CFLAGS
, or by using a version >= 3.38 and adding -DSQLITE_OMIT_JSON
.
Happy to raise a ticket and PR for that.
Edit: I've raised a ticket https://code.djangoproject.com/ticket/36156#ticket
SQLITE_MAX_VARIABLE_NUMBER
There are a few tests that would fail when it does ContentType.objects.all().delete()
due to a row count query exceeding the variable limit. This only happens when the whole test suite is run, but not when the failing tests are run in isolation (likely because other tests would create more ContentType
instances).
This flag defaulted to 999 in < 3.32 (and 32766 in >= 3.32.0).
This is something that Debian/Ubuntu has customized, even in 3.31.
Related:
if [ -f libsqlite3.so ]; then | ||
cp libsqlite3.so /tmp/ | ||
else | ||
cp .libs/libsqlite3.so /tmp/ | ||
fi | ||
rm -rf /tmp/sqlite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "configure" script underwent major refactoring in 3.48.0: https://www.sqlite.org/releaselog/3_48_0.html
As a result, the compiled library is placed directly in the source root. In older versions, it's placed inside a .libs
directory.
The rm -rf
is just a cleanup step to keep the image size small.
@@ -285,6 +289,38 @@ services: | |||
|
|||
sqlite: | |||
<<: *base | |||
image: django-docker-box:${PYTHON_IMPLEMENTATION}-${PYTHON_VERSION}-sqlite${SQLITE_VERSION} | |||
pull_policy: never |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't push the image to Docker Hub. This ensures doing FROM django-docker-box:${PYTHON_IMPLEMENTATION}-${PYTHON_VERSION}
won't make Docker try to pull it from Docker Hub if the image isn't available locally.
Could also try build
instead, but if I recall correctly it doesn't work in this context unless you already built the base image. Using build
is even worse as it will run the build process even if the image has already been built. It does use the cache, but it also adds a few seconds to the run, compared to never
that would just skip it.
Thanks so much for your work on this @laymonage! From reviewing your changes I think the tradeoff of sticking This is way better than what we ever had when I was pulling my hairs compiling multiple versions of SQLite and managing them to pinpoint the exact version that regressed and filed orf#25. I'll give it a shot tomorrow trying to spike a solution to the the SQLite max query parameters issue but from a my relative exposure to Docker everything seemed right. |
Fixes #11, built on top of #49.
There are a few caveats as noted in the README. Will add more details in PR comments.
Marking as draft for now, as ideally the SpatiaLite service should also pick up the SQLite version, and possibly allow for a specific SpatiaLite version. I spent hours digging into it but I encountered either segfaults, uninitialized DB errors, or missing library errors.
Here's my WIP patch for SpatiaLite if you want to look into it, but please note that it contains commands that are redundant as I'm still experimenting different combinations.
Details
I'm not a Docker expert, so any suggestions to improve the setup would be very much appreciated.