xerial / sqlite-jdbc

SQLite JDBC Driver
Apache License 2.0
2.86k stars 619 forks source link

Create native SQLite loader chain #1164

Open belugabehr opened 3 months ago

belugabehr commented 3 months ago

Related to #1154

I have separated all of the various ways of loading the native SQLite file into their own discrete classes.

Note that all of these classes are in their own package, therefore, I can tune the logging just for this one package to get a more clear idea of what is going on in the loading of the native file.

Also, if I want to implement something like #1140 and/or #1155, then I can create the new feature as a new class, and then provide a System property to toggle which loading mechanism I want to use "default" v.s. "cache" or something like that.

gotson commented 3 months ago

hi, i don't quite see how you will solve #1140 and #1155 with this ? It seems this is just a stepping stone refactoring without any other feature ?

I am generally against that kind of preparatory work, and would prefer a PR that solves issues or brings new features.

belugabehr commented 3 months ago

Hello,

This moves all the code into a new package, so I can set my logger to more verbose just for this one package to get better understand where the native lib is being loaded from.

Added logging (at least DEBUG, maybe INFO) where the Native JDBC lib file is loaded from.

For the other changes, you pointed out correctly that all of these proposals are breaking backwards compatibility in subtle ways.

So, in order to preserve backwards functionality, and to put new features behind flags, it will be more simple:

if (featureFlag) {
    MyCacheBasedLoader.getInstance().loadSQLiteNative();
} else {
    DefaultSQLiteNativeLoaderChain.getInstance().loadSQLiteNative();
}

and would prefer a PR that solves issues or brings new features.

The feature here is the logging aspect and easy to add new loading functionality. Also, this is my first engagement with the project, so I am hesitant to spend the time delivering a new feature and then it gets rejected or ignored. I hope you understand that perspective :)

gotson commented 3 months ago

if it's just about moving the loader into it's own package for logging purposes, you can do a PR for that without all the refactoring, which IMHO may be useful, but may be bloated also. For instance it breaks the GraalVM native image tests.

Moving SQLiteJSBCLoader into its own loader subpackage could maybe suffice for now?

05nelsonm commented 1 month ago

Would also like to add any usage of Path will break android API 25 and below. Must use java.io.File.

gotson commented 1 month ago

Would also like to add any usage of Path will break android API 25 and below. Must use java.io.File.

API 25 is 8 years old, this library will not make any stretch to support old android API versions.