Add option to auto-load built-in extension #3541
Conversation
|
You can download the test builds from the following link: https://github.com/sqlitebrowser/sqlitebrowser/releases/tag/83649a3-3357 |
|
The concept sounds good. 😄 With the new Does it only work if the |
|
No, the two options work independently of each other. |
|
|
|
Cool. I don't remember C++ enough to really review this. Hopefully @MKleusberg or @mgrojo have a few minutes to look at it? The code itself seems pretty short. 😄 |
|
Thank you for the review. In fact, I'm wondering if there's a better way to get the path fo the extension. Additionally, issue #3503 enlightened me: we have another |
|
Well, "so far" our extension support has been a bit of a mess. Though stuff improves as people feel motivated to put time into it. 😁 The old PR #1930 ("Create SQLite extensions pack installer for Windows") is still open, and that was created back in 2019. So it'd be good to get a fairly complete (but simple) approach in place, like this sqlean package. We can hopefully get it working across all our supported OS's at some point. 😄 For now though, having the sqlean one be (optionally) automatically loaded sounds like a win. And we should probably have that option turned on by default so things "work out of the box" for the majority of our users. Allowing other extensions to be (manually) added to that dialog for loading also gives options to the people that need something special. |
|
Yes, that makes sense. and I'll also have 'sqlean' built-in for Windows builds too in the near future. :) |
That makes sense too. It might have already been considered, but it doesn't hurt to ask. 😄 |
|
Yes, I will complete the Windows side first and then ask to the original author. :) |
|
I don't oppose to the PR, but ideally, we should be agnostic from the application about what extensions we deploy along with it and automatically load. It should also be OS independent. As I suggested in #1930, we could generate an initial configuration for the list of loaded extensions from what we are installing. One way to do it is to call the application itself in this way after determining the list of extensions: The truth is that This will allow users to later remove some of the loaded extensions and not others, if they want to use different versions or implementations. The directory of the built-in extensions could include a copy of |
|
Although I understand your sentiment, there is a specific reason for this PR.
|
This doesn't really sound right to me, as there aren't competing sides who we'd be picking favourites from. We're making this GUI for our users, and it'll make things that little bit easier for them if we have the sqlean extension enabled by default. It's one less thing for people to have to worry about, and if their situation is some kind of edge case then they can disable it anyway. Does that make sense? |
Probably, I didn't explain myself correctly. It's not that we should be neutral, what I mean is that 'sqlean' (or any other extension that we distributed and install) should be loaded as default, but through our usual configuration mechanism. That is, I would prefer that users see, when starting the application after installation, just a list of preloaded extensions that they can just edit as usual, adding new extensions or removing them.
Agreed. My remark was only about how to do that, not about not doing it at all. But if we don't find another way, then I don't mind if this enters the codebase. It's only that I would prefer a more generic approach. |
|
What if we simply do something like this: diff --git a/src/Settings.cpp b/src/Settings.cpp
index bb1e31c4..b36c4187 100644
--- a/src/Settings.cpp
+++ b/src/Settings.cpp
@@ -403,8 +403,13 @@ QVariant Settings::getDefaultValue(const std::string& group, const std::string&
return true;
// extensions/list?
- if(group == "extensions" && name == "list")
+ if(group == "extensions" && name == "list") {
+#ifdef Q_OS_MACX
+ return QStringList(qApp->applicationDirPath() + "/../Extensions/sqlean.dylib");
+#else
return QStringList();
+#endif
+ }
// extensions/disableregex?
if(group == "extension" && name == "disableregex") |
|
Got it. So, if I understand correctly, instead of offering only one option like this PR, I draw a simple prototype to make sure I understood it well :) |
|
I was thinking of just treating them as any other extension that the user has loaded, but done automatically by default (like in the patch I suggested). But your approach is also good, and has the advantage of not losing the reference to the file if the user unselects some built-in extension. |
|
In app bundles on macOS, built-in extensions are stored and distributed in the |
|
For now, this PR requires additional code changes, so I'll move it to draft. |
|
I've modified the code based on the review, and I'm ready for a new review now. Now, get a list of extensions that are included in the app bundle, and use checkboxes to let the user decide whether or not to laod them by checking or unchecking them. (Implemented as shown in the screenshot above) Thank you. :) |
lucydodo
changed the title
|
Awesome. I can't do a proper technical review of this personally. But I did notice some small potential items that seemed a bit strange, so made comments on those. The potential change of the |
src/PreferencesDialog.cpp
Outdated
| void PreferencesDialog::listUpBuiltinExtension() | ||
| { | ||
| const QDir dir(qApp->applicationDirPath() + "/../Extensions/"); | ||
| QStringList files = dir.entryList(QStringList() << "*.dylib", QDir::Files); |
There was a problem hiding this comment.
I think this is the only part preventing this new feature to be portable. The "*.dylib" could be made OS-dependent and all the #ifdef Q_OS_MACX removed.
There was a problem hiding this comment.
Aren't we starting with doing these on macOS first, and then expanding out to other OS's if it goes well?
There was a problem hiding this comment.
Yes, I was actually going to implement the feature for Windows after I finished this PR, but i's okay to do it in this PR. However, I'm not sure if Linux has a place in the executable file to store the extension library.
There was a problem hiding this comment.
At least on Debian/Ubuntu, if you install an SQLite extension, it will be placed under /usr/lib/x86_64-linux-gnu/mod_*.so. Thus, it would make sense to use the path qApp->applicationDirPath() + "/../" + QSysInfo::currentCpuArchitecture + "-linux-gnu/mod-*.so". With that, when the user installs the official Debian sqlitebrowser package and any SQLite extension package, the extension will be listed as an available extension to load. In that way, we will save the user from having to search the extension after the installation.
If this defers per Linux distribution, we might choose to support only one, or try to detect the distribution using QSysInfo::productType().
No problem if this is done for macOS only because it is urgent for the release, and then generalized for other OS's. But in general, we should prefer to make everything as common and portable as possible.
There was a problem hiding this comment.
Thank you so much. This will go a long way toward to resolving the issue.
Let's take a look at a few popular Linux distributions and see where other distributions store their extensions.
As an aside, I personally think it's better not to include this PR in this release. 🤔
There was a problem hiding this comment.
Yes, I don't know much about Linux-side distributions, so I'll wait for others to chime in. 😄
There was a problem hiding this comment.
You're missing the prefix used by SQLite extensions (mod_*.so). On my system:
$ ls /usr/lib/x86_64-linux-gnu/mod_*.so
/usr/lib/x86_64-linux-gnu/mod_spatialite.so
/usr/lib/x86_64-linux-gnu/mod_virtualpg.so
$ ls /usr/lib/x86_64-linux-gnu/*.so | wc -l
261There was a problem hiding this comment.
Oops, just saw that libsqlite3-mod-csvtable uses a different convention: /usr/lib/x86_64-linux-gnu/libsqlite3_mod_csvtable.so. So not so easy, but maybe just a matter of using *mod_*.so, but might be fragile.
There was a problem hiding this comment.
Checking list of packages on Debian, most of them are libsqlite3_mod_*.so, but there are a few weird cases, like https://packages.debian.org/sid/amd64/libsqlite3-mod-ceph/filelist
There was a problem hiding this comment.
I didn't realize I needed to add that prefix. Thanks for letting me know.
But then I thoguht, this is a PR that implements auto-loading for the built-extensions that our application provides, so maybe we don't need to automatially load SQLite extensions installed by external packages on the user's system? (If user need it, user can still add that extension in the Preferences today)
Alternatively, I think we could implement an additional feature to find and automatically load SQLite extensions that users have installed on their system. (Maybe feature name will be 'Load System-wide Installed SQLite Extensions). 🤔
This patch changes
macOS* users can set to automatically load thesqleanextension when loading DB files.This PR is related to issue #3357.
Detailed Changes and Code-Flow
Changes
Add a checkbox to the 'Extension' tab of the 'PreferencesDialog' for "Automatically load the 'sqlean' extension"
Code-Flow
sqlitebrowser/src/sqlitedb.cpp
Lines 2174 to 2181 in 83649a3
When loading the DB, get the path to the current app execution if that option is enabled, then locate and load the extension.
(This extension path is determined during packaging, see the following collapse section for more information)
Part of the CI workflow for adding the `sqlean` extension during packaging
sqlitebrowser/.github/actions/notarize-macos/action.yml
Lines 37 to 58 in 0038f0f
and if it fails to load the
sqleanextension, it will warn the user and deselect the option.Screenshot
Test Environment
Outdated
Thank you.