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] custom sqlite and sqlcipher #30824

Merged
merged 6 commits into from
Aug 9, 2024
Merged

[sqlite] custom sqlite and sqlcipher #30824

merged 6 commits into from
Aug 9, 2024

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Aug 5, 2024

Why

close ENG-11976
close ENG-12401

How

  • to fix the incompatible symbol with system sqlite on ios, we now replace sqlite3_* api as exsqlite3_*.
  • we then have to store sqlite3 source code on our git repo as well as publish to npm. in theory we can download sqlite and replace symbols on build time, but then we will have difficulty to run ./configure on windows to sqlite3.c building. publishing npm would be easier.
  • sqlcipher is then feasible.

the scripts/prepare_sqlite.ts is for downloading sqlite and sqlcipher. the scripts/replace_symbols.ts is to replace exsqlite3_ in both sqlite, sqlcipher and our source code. both these scripts are for maintenance when we need to upgrade sqlite version.

the expo-sqlite tarball size will increase from 2.9MB to 7.7MB

Test Plan

  • ci passed
  • add sqlcipher test case in test-suite and add expo.sqlite.enableFTS to **ios/Podfile.properties.json` / android/gradle.properties to bare-expo for testing.

Checklist

Copy link
Contributor Author

Kudo commented Aug 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Kudo and the rest of your teammates on Graphite Graphite

@expo-bot expo-bot added the bot: needs changes ExpoBot found things that don't meet our guidelines label Aug 5, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented Aug 5, 2024

The Pull Request introduced fingerprint changes against the base commit: dea2ae0

Fingerprint diff
[
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-sqlite/android",
      "reasons": [
        "expoAutolinkingAndroid"
      ],
      "hash": "ad80ae03660f3a20cba5a2be1b6d6fc059993937"
    }
  },
  {
    "op": "changed",
    "source": {
      "type": "dir",
      "filePath": "../../packages/expo-sqlite/ios",
      "reasons": [
        "expoAutolinkingIos"
      ],
      "hash": "68771f69ca4bd94990e69d5a2f38fcc584816689"
    }
  }
]

Generated by PR labeler 🤖

@Kudo Kudo mentioned this pull request Aug 5, 2024
3 tasks
@Kudo Kudo changed the title [sqlite] add script to customize sqlite3 symbol [sqlite] custom sqlite and sqlcipher Aug 5, 2024
@Kudo Kudo marked this pull request as ready for review August 5, 2024 19:24
@Kudo Kudo requested a review from alanjhughes as a code owner August 5, 2024 19:24

int crsqlite_init_from_swift(sqlite3 *db) {
sqlite3_enable_load_extension(db, 1);
exsqlite3_enable_load_extension(db, 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This reminds me. We should remove crsqlite. It's no longer maintained. Matt has moved on to other things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we've talked this with Brent. we would remove it from sdk 53 and give a deprecated warning in sdk 52
https://linear.app/expo/issue/ENG-12984/deprecate-enablecrsqlite-and-warn-when-used

@expo-bot
Copy link
Collaborator

expo-bot commented Aug 8, 2024

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

❌ Error: Forbidden file size or format


Generated by ExpoBot 🤖 against 7bd33ae

Copy link
Contributor Author

Kudo commented Aug 9, 2024

Merge activity

  • Aug 8, 10:54 PM EDT: @Kudo started a stack merge that includes this pull request via Graphite.
  • Aug 8, 10:54 PM EDT: @Kudo merged this pull request with Graphite.

@Kudo Kudo merged commit f2f335d into main Aug 9, 2024
12 checks passed
@Kudo Kudo deleted the @kudo/sqlite-symbol-1 branch August 9, 2024 02:54
Kudo added a commit that referenced this pull request Aug 9, 2024
# Why

we forced to use third-party sqlite in expo-updates because #24375. it caused ios build issue like #30546 if lib uses system sqlite. with #30824, the third-party sqlite is not necessary anymore.

close ENG-12866

# How

rollback to use system sqlite by default but still provides a configurable `expo.updates.use3rdPartySQLitePod` for **ios/Podfile.properties.json** to change it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: needs changes ExpoBot found things that don't meet our guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants