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

Enable extension loading #178

Closed
wants to merge 1 commit into from
Closed

Conversation

FaFre
Copy link
Contributor

@FaFre FaFre commented Sep 1, 2023

This PR is a draft to enable manual extensions loading via manual C-API call (https://www.sqlite.org/c3ref/enable_load_extension.html) for the built binaries.

Also I removed SQLITE_OMIT_GET_TABLE which is removing functions that are required by some extensions e.g. spatialite.

Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

I'm not strictly opposed to this, but we'd also have to check these changes against the iOS/macOS dependencies and patch the cmake in my sqlite3-native-libraries repository for Android as well.

Hopefully we'll soon get a native Dart build system which would allow us to make the compile time options configurable for users.

@@ -44,7 +44,6 @@
#define SQLITE_OMIT_AUTHORIZATION
#define SQLITE_UNTESTABLE
#define SQLITE_OMIT_COMPILEOPTION_DIAGS
#define SQLITE_OMIT_LOAD_EXTENSION
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we can really use dlopen with wasm either way. So it can probably stay disabled if it keeps the size of the compiled code smaller.

@blaugold
Copy link

blaugold commented Sep 28, 2023

Just FYI, the native assets feature and toolchain integration is far enough along to compile SQLite with it on all Dart supported native platforms. I've started playing around with a package that provides an SQLite API and makes use of native assets. Here is the build.dart file that defines how to build the native code, for reference. It's awesome how easy this makes shipping native code.

Support for native assets is available for standalone Dart when passing --enable-experiment=native-assets. On the Flutter side, support has landed for iOS, macOS, Linux and Windows on the master channel, with the PR for Android already open.

@simolus3
Copy link
Owner

That's awesome @blaugold! It's also pretty neat that we're able to use NativeCallables now.

The only feature missing for this library is being able to access native fields with this functionality (dart-lang/sdk#50551). We use that to bind sqlite3_temp_directory. It's currently blocked on a refactoring of how @Native functions are implemented, I hope I'll be able to implement it quickly once that blocker is gone.

I'll see if I can already integrate native assets for the other things though.

@blaugold
Copy link

As a workaround for accessing sqlite3_temp_directory you could do something like this: blaugold/sqlite_dart_native@a4fd7c4

@simolus3
Copy link
Owner

I've played around with it a bit in #185 - I also had to apply some workarounds for variadic @Native functions, but it works.

@blaugold
Copy link

I've played around with it a bit in #185 - I also had to apply some workarounds for variadic @Native functions, but it works.

Nice! 🤓

@mugikhan
Copy link
Contributor

mugikhan commented May 30, 2024

Hi @simolus3 This is the exact functionality I am also looking for. I need to enable extension loading specifically for linux and windows.
Can I ask why you would not want to enable extension loading?

I can create a separate PR to remove only the SQLITE_OMIT_LOAD_EXTENSION flag but would like to clarify first.

@simolus3
Copy link
Owner

Can I ask why you would not want to enable extension loading?

My main concern is that sqlite_flutter_libs should behave similarly on all platforms it supports. Enabling extension loading only on Windows and Linux introduces platform-specific behavior that I'm trying to avoid.
I also don't want to enable extensions (which may bring security concerns) for all users unconditionally.

At the moment we're using native buildscripts to compile sqlite3 (CMake on Windows+Linux, NDK on Android and CocoaPods on iOS and macOS). This makes it impossible for users to customize the sqlite3 that they're getting in the end. Once native assets are stable, we can migrate from sqlite3_flutter_libs towards a solution that works for all Dart projects and doesn't require any platform-specific builds. That would also be easier to customize, so we can offer options to enable extension loading for those who need it.

If you only need Windows and Linux either way, you can pretty much copy the CMakeLists.txt from this repository and adapt them to your needs to enable extensions. You pretty much just have to copy these parts, replacing $PLUGIN_NAME with sqlite3. Then you put target_link_libraries(${BINARY_NAME} PRIVATE sqlite3) somewhere and you have sqlite3 in your Flutter app without sqlite3_flutter_libs.

@mugikhan
Copy link
Contributor

Can I ask why you would not want to enable extension loading?

My main concern is that sqlite_flutter_libs should behave similarly on all platforms it supports. Enabling extension loading only on Windows and Linux introduces platform-specific behavior that I'm trying to avoid. I also don't want to enable extensions (which may bring security concerns) for all users unconditionally.

At the moment we're using native buildscripts to compile sqlite3 (CMake on Windows+Linux, NDK on Android and CocoaPods on iOS and macOS). This makes it impossible for users to customize the sqlite3 that they're getting in the end. Once native assets are stable, we can migrate from sqlite3_flutter_libs towards a solution that works for all Dart projects and doesn't require any platform-specific builds. That would also be easier to customize, so we can offer options to enable extension loading for those who need it.

If you only need Windows and Linux either way, you can pretty much copy the CMakeLists.txt from this repository and adapt them to your needs to enable extensions. You pretty much just have to copy these parts, replacing $PLUGIN_NAME with sqlite3. Then you put target_link_libraries(${BINARY_NAME} PRIVATE sqlite3) somewhere and you have sqlite3 in your Flutter app without sqlite3_flutter_libs.

I forgot to mention that it is already enabled on Android, iOS and macOS. There is an extension that I am loading and it works correctly on all those platforms (gradle on Android and pod on iOS and macOS). I was wondering why it isn't enabled for the remaining two platforms. My testing only indicated Windows and Linux as an issue unless I am missing something.

Copy link
Owner

@simolus3 simolus3 left a comment

Choose a reason for hiding this comment

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

Oops - I have to apologize for pointlessly leaving this hanging so long then! I think in that case it's justified to get extension support working on Windows and Linux as well. I've just checked, we also don't have OMIT_GET_TABLE on Android/iOS/macOS.

So I'm fine with this except that OMIT_LOAD_EXTENSION should still be there for wasm builds, since there's no way to support it there at the moment.

@mugikhan
Copy link
Contributor

mugikhan commented Jun 1, 2024

Oops - I have to apologize for pointlessly leaving this hanging so long then! I think in that case it's justified to get extension support working on Windows and Linux as well. I've just checked, we also don't have OMIT_GET_TABLE on Android/iOS/macOS.

So I'm fine with this except that OMIT_LOAD_EXTENSION should still be there for wasm builds, since there's no way to support it there at the moment.

I'm not sure if @FaFre is still looking for this functionality or if he will be able to merge this change. @simolus3 I also see this PR removes the OMIT_LOAD_EXTENSION flag for wasm.

I can create a new PR if @FaFre cannot make those changes and merge this PR. Does sqlite_flutter_libs and sqlcipher_flutter_libs require a minor or patch version bump?

@simolus3
Copy link
Owner

simolus3 commented Jun 1, 2024

A new PR would be helpful, thanks!

Does sqlite_flutter_libs and sqlcipher_flutter_libs require a minor or patch version bump?

I think a patch bump is fine, this is more of a consistency fix than a new feature.

@mugikhan mugikhan mentioned this pull request Jun 3, 2024
@FaFre
Copy link
Contributor Author

FaFre commented Jun 12, 2024

Cool that this found its way into the package now! Guess I can close this now :)

@FaFre FaFre closed this Jun 12, 2024
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.

4 participants