vapor / sqlite-nio

Non-blocking wrapper for libsqlite3-dev using SwiftNIO
MIT License
61 stars 16 forks source link

Embed sqlite amalgamation v3.40.0 source code #35

Closed Austinpayne closed 1 year ago

Austinpayne commented 1 year ago

This PR removes dependence on a system libsqlite3 by embedding the amalgamation (single source file) release of sqlite3 in vendored form. The included source code is fetched from https://sqlite.org/download.html. (#35)

Projects which include logic in their Dockerfile and/or CI workflows to ensure that the libsqlite3-dev package (or equivalent) is installed on the build image can now safely remove it, but are not required to do so. No additional actions are necessary to take advantage of any additional features supported by the embedded library; SQLKit and Fluent will detect their availability automatically when possible (most notably UPSERTsupport).

Closes #32

Austinpayne commented 1 year ago

@gwynne I think I've resolved all your comments except regarding compile options. Happy to trim them down but I am curious to get your input on the base set we'd like to use.

codecov-commenter commented 1 year ago

Codecov Report

Merging #35 (45e5108) into main (f68a2bc) will not change coverage. The diff coverage is 65.21%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #35 +/- ## ======================================= Coverage 45.68% 45.68% ======================================= Files 7 7 Lines 510 510 ======================================= Hits 233 233 Misses 277 277 ``` | Flag | Coverage Δ | | |---|---|---| | unittest | `?` | | | unittests | `45.68% <65.21%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/vapor/sqlite-nio/pull/35?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Δ | | |---|---|---| | [Sources/SQLiteNIO/SQLiteConnection.swift](https://codecov.io/gh/vapor/sqlite-nio/pull/35/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9TUUxpdGVOSU8vU1FMaXRlQ29ubmVjdGlvbi5zd2lmdA==) | `61.34% <33.33%> (ø)` | | | [Sources/SQLiteNIO/SQLiteStatement.swift](https://codecov.io/gh/vapor/sqlite-nio/pull/35/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9TUUxpdGVOSU8vU1FMaXRlU3RhdGVtZW50LnN3aWZ0) | `79.84% <76.47%> (ø)` | |
gwynne commented 1 year ago

Sorry for the long delay on this, @Austinpayne 😕 Your latest changes look good, I'm satisfied with this as is - with one caveat: I left this waiting so long, we're not on the latest anymore 😅 Can you update for SQLite 3.40.0?

Austinpayne commented 1 year ago

Still not 100% sure what to think of the TSan failure, but addressing it definitely isn't important enough to be worth blocking this badly overdue change even longer. Thanks for your diligence, @Austinpayne - this is excellent work! 💯

Yeah, same here. I'll probably do a follow up over in the SQLite Fluent driver after the merge to use RETURNING in that place if possible (and if I'm understanding that part of the fluent code correctly).

gwynne commented 1 year ago

The original PR description text is available via GitHub's edit history, but the removed portion is reproduced here for simplicity:

I have also included a utility swift script mainly for bumping to the latest patch version of sqlite so that CI can automatically query and open a patch version bump PR if desired. The script also includes the ability to an update to arbitrary version and inspect the system's sqlite3 compile options. Both update commands (bump-latest-version and update-version) prefix all externally visible SQLite symbols with sqlite_nio to avoid namespace collisions with other linked versions of SQLite. For lack of a better system, this is done quite naively and brute force by simply getting the list of symbols using nm and ar and string matching in the sqlite.c and sqlite.h files. Unfortunately, Swift's automatic importing of C macros does not support complex defines (like symbol aliasing).

On compile options, I'm not sold that these are the ones we need and simply grabbed them from a macOS 12.6 system version of sqlite and from the swift:5.6-focal docker image. However, they do work on the platforms I have tested on so far (swift:5.6-focal, swift:5.7-focal, swift:5.6-amazonlinux2, swift:5.7-amazonlinux2, and an arm64 macOS running 12.6). I think ideally the options should be more or less the same between the platforms but currently there is quite a lot more on the macOS platform.

gwynne commented 1 year ago

Overriding api-breakage check (spurious failure due to limitations of checker) and dependents check (TSan failure appears to be spurious) in order to merge.

VaporBot commented 1 year ago

These changes are now available in 1.3.0