Page MenuHomePhabricator

[native][web] Add fts5 extension to SQLite
ClosedPublic

Authored by inka on Jul 8 2024, 7:07 AM.
Tags
None
Referenced Files
F3357829: D12693.diff
Sun, Nov 24, 1:50 AM
F3357277: D12693.id.diff
Sat, Nov 23, 11:21 PM
Unknown Object (File)
Thu, Nov 21, 6:29 AM
Unknown Object (File)
Wed, Nov 20, 2:16 PM
Unknown Object (File)
Wed, Nov 20, 2:16 PM
Unknown Object (File)
Fri, Nov 15, 2:48 AM
Unknown Object (File)
Thu, Nov 14, 2:53 PM
Unknown Object (File)
Fri, Nov 8, 12:25 PM
Subscribers

Details

Summary

issue: ENG-8784
FTS5 docs: https://www.sqlite.org/fts5.html
To add fts5 we need to add a compile flag to SQLite. We need to do this on web, iOS and Android.
For iOS I had to add patch to our SQLCipher amalgamation

Test Plan

Before leaving, @kamil instructed me to verify the .wasm size changes caused by adding the library.
I did the following:
1.Checked size of web/shared-worker/_generated/comm_query_executor.wasm
2.Added -DSQLITE_ENABLE_FTS5 flag to SQLITE_COMPILATION_FLAGS in web/scripts/run_emscripten.sh
3.Ran yarn cleaninstall
4.Ran yarn build-db-wasm
5.Checked the size again

The first size was 2 233 808 bytes, the second 2 334 379 bytes. That doesn't seem too bad.

I also added code that creates a virtual fts5 table (next diff) and run web, iOS and Andorid. Checked that no errors are shown.

Diff Detail

Repository
rCOMM Comm
Branch
inka/search
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

I don't quite understand why so many changes happened in native/ios/Comm.xcodeproj/project.pbxproj, does this look concerning?

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 8 2024, 7:23 AM
Harbormaster failed remote builds in B30199: Diff 42111!
inka requested review of this revision.Jul 8 2024, 8:56 AM
patches/@commapp+sqlcipher-amalgamation+4.5.5-d.patch
1–23 ↗(On Diff #42119)

Instead of this patch, we should simply make a new release of this library. Let me know when you're ready for this and I can make it

native/ios/Comm.xcodeproj/project.pbxproj
1417 ↗(On Diff #42119)

I am afraid that it is my fault that this huge change appeared. I told you to manually set necessary CFLAGS in XCode. I think that this is not necessary and setting then inside podspec of SQLCipherAmalgamation is enough. @inka could you please revert this particular change and repeat the testing plan?

1608 ↗(On Diff #42119)

Same as above

inka planned changes to this revision.Jul 11 2024, 7:20 AM

Remove manual changes in XCode. I had to rever the changes in native/ios/Comm.xcodeproj/project.pbxproj for all of them to disappear.
I ran yarn cleaninstall and install pods and checked that removing and reinstalling the iOS app still doesn't throw

This revision is now accepted and ready to land.Jul 12 2024, 3:12 AM

Should I make a new release of @commapp/sqlcipher-amalgamation now? We should do that before landing, so we don't need to commit the patch

@ashoat I think we can make the release now. Should I do anything to make that possible?

I've made a release. Can you remove patches/@commapp+sqlcipher-amalgamation+4.5.5-d.patch here, and instead update the version in native/package.json to 4.5.5-e?

Remove patch and update package.json

inka planned changes to this revision.Jul 18 2024, 5:19 AM

I need to update podfile

This revision is now accepted and ready to land.Jul 18 2024, 5:51 AM
This revision was automatically updated to reflect the committed changes.