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

Add loadext plugin #3226

Merged
merged 2 commits into from Apr 21, 2019

Conversation

Projects
None yet
2 participants
@jackwilsdon
Copy link
Member

commented Apr 20, 2019

This is a rough draft at the moment of what a loadext plugin could look like for loading SQLite extensions. This plugin can be used to fix #3160 by allowing users to load the ICU plugin like so;

loadext:
  - libicu

Note that this will require the user to compile their own copy of ext/icu from SQLite sources.

Points of Discussion

I've got a few points of discussion I'd like to bring up regarding this PR;

  1. The elephant in the room;

    # TODO: Find a way of getting a connection without calling a private method.
    conn = lib._connection()

    This is certainly less than ideal, as we're reaching into beets internals from this plugin. There are two is one possible solutions I can think of to this;

    • Use SELECT load_extension(...) This doesn't work properly, see this thread regarding the issues.
    • Add a load_extension method to dbcore.Database.
  2. Where should these extension paths be relative to? In this initial draft the paths are relative to wherever beets is running, and as such running beets in different folders breaks this unless you use an absolute path for the extension. I think using the beets config directory for relative paths is probably the best idea, but I'll leave this open to others to decide.

  3. How in depth do we want to go into the documentation with this? This plugin seems like a somewhat advanced feature, as it requires the user to compile their own ext/icu extension from the SQLite sources.

  4. How do we test this? Do we mock out the load_extension method and make sure it's called? Or do we compile the ICU extension on Travis and attempt to call one of the methods provided by it?

Compiling the ICU extension

To compile the ICU extension, you need the following;

  • gcc
  • icu-devtools
  • libicu-dev
  • libicu60 (or any other version I guess?)
  • libsqlite3-dev
$ wget https://sqlite.org/2019/sqlite-src-3280000.zip
$ unzip sqlite-src-3280000.zip
$ cd sqlite-src-3280000/ext/icu
$ gcc -shared -fPIC icu.c `icu-config --ldflags` -o libicu.so
$ cp libicu.so [wherever you want]

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch from 3553c20 to 3267ec5 Apr 20, 2019

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Awesome!! Thanks for getting this started!

Here are a few thoughts on your questions:

  1. A load_extension method in Database sounds like just the right thing. 😃
  2. I think relative to the beets configuration directory would be the most predictable. In fact, that's what Confuse's as_filename will do by default. It makes sense to recommend that users put their extensions in that directory.
  3. Yeah, it does seem pretty advanced. I think a good place to start would be to leave it up to advanced users to accomplish for themselves, mostly—you could include your snippet about how to compile the ICU extension, for example, but surround it with language that says "this is for advanced users, but you will want to do something like this, although the details will vary based on your system."
  4. Perhaps this is heresy, but I think we can get away without tests for something this simple (and this hard to test)? Compiling the ICU extension on Travis sounds like a little bit of a nightmare, although it's possible…
@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

I've just realised that connections in beets are per-thread, which opens up an issue of the extension being loaded on one thread but not another. Would we be alright iterating through all of the open connections and loading the extension for all of them?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Aha; I didn't realize connections were per thread! Maybe the right thing to do, then, would be for the Database to keep track of which plugins the application needs and to load them whenever it creates a new connection?

@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Yeah that makes sense to me. sqlite3 itself doesn't do a connection per thread, but it seems our code is doing it instead.

beets/beets/dbcore/db.py

Lines 885 to 896 in 0f8a748

def _connection(self):
"""Get a SQLite connection object to the underlying database.
One connection object is created per thread.
"""
thread_id = threading.current_thread().ident
with self._shared_map_lock:
if thread_id in self._connections:
return self._connections[thread_id]
else:
conn = self._create_connection()
self._connections[thread_id] = conn
return conn

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch 2 times, most recently from 039df7c to e76652c Apr 20, 2019

Show resolved Hide resolved beetsplug/loadext.py Outdated
@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

It's true—the reason we do that is that SQLite doesn't allow connections to be shared across threads. See the docs for the check_same_threads option for some more details.

Anyway, this looks awesome!! Seems like this should totally work. 😃

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch 2 times, most recently from bdb52c4 to efed541 Apr 20, 2019

@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

Just to put another spanner in the works (see the footnotes).

Oddly enough it's enabled on the versions of Python 2 and 3 I have installed from Homebrew on macOS, as well as on Python 2 and 3 I have installed from apt on Ubuntu.

My thoughts are now as follows:

  1. Add a supports_extensions property to dbcore.Database, which is backed by hasattr(sqlite3.Connection, 'enable_load_extension')
  2. Throw an exception when dbcore.Database#load_extension is called if supports_extensions is false
  3. Check if supports_extensions is true when loadext starts, logging a warning (or error?) if loadext is enabled but the user's Python SQLite installation does not support extensions

What do you think @sampsyo?

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Wow; that's really too bad! So close, and yet so far.

All three of your proposals sound right to me. I think it's fine for loadext to warn and do nothing if it detects that extensions are not supported.

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch 2 times, most recently from c9bf4d3 to 08ffa01 Apr 21, 2019

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch from 08ffa01 to 1be374a Apr 21, 2019

@jackwilsdon jackwilsdon marked this pull request as ready for review Apr 21, 2019

@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Alright I think this is ready for a final review now 😁 I'm not exactly great with documentation, so any feedback there is greatly appreciated!

@sampsyo
Copy link
Member

left a comment

Very nice!! Here are just a few documentation suggestions.

=====================

Beets uses an SQLite database to store and query library information, which
has support for extensions to extend its functionality.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 21, 2019

Member

Maybe an additional sentence here:

The loadext plugin lets you enable these SQLite extensions for beets.

-------------

To configure the plugin, make a ``loadext`` section in your configuration
file. The section must consist of a list of paths to extensions to load.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 21, 2019

Member

I recommend putting the example right here, instead of later down in the last section. You can introduce it by saying something like "The section should look like this:" and list the YAML right there. This makes the introduction mostly self-contained.

- libicu-dev
- libsqlite3-dev

The following commands can be then be used to compile the ICU extension:

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 21, 2019

Member

I recommend a slightly different wording:

Here's roughly how to download, build, and install the extension (although the details might depend on your system):

$ unzip sqlite-src-3280000.zip
$ cd sqlite-src-3280000/ext/icu
$ gcc -shared -fPIC icu.c `icu-config --ldflags` -o libicu.so
$ cp libicu.so [your beets config directory]

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 21, 2019

Member

To make this a little more digestible, I'd probably just say:

$ cp libicu.so ~/.config/beets

Readers (advanced ones, anyway) will be able to sort out that they need to think about where to copy the file.

configuration directory.

No file extension is required, as SQLite will automatically use the correct
extension for the current platform.

This comment has been minimized.

Copy link
@sampsyo

sampsyo Apr 21, 2019

Member

The correct dynamic library extension for the current platform?

@jackwilsdon jackwilsdon force-pushed the jackwilsdon:loadext branch from 1be374a to f5f9aed Apr 21, 2019

@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Thanks for the feedback on the documentation! I think that's all corrected now 👍

@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

Awesome work!! I'll hit the green button now. 😃 🚢

@sampsyo sampsyo merged commit 84c29d0 into beetbox:master Apr 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jackwilsdon jackwilsdon deleted the jackwilsdon:loadext branch Apr 21, 2019

@jackwilsdon

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

Much appreciated! Thanks for all the help regarding the review 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.