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 a plugin system #119

Closed
wants to merge 26 commits into from
Closed

Add a plugin system #119

wants to merge 26 commits into from

Conversation

natanbc
Copy link

@natanbc natanbc commented Jun 20, 2018

Currently it allows loading from both jars and folders with classes.

An example implementation can be found in LavalinkServer/src/example-plugin

update example config
set core pool sizes to at least one
add example plugin
call onStart() after SocketServer is ready
 - DirectoryPluginFinder$FileIterator now returns all files, not just classes
 - PluginFinder keeps a cache of all known entries, generated on first use
 - PluginFinder subclasses must also define a createURL(String) method
 - allow disabling individual plugins
 - log plugin names during load
@freyacodes freyacodes added this to the v3.0 milestone Jun 21, 2018
@freyacodes
Copy link
Member

freyacodes commented Jun 21, 2018

It looks like the classloading is handled after Spring has started. To my understanding, this means that dependency injection isn't applied to these classes. If plugins are subject to dependency injection it would allow much greater flexibility.

The obvious problem is that the classloading requires the plugin configuration to be loaded by Spring. Perhaps this problem can be solved by using an initial spring context to only loads this configuration before the full application is launched.

@freyacodes freyacodes self-requested a review June 21, 2018 10:41
@freyacodes
Copy link
Member

In addition to documentation (an example is not enough), I think this feature needs a boilerplate project that developers can fork. This would probably be a separate repo with an example plugin similar to the one you showed.

Would you be interested in working on such a boilerplate, or should I?

@natanbc
Copy link
Author

natanbc commented Jun 26, 2018

I tried getting jitpack to build the server so it could be added as a dependency on the boilerplate project, but there's currently no support for jdk 10, so this happens. A temporary workaround would be setting server source/target compatibility to 9, at the cost of not using java 10 features (although none are used currently, and building with target as java 9 works).

@schnapster
Copy link
Contributor

Put this into the jitpack.yml:

before_install:
- wget https://github.com/sormuras/bach/raw/master/install-jdk.sh
- source install-jdk.sh -F 10 -L GPL

@schnapster
Copy link
Contributor

schnapster commented Jun 26, 2018

Why are you trying to publish the server via jitpack tho? It's a standalone application, not a library.

@natanbc
Copy link
Author

natanbc commented Jun 27, 2018

Why are you trying to publish the server via jitpack tho?

To be able to write this

This would probably be a separate repo with an example plugin similar to the one you showed.

without having to copy lavalink classes/source to the project

@schnapster
Copy link
Contributor

Imo that means Lavalink should publish a public API / contract, not the whole server project.

move PluginManager#callOnOpen() invocation to the correct place
@natanbc
Copy link
Author

natanbc commented Jun 27, 2018

Linkink to the server allows plugins to access server classes, and eg read server configurations with spring dependency injection.

While a public API could be implemented to provide similar functionality, it would limit access to server code or require adding all methods to it, which is why I chose to have the server as a (compileOnly) dependency.

@schnapster
Copy link
Contributor

schnapster commented Jun 27, 2018

No public API / contract and importing the whole thing means downstream will just have to eat any changes to the server project.

@natanbc
Copy link
Author

natanbc commented Jul 4, 2018

@napstr how's this?

@schnapster
Copy link
Contributor

Creating an interface of all the important internal classes does not make an acceptable public API in my eyes.

@schnapster
Copy link
Contributor

This PR seems, for a reason I don't understand, insisting on making internal Lavalink classes public API.
That is a concern very separate from, even if dependent on by, a plugin system, and I suggest doing that in a separate PR so that existing contributors and especially the parties interested in having a public API of lavalink classes have a chance at defining, which, if any, parts of Lavalink should be a public API.
A well written draft document about how Lavalink works will surely help for that to move forward.

@freyacodes
Copy link
Member

@natanbc it would be great if you outlined what sort of functionality you want plugins to access. It's difficult to construct an API if you don't know what the goal is. Judging by your code one such goal is to support custom WS ops.

Let's say we want plugins to define custom WS ops. Instead of directly overriding the methods in the WS server, we could instead allow plugins to register ops on a per-plugin basis, which will be invoked if the ops are not already defined directly in the server. This wouldn't touch on any internal class from the plugin's perspective.

@freyacodes freyacodes removed their request for review July 20, 2018 09:48
@freyacodes freyacodes removed this from the v3.0 milestone Jul 20, 2018
@freyacodes freyacodes mentioned this pull request Jul 20, 2018
4 tasks
@freyacodes freyacodes changed the base branch from v3 to v4 July 24, 2018 15:39
@freyacodes freyacodes closed this Sep 30, 2018
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.

None yet

3 participants