Page MenuHomePhabricator

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

Authored by inka on Jul 8 2024, 7:07 AM.
Tags
None
Referenced Files
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
Unknown Object (File)
Sat, Nov 2, 2:08 AM
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
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.