Skip to content

Conversation

@Austinpayne
Copy link
Contributor

@Austinpayne Austinpayne commented Nov 11, 2022

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

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent start! Some issues did pop up, comments inline as usual 🙂

* Removes dependence on a system vendored sqlite3
* Includes swift script for bumping to the latest patch version, inspecting
  the system's sqlite3 compile options, and updating to an arbitrary version
@Austinpayne
Copy link
Contributor Author

@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.

@gwynne gwynne added enhancement New feature or request semver-minor Contains new APIs labels Nov 15, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022

Codecov Report

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

Additional details and impacted files
@@           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 to find out more.

Impacted Files Coverage Δ
Sources/SQLiteNIO/SQLiteConnection.swift 61.34% <33.33%> (ø)
Sources/SQLiteNIO/SQLiteStatement.swift 79.84% <76.47%> (ø)

@gwynne
Copy link
Member

gwynne commented Dec 4, 2022

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?

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 💯

@gwynne gwynne changed the title Embed sqlite amalgamation v3.39.4 source code Embed sqlite amalgamation v3.40.0 source code Dec 4, 2022
@Austinpayne
Copy link
Contributor Author

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
Copy link
Member

gwynne commented Dec 4, 2022

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
Copy link
Member

gwynne commented Dec 4, 2022

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

@gwynne gwynne merged commit 3b93e0a into vapor:main Dec 4, 2022
@VaporBot
Copy link

VaporBot commented Dec 4, 2022

These changes are now available in 1.3.0

@Austinpayne Austinpayne deleted the feature/embedded-libsqlite branch February 13, 2023 22:03
gwynne pushed a commit to danramteke/sqlite-nio that referenced this pull request Feb 22, 2023
* Embed sqlite amalgamation v3.40.0 source code
* Includes swift script for bumping to the latest patch version, inspecting
  the system's sqlite3 compile options, and updating to an arbitrary version
* Use sqlite serialized threading on linux
* Use async interfaces in vendor-sqlite3
* Prefix sqlite amalgamation symbols with "sqlite_nio_"
* Update CI to not depend on system libsqlite3
* Adjust CSQLite compile options to Ubuntu 22.04

Co-authored-by: Austin Payne <austin@passivelogic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request semver-minor Contains new APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

libSqlite should be embedded, not loaded from the host system

4 participants