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

Plugin directories #2127

Merged
merged 6 commits into from Dec 5, 2018

Conversation

Projects
None yet
3 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Dec 3, 2018

When we have default plugins (eg. pay and probe) we need infrastructure to autoload them. This is my attempt to add that.

@rustyrussell rustyrussell requested a review from cdecker Dec 3, 2018

Makefile Outdated
@@ -451,6 +453,7 @@ install-program: installdirs $(BIN_PROGRAMS) $(PKGLIBEXEC_PROGRAMS)
@$(NORMAL_INSTALL)
$(INSTALL_PROGRAM) $(BIN_PROGRAMS) $(DESTDIR)$(bindir)
$(INSTALL_PROGRAM) $(PKGLIBEXEC_PROGRAMS) $(DESTDIR)$(pkglibexecdir)
$(INSTALL_PROGRAM) plugins/lplugin_* $(DESTDIR)$(plugindir)

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Dec 3, 2018

Collaborator

This is installing files that are not specified in the Makefile rule as dependency of this rule.

This comment has been minimized.

@rustyrussell

rustyrussell Dec 3, 2018

Contributor

If they're python, they don't have deps. But yes, we should create a variable for these!

This comment has been minimized.

@rustyrussell

rustyrussell Dec 3, 2018

Contributor

Fixed and rebased....

This comment has been minimized.

@ZmnSCPxj

ZmnSCPxj Dec 4, 2018

Collaborator

I think still best to add as dependency. Someday we may want to have non-python plugins (e.g. built from C code) or python code that is generated instead of part of the distributed code. Not a big effort to add $(PLUGINS) to the dependencies line.

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-dir branch 3 times, most recently from a2d2560 to e8139ec Dec 3, 2018

doc: Use subsections for option types.
And drive-by fix "if it's seems" to "if it seems".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-dir branch from e8139ec to 0678341 Dec 3, 2018

@cdecker
Copy link
Member

cdecker left a comment

I think the OrderedDict change is worth doing in order to avoid complexity with the cli arguments in tests.

@@ -10,7 +10,7 @@ def test_option_passthrough(node_factory):
First attempts without the plugin and then with the plugin.
"""
plugin_path = 'contrib/plugins/helloworld.py'
plugin_path = 'contrib/plugins/lplugin_helloworld.py'

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

Is this really necessary? I think requiring plugins to an executable mode set should be sufficient to differentiate between plugins and regular files.

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

It's just that I've been typing the plugin paths quite a lot and any character that is not strictly needed is a big win imho :-)

This comment has been minimized.

@rustyrussell

rustyrussell Dec 4, 2018

Contributor

OK, I've modified it with your idea: must be executable, must not contain punctuation other than . - _. It's even UTF-8 aware :)

@@ -303,6 +308,9 @@ static void config_register_opts(struct lightningd *ld)
* them register more command line options */
opt_register_early_arg("--plugin", opt_add_plugin, NULL, ld,
"Add a plugin to be run.");
opt_register_early_arg("--plugin-dir", opt_add_plugin_dir,
NULL, ld,
"Add a directory to load plugins from (cumulative).");

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

I stumbled over cumulative, maybe "can be specified multiple times" would be better?

This comment has been minimized.

@rustyrussell

rustyrussell Dec 4, 2018

Contributor

Ack...


while ((di = readdir(d)) != NULL) {
/* Must start with lplugin_ */
if (!strstarts(di->d_name, "lplugin_"))

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

If we were to use the executable bit in the mode to differentiate this would mean we'd have an additional call to stat here if I'm not mistaken.

if (!strstarts(di->d_name, "lplugin_"))
continue;
/* Ignore emacs backup files. */
if (strends(di->d_name, "~"))

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

I was hoping that the mode-differentiation would save us from this, but apparently emacs syncs the backup file mode with the original.

@@ -345,7 +345,11 @@ def cmd_line(self):

opts = []
for k, v in sorted(self.opts.items()):
if v is None:
# Used to force literal options into a given order

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

We could also switch to collections.OrderedDict which returns the items in the same order they were added when iterating over them.

if (paths_match(p->cmd, name)) {
list_del_from(&plugins->plugins, &p->list);
tal_free(p);
removed = true;

This comment has been minimized.

@cdecker

cdecker Dec 3, 2018

Member

I assume the reason for not returning here, and finishing the iteration is because a plugin may be registered multiple times? Notice that this only works if the plugin doesn't register any options and no JSON-RPC methods, so a bit of a cornercase.

This comment has been minimized.

@rustyrussell

rustyrussell Dec 4, 2018

Contributor

Could have same basename in multiple directories. Yes, it's silly, but if it ever happens I want to remove all of them...

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-dir branch from 0678341 to b022ce6 Dec 4, 2018

fixups pushed

rustyrussell added some commits Dec 3, 2018

lightningd: add --plugin-dir option to load directory full of plugins.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
pytest: add hack to force options into a given order.
Needed for testing plugin options which are order-senditive.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins: add and install built-in plugin dir, add clear and disable o…
…ptions.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
docs: document plugin options.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
plugins: make log prefix the basename.
It's not perfect if they have multiple with same name, but better than number.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

@cdecker cdecker force-pushed the rustyrussell:guilt/plugin-dir branch from b022ce6 to 29f1659 Dec 4, 2018

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Dec 4, 2018

Rebased and squashed

ACK 29f1659

@cdecker cdecker merged commit 111d6df into ElementsProject:master Dec 5, 2018

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment