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

sqlite-preprocessed-3350500 in third_party #162

Merged
merged 2 commits into from
May 14, 2021
Merged

Conversation

ahgamut
Copy link
Collaborator

@ahgamut ahgamut commented May 4, 2021

Similar to sqlite amalgamation (#161) but with https://www.sqlite.org/2021/sqlite-preprocessed-3350500.zip instead.

First commit adds all the files (after running clang-format), second commit adds sqlite3.mk and does the minimal necessary changes to get SQLite to build.

sqlite3.com is 1036KB with MODE=tiny.

* changed headers
* removed redundant declarations
* ran clang-format
* added sqlite3.mk
Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for your patience on the review.

@jart jart merged commit 782ca71 into jart:master May 14, 2021
@ahgamut
Copy link
Collaborator Author

ahgamut commented May 14, 2021

Yay! I'll close #161 as this got merged.

@jart
Copy link
Owner

jart commented May 14, 2021

I noticed after merging that you ran clang-format on the imported sources. The first commit is intended to be byte-for-byte identical with the upstream source. I'm going to see if I can do a ninja rewrite of your changes.

@jart
Copy link
Owner

jart commented May 14, 2021

Perfect. I did a quick force push of the change with cherry-picked onto the unformatted upstream sources so provenance is clearer. If you pulled master after I merged then you'll need to do a force pull. Sorry about that! Thanks for your contribution! SQLite shell now works like a charm:

make -j8 o//third_party/sqlite3
o/third_party/sqlite3/sqlite3.com
SQLite version 3.35.5 2021-04-19 18:32:05
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
sqlite> select 2 + 2;
4
sqlite>

@jart
Copy link
Owner

jart commented May 14, 2021

I'm also going to do my best to sure POSIX advisory locks work as advertised in a follow-up change. I need it for redbean too. It's probably the most important aspect of the systems integration.

@jart
Copy link
Owner

jart commented May 14, 2021

POSIX advisory locks are now polyfilled across platforms as of 690be54

@ahgamut
Copy link
Collaborator Author

ahgamut commented May 14, 2021

I noticed after merging that you ran clang-format on the imported sources. The first commit is intended to be byte-for-byte identical with the upstream source. I'm going to see if I can do a ninja rewrite of your changes.

I glossed over "byte-for-byte identical" in the discussion thread and ran clang-format before each commit as usual. Sorry for causing a force-push on master 😞 I'll remember to keep the first commit untouched if I add stuff to third_party from now on.

@jart
Copy link
Owner

jart commented May 14, 2021

What are you talking about? We just added 200,000+ lines of code to this codebase. I'm very happy with what you've achieved. It's also obvious reading your diffs that you're starting to really get the feel for how this monorepo works. You've also been helping a long time. The way I see it, any time I spend helping you is likely to pay dividends for the community in the long run.

@jart
Copy link
Owner

jart commented May 14, 2021

Out of curiosity, it looked like you dipped your toes in the water with the sqlite zip virtual table extension and then turned it off. What's your judgement on it? It seems like it's something potentially cool. For example:

echo 'SELECT name, sz FROM zipfile("o/tiny/third_party/sqlite3/sqlite3.com")' | 
    o/tiny/third_party/sqlite3/sqlite3.com
usr/share/zoneinfo/Beijing|554
usr/share/zoneinfo/Berlin|2335
usr/share/zoneinfo/Boulder|2453
usr/share/zoneinfo/Chicago|3585
usr/share/zoneinfo/GST|2845
usr/share/zoneinfo/Honolulu|338
usr/share/zoneinfo/Israel|2321
usr/share/zoneinfo/Japan|318
usr/share/zoneinfo/London|3687
usr/share/zoneinfo/Melbourne|2223
usr/share/zoneinfo/New_York|3545
usr/share/zoneinfo/Singapore|410

jart added a commit that referenced this pull request May 14, 2021
- Now integrated with `make tags` for Emacs IDE features
- Delete some old deprecated broken full-text search engines
- Rename .h → .inc files that don't meet our definition of header
- Make sure every #include line is normal form so tools understand

See #162
@ahgamut
Copy link
Collaborator Author

ahgamut commented May 15, 2021

I thought third_party/sqlite3/geopoly.inc was the only file that should be a .inc because it was #included exactly once into another file. Most of the .h files have include guards, so I thought .inc wasn't necessary.

#ifndef SQLITE_WAL_H
#define SQLITE_WAL_H

#include "third_party/sqlite3/sqliteInt.inc"

Changing to .inc does make that subdirectory clearer to understand: sqlite3.h and sqlite3ext.h are the only headers that anyone would look to include when embedding SQLite.

@ahgamut
Copy link
Collaborator Author

ahgamut commented May 15, 2021

Out of curiosity, it looked like you dipped your toes in the water with the sqlite zip virtual table extension and then turned it off. What's your judgement on it? It seems like it's something potentially cool.

The extension flags in sqlite3.mk of this PR are just the ones that compiled without warnings when I was trying to build the amalgamation. I used the same config when modifying the files here. I think it's pretty cool to have the above feature. I am unable to think of a direct use case right now (maybe redbean collects some minimal logging info to display in a self-updating database?), but at the very least it's another solid example of what someone could accomplish with Cosmopolitan.

It is possible to add a few more SQLite features/extensions to the build. I looked up the SQLite compile flags and tried adding a few new ones:

$(THIRD_PARTY_SQLITE3_A_OBJS):						\
		OVERRIDE_CFLAGS +=					\
			-DNDEBUG					\
			-DBUILD_sqlite					\
			-DHAVE_READLINE=0				\
			-DHAVE_EDITLINE=0				\
			-DHAVE_MALLOC_USABLE_SIZE			\
			-DHAVE_FDATASYNC				\
			-DHAVE_STRCHRNUL				\
			-DHAVE_LOCALTIME_R				\
			-DHAVE_GMTIME_R					\
			-DHAVE_USLEEP					\
			-DSQLITE_CORE					\
			-DSQLITE_OS_UNIX				\
			-DSQLITE_THREADSAFE=0				\
			-DSQLITE_HAVE_ZLIB				\
			-DSQLITE_HAVE_C99_MATH_FUNCS			\
			-DSQLITE_DEFAULT_MEMSTATUS=0			\
			-DSQLITE_ENABLE_BYTECODE_VTAB			\
			-DSQLITE_ENABLE_DBPAGE_VTAB			\
			-DSQLITE_ENABLE_DBSTAT_VTAB			\
			-DSQLITE_ENABLE_DESERIALIZE			\
			-DSQLITE_ENABLE_EXPLAIN_COMMENTS		\
			-DSQLITE_ENABLE_FTS3				\
			-DSQLITE_ENABLE_FTS3_PARENTHESIS	\
			-DSQLITE_ENABLE_FTS3_TOKENIZER		\
			-DSQLITE_ENABLE_FTS4				\
			-DSQLITE_ENABLE_FTS5				\
			-DSQLITE_ENABLE_GEOPOLY				\
			-DSQLITE_ENABLE_IOTRACE			\
			-DSQLITE_ENABLE_JSON1				\
			-DSQLITE_ENABLE_MATH_FUNCTIONS			\
			-DSQLITE_ENABLE_MEMORY_MANAGEMENT	\
			-DSQLITE_ENABLE_MEMSYS5			\
			-DSQLITE_ENABLE_OFFSET_SQL_FUNC			\
			-DSQLITE_ENABLE_PREUPDATE_HOOK			\
			-DSQLITE_ENABLE_RTREE				\
			-DSQLITE_ENABLE_SESSION				\
			-DSQLITE_ENABLE_SNAPSHOT			\
			-DSQLITE_ENABLE_STMTVTAB			\
			-DSQLITE_ENABLE_UNKNOWN_SQL_FUNCTION		\
			-DSQLITE_ENABLE_UNLOCK_NOTIFY		\
			-DSQLITE_OMIT_DECLTYPE				\
			-DSQLITE_OMIT_DEPRECATED			\
			-DSQLITE_OMIT_LOAD_EXTENSION			\
			-DSQLITE_OMIT_SHARED_CACHE			\
			-DSQLITE_TEMP_STORE=1				

Adding all these features increases binary size: sqlite3.com is 772K when none of the above extensions are enabled vs 1092K when all of them are enabled.

@justinclift
Copy link

As a data point, we (DB Browser for SQLite project) enable a bunch of extensions in our builds too, as people definitely use them and ask us for them. 😄

@jart
Copy link
Owner

jart commented May 21, 2021

@ahgamut The internal sqlite headers need to be .inc files because they don't specify their dependencies. For example, btree.h references the type sqlite3_vfs but it doesn't #include the header that defines it. So the %.h.ok check fails. I tried fixing that but the file relationships are too cyclic to be easily untangled. The Cosmopolitan codebase wants each header to be a thing in and of itself. Otherwise they can't be considered by validators independently of the .c files that include them.

I'm fine adding all the extensions. One thing we can do in the future, if we want tinier SQLite builds, is to create multiple build targets for each extension similar to how it's done in https://github.com/jart/cosmopolitan/blob/master/libc/nt/nt.mk It's a huge pain to configure, but SQLite is written in such a way where extensions can be statically yoinked in that manner.

Another thing worth considering, is companies like Apple have published SQLite configs we can reference for inspiration, which are usually reasonably well thought out. Although there's a few things here and there we might disagree with.

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

Successfully merging this pull request may close these issues.

None yet

3 participants