-
Notifications
You must be signed in to change notification settings - Fork 40
feat: add SQLite3 support to CPython WASI build #195
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
Conversation
dicej
left a comment
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.
Thanks for this!
LGTM overall, but see the comment inline about using Path::display. In general, we should use Path::to_str when passing paths to command options, etc., using Path::display only for output that humans will see (e.g. println! statements). I see that build.rs was already breaking that rule, so I'll fix the existing cases.
build.rs
Outdated
|
|
||
| _sqlite3 _sqlite/blob.c _sqlite/connection.c _sqlite/cursor.c _sqlite/microprotocols.c _sqlite/module.c _sqlite/prepare_protocol.c _sqlite/row.c _sqlite/statement.c _sqlite/util.c -I{include} -L{lib} -lsqlite3 | ||
| "#, | ||
| include = deps_dir.join("include").display(), |
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.
Per the docs, Path::display does a lossy conversion, which can cause a subtle, hard to diagnose failure in a context like this. Something like .to_str().ok_or_else(|| anyhow!("non-utf8 path"))? would make it fail earlier and more explicitly.
I just realized the existing maybe_make_cpython and build_zlib functions do the same thing; I'll fix that.
|
Regarding the increased |
build.rs
Outdated
| ), | ||
| ) | ||
| .arg("--host=wasm32-wasi") | ||
| .arg(format!("--prefix={}", install_dir.display())) |
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.
Here's another place we should use Path::to_str
build.rs
Outdated
| .env("CC", wasi_sdk.join("bin/clang")) | ||
| .env("RANLIB", wasi_sdk.join("bin/ranlib")) | ||
| .env("CFLAGS", &sqlite_cflags) | ||
| .arg(format!("AR={}", wasi_sdk.join("bin/ar").display())) |
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.
Should use Path::to_str here.
Downloads and builds SQLite 3.51.2 as a static library, then links it into libpython3.14.so. The _sqlite3 module is enabled via Modules/Setup.local. WASI-specific SQLite configuration: - SQLITE_OMIT_WAL (no mmap in WASI preview1) - SQLITE_OMIT_LOAD_EXTENSION (no dlopen) - SQLITE_THREADSAFE=0 (single-threaded WASM) Adds ~500KB to libpython3.14.so.zst (7.3MB → 7.8MB).
Path::display() does a lossy conversion which can cause subtle, hard-to-diagnose failures when passing paths to compiler commands. Use to_str() with explicit error handling instead, as suggested in PR review. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
GitHub CI is having issues. Going to close and reopen this to bump it. |
Downloads and builds SQLite 3.51.2 as a static library, then links it into libpython3.14.so. The _sqlite3 module is enabled via Modules/Setup.local.
WASI-specific SQLite configuration:
Adds ~500KB to libpython3.14.so.zst (7.3MB → 7.8MB).
I've used the resulting Python build in eryx and it works just fine, but there may be some other requirements for componentize-py I'm not aware of. Happy to iterate if you have suggestions!