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

WIP: Plugins v3 #2419

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

WIP: Plugins v3 #2419

wants to merge 15 commits into from

Conversation

phw
Copy link
Member

@phw phw commented Apr 20, 2024

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

This is the work in progress of a proposed rework of the plugin system.

The main part here is an initial specification of the plugin API in PLUGINS.md. The document builds upon the extended discussion of requirements for a new plugin system on the wiki. But currently the focus is on the plugin API and loading inside Picard, not so much on the distributions side. But @zas had some good ideas here of using git, and parts of this are documented in the spec

Beside the document there is only very rough prototyping to evaluate the ideas a bit. The very last commit does do some initial plugin loading, but this was committed as current state very much in the middle of the work.

This is IMHO not yet ready for review, but as I had little time the recent weeks let's have this up here to build upon. So far it was mostly @zas and myself discussing some ideas. I think the general direction is solid, but we should put this up for a wider discussion.

Solution

Action

Additional actions required:

  • Update Picard documentation (please include a reference to this PR)
  • Other (please specify below)

PLUGINS.md Outdated Show resolved Hide resolved
PLUGINS.md Show resolved Hide resolved
allow plugins to provide translations for user facing strings.

Plugins could provide gettext `.mo` files that will be loaded under a plugin
specific translation domain.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would that work?

  1. Plugins have their own domain, but cannot access picard domains, this may be a problem for constants/attributes from database
  2. Plugins have their own domain, a set of methods from plugin API to use it ((), N() are those from Picard, P_() and PN_() for plugins)
  3. We append plugin domain to the main one (chaining fallbacks https://docs.python.org/3/library/gettext.html#gettext.NullTranslations.add_fallback), basically Picard main domain is used, but plugin "extends" it, standard picard/i18n are used
  4. ?

We need to test each approach to find the best.
Loading plugin translations can be either automatic (we expect .mo files to be somewhere identical for all plugins), or by a specific call from the plugin to the API to indicate where to find translations (decided by plugin author). I tend to prefer the first approach, as it is simple, safer, and "normalize" plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

My preference would also be 1. I think plugins should have their own translation generally. For thing like attribute translations we have separate domains anyway. If this becomes a requirement we could consider exposing this via the API as well.

Opiton 3. sounds interesting though, we should probably try how well this works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually using fallbacks isn't a good idea, as it may leads to conflicts between strings (if we search main domain first and plugin has a different translation for the same string, it will not appear, and the reverse searching plugin domain first and falling back on main, would let plugin translations to override main translations).

Also .mo aren't supposed to be portable between systems.
Though GNU gettext is able to take care of endianess at runtime it seems, it is usually preferable to compile .po to .mo on target system.
An interesting thread about that: https://stackoverflow.com/questions/53285634/is-there-a-portable-way-to-provide-localization-of-a-package-distributed-on-pypi

@zas
Copy link
Collaborator

zas commented Jun 3, 2024

@phw, I rebased onto master and added fixes for imports related to extension_points changes

@zas
Copy link
Collaborator

zas commented Jun 3, 2024

@phw : I fixed dependencies in 8951ddf though I'm not sure that's the correct way of doing it for those, please check.

@Sophist-UK
Copy link
Contributor

In PLUGINS.md you ask whether toml is the best format for the manifest file. I have had a quick look and I think that (because it has a lot of similarities with windows ancient-style ini files, that it is more easily understood and edited than json or yaml files.

I am surprised that you guys have decided to adopt a one-plugin-per-repository model in order to support external 3rd-party (i.e. dubious security) git repos. I know that this can potentially be controlled by limiting the repos to metabrainz owned repos, where you guys can set the security permissions etc., but in practice this will become very unwieldy and become very insecure very quickly. It is also likely that 3rd party repos will have default security (i.e. insecure) which allow merges by any Github user. I urge you to reconsider this approach, because I consider it extremely insecure and therefore likely to eventually be used for malicious purposes and become a brand-image issue. (I really really really do NOT want to be saying "Told you so" in a few years (or months) time.)

I see nothing in the pygit2 calling code to verify (by a hash or other means) that the local clone matches the git main source version (and has not been maliciously modified locally).

I see nothing in the code which asks the user explicitly to authorise manually installed and locally modified plugins - the hashes for these should be stored locally and reauthorisation requested if the local source code changes.

I see no changes to the options page for plugins to distinguish between valid (i.e. has matching hashes) centrally downloaded plugins, manually installed plugins, and plugins that were downloaded from a central repo but have since been locally modified. IMO this should be done by having several sections to the plugin list (manually installed and locally modified sections only being shown when needed - so for most users they will continue to see only a single section).

When using git cloning, I am unclear how you tell it to download a release rather than the current commit (which may be a WIP). I am also unclear how the plugins website / API will know when a new release is published.

Future plugin upgrades e.g. from v3 to v4: By allowing external plugin repos you are going to make it MUCH more difficult to migrate to a newer incompatible api. At present you can create a v3 branch in the musicbrainz/picard-plugins repo and edit each plugin to meet the new standard. How will you do this when there are hundreds of separate external repos?

@zas / @phw Zas / Philipp - I urge you to reconsider this approach.

@zas
Copy link
Collaborator

zas commented Jun 3, 2024

@Sophist-UK We'll continue to support "official" plugins, and there will be a list of them from an official website. Nothing changes here.
We go for one repo per plugin to ease maintenance and contributions, to separate commit history of each plugin, and to allow easy forking.

User will get a warning about installing "third-party" plugins.

I see no changes to the options page for plugins

Yes, that's why that's a Draft Pull Request, with WIP in the title... we hardly started to implement this.

I urge you to reconsider this approach.

And what do you suggest exactly?

@phw
Copy link
Member Author

phw commented Jun 3, 2024

@Sophist-UK This PR is all WIP right now. The primary goal of the existing code was to validate and prototype the concepts we had in mind for the new plugin. Also the primary focus so far has been on the plugin structure and API, and ideas about distribution are just coming together.

But having said this, having the ability to allow third-party plugin repositories is definitely one of the goals. We have seen that the single repo controlled by Picard developers doesn't scale. This leads to situations like the classical extras.

We can (and will) still have official repositories. And we can war users about using third party plugins. But in the end the users can already now add plugins from other places. I don't see how the repository system is much different. But it definitely makes it easier for both users and developers.

@rdswift
Copy link
Collaborator

rdswift commented Jun 3, 2024

Sorry I haven't had much chance to go over this yet.

So each (third-party) plugin will have to be in a separate repo, right? I can work with that, but it means that I'll need to create a bunch of new repos 😀 and it won't be quite as easy to get a full list of all of my plugins. Not a big deal, and I'm sure I can work around that somehow.

Am I understanding correctly that the third-party plugins will still be installed the same way (by downloading and installing a zip file), and that active plugins will be checked for updates (by querying the plugin repo) during Picard startup? Sort of like subscribing to a plugin?

I'm wondering if a potential enhancement might be to allow the user to enter (paste) a url to the plugin repo and Picard could do the download and install?

Another possible enhancement might be to allow the user to subscribe to a plugin provider so they can see all plugins available from that provider (similar to the way that they can see all the plugins available from picard.musicbrainz.org/plugins).

@zas
Copy link
Collaborator

zas commented Jun 3, 2024

I'm wondering if a potential enhancement might be to allow the user to enter (paste) a url to the plugin repo and Picard could do the download and install?

There will be 2 main sources of plugins:

  • local directories (no upgrade procedure, what's in the directory is the plugin, it will allow to use no or whatever source control system)
  • local and remote git repositories: a local repository accessible on local medias, or a remote repository, installation by cloning, upgrading by updating working tree

There will be 2 ways to add plugins:

  • adding a local directory (with .git or not)
  • adding a remote repository specified by url & ref

There will be an "official" plugin selector:

  • a list is provided via Picard website API, listing url & ref for known plugins
  • optionally we can support lists from other sources, but that's not planned yet

The plugin UI will provide means to:

  • select a local directory (manual git repo or simple directory)
  • select a remote url + ref (manual git repo)
  • select a plugin from a list (automatic download, from remote git repo)
  • select a different ref for a git installed plugin
  • if commits are signed, show an icon indicating the signature was validated.

A way for the user to change the ref used by a git-based plugin: it will let a dev to run its beta version, or a user to select a certain version of the plugin. If ref doesn't match "official" list then it's considered a user-installed plugin, and it will not be upgraded automatically.

Using git will provide:

Every plugin has 3 mandatory things:

  • a MANIFEST file
  • a Python module structure (that's at least init.py file)
  • a directory containing both

So we drop support for single file plugins (because we want to separate metadata from plugin code, hence MANIFEST file).
We also drop support for zipped plugins (nothing prevents to share a plugin as a zipped directory, and unzip it locally), it will simplify plugin code.

That's more or less our thoughts about this at this point. But things can evolved along the way, that's work in progress.

@Sophist-UK
Copy link
Contributor

@phw Philipp - so not so much WIP as an experiment? Nothing wrong with that, but it does mean different things than WIP.

I don't have any issues for e.g. a well supported plugin by a reputable known developer having a separate source (and Classical Extras almost certainly fits this profile) - and we can provide guidelines to ensure that the repo is secure.

But that is wildly different from A) switching to Git without ensuring that it will be secure; and B) insisting that every plugin has its own repo (which IMO might be unmanageable in practice - there isn't a huge influx of new plugins, so more of a big one-off change than an ongoing overhead, but still more likely to have human errors in security settings, and any churn on the change approval team will be a big overhead too).

Also, git does NOT have a concept of releases (and no concept of beta / full releases either) - these are Github enhancements. Git does have a concept of Tags which you could use (if you insist on being aligned to semantic-versioning).

I would suggest that we have 5 tiers of plugins:

  1. Primary picard-plugins repo, v3 branch.
  2. Primary external repos - for special cases like Classical Extras which are somewhere between 1. and 3. in terms of MusicBrainz brand support.
  3. Secondary picard-community-plugins, v3 branch - just like 1. except the requirements for plugins to be added and updated are less stringent and less supported (i.e. the older, less supported, less frequently used existing plugins that the core Picard team doesn't want to support). (Classical Extras might fit here or might not.)

All the above would be in the Picard plugin catalogue.

  1. Other external repos - specified by git URL including a commit hash - you cannot add a repo without specifying a hash. Picard provides upgrade support by the user supplying a new URL with a different commit hash.
  2. Manually installed plugins.

I have no real issue with using a git clone as a means of downloading so long as there is good central and local security control for any plugins in the catalogue and good local security controls and warnings for any plugins not in the catalogue.

@zas
Copy link
Collaborator

zas commented Jun 4, 2024

Supporting one repo with multiple plugins isn't excluded. But it shouldn't be mandatory either.

So we could support 2 structures:
topdir/MANIFEST and topdir/plugin/MANIFEST

For plugin "release" version, we can rely on version from MANIFEST: we update the plugin code only when this version is increased. Then we need to separate code repository from code of the installed plugin. Also what happens when code was changed but version not updated, or the reverse. It would be much more reliable to use git tags as "releases" imho.
But then what about non-git directories containing plugins...
Not clear yet.

Repos are resynced on regular basis (automatically or manually). But installing a new version only happens if the version from MANIFEST changed.

Actually it would be convenient to manage lists of plugins in a git repo too, so we can easily update it without redeploying Picard website.

Each listed plugin has simple metadata: a list of plugin UUIDs, a git URL with a ref (let's called this "Official Plugin List")

Each plugin has a MANIFEST listing a version, an UUID (matching the list above), and the rest of metadata (name, description, etc)

On first sync, Picard clones official repo containing "Official Plugin List", loads it, and for each of entries, clone matching repositories locally (let's call that "Installable plugin repos").

Then it looks for all MANIFEST files in "Installable plugin repos/repo_name/" or "Installable plugin repos/repo_name/subdir".
repo_name has to be unique, it is based on URL + ref.
If MANIFEST contains one UUID that's listed in "Official Plugin List" then it extracts plugin metadata (version, name, description, etc...)

Now the user can choose to install one plugin:

  • he selects one of the available plugins
  • the plugin code is copied to "Installed Plugins/UUID" and the module can then be loaded

Now let's explore the evolution over time:

  • user uninstalls a plugin: "Installed Plugins/UUID" is totally removed
  • use disables a plugin: "Installed Plugins/UUID" is left untouched, but the plugin is marked as disabled, and the module is not loaded (or unloaded if it was)
  • the plugin disappears from "Official Plugin List" (=its UUID doesn't appear anymore): the plugin is left untouched but a warning informs the user, and asks if he wants to keep it, disable it, uninstall it
  • the plugin is moved to another repo (UUID is preserved, but URL + ref change): the plugin is left untouched but a warning informs the user, and asks if he wants to keep it, disable it, uninstall it, or resync from new repo
  • a new plugin is added to "Official Plugin List": it just appears as installable (as above)
  • user wants to use a different version of the plugin (ie. the current version is bugged, the previous one was working): we go through MANIFEST file history (a command like git log --source -S 'version = ' --patch -- MANIFEST.toml could detect changes of versions) to list previous "released" versions, or let the user selects a specific ref
  • If the "Installed Plugin" is set to a different ref, upgrade is disabled until manually re-enabled.

Installing a plugin is basically: cloning to a temp dir (in the destination), with a certain revision, removing .git directory, renaming temp dir to final dir, unloading/reloading the plugin if it was loaded.

I really think we need an UUID for each plugin to handle changes of sources (author moves the git repo elsewhere, but the plugin is still the same). We could use an "unique name" (org.metabrainz.picard.plugins.pluginA) instead of an UUID perhaps, not sure about this.

Still many questions.

@Sophist-UK
Copy link
Contributor

  1. We probably cannot use Git Clone for downloading plugins which are in a multiple-plugin repo because cloning is (AFAIK) an all-or-nothing thing at a particular commit. The current download of ZIP files seems to me to work pretty well, but what do I know of the difficulties you guys face on a day-to-day basis, but if not this, then what?

  2. I agree that we probably need to handle moves from one repository to another. I am not sure that any GUIDs are the way to go because they might introduce a security vulnerability by someone spoofing it. The only means I can see is to retain a global catalogue of MB-supported and community-supported plugins - and Picard has to consult that directory to get the list of the plugins, their versions, their hashes and their download locations.

  3. However we do need to ensure that each plugin has a unique directory - and I don't see any security issues with using a GUID or (better) a unique directory name per plugin (and we should probably insist that the unique name is never changed).

- they'll be used to install or update plugins
- the idea is to support both raw directories and git repos from local media
- and of course support for git remote repositories
- PluginSources are expected to be configured by UI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants