Skip to content

Plugin directories #2127

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Dec 5, 2018
Merged

Plugin directories #2127

merged 6 commits into from
Dec 5, 2018

Conversation

rustyrussell
Copy link
Contributor

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 December 3, 2018 04:31
ZmnSCPxj
ZmnSCPxj reviewed Dec 3, 2018
View reviewed changes
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and rebased....

Copy link
Contributor

Choose a reason for hiding this comment

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

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 guilt/plugin-dir branch 3 times, most recently from a2d2560 to e8139ec Compare December 3, 2018 09:00
@rustyrussell
doc: Use subsections for option types.
993a86d 
And drive-by fix "if it's seems" to "if it seems".

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
cdecker
cdecker previously requested changes Dec 3, 2018
View reviewed changes
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

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

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

tests/test_plugin.py Outdated
@@ -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'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

lightningd/options.c Outdated
@@ -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).");
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack...

lightningd/plugin.c Outdated

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

Choose a reason for hiding this comment

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

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.

lightningd/plugin.c Outdated
if (!strstarts(di->d_name, "lplugin_"))
continue;
/* Ignore emacs backup files. */
if (strends(di->d_name, "~"))
Copy link
Member

Choose a reason for hiding this comment

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

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

tests/utils.py Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

lightningd/plugin.c
if (paths_match(p->cmd, name)) {
list_del_from(&plugins->plugins, &p->list);
tal_free(p);
removed = true;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell @cdecker
docs: document plugin options.
b95dfd0 
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell @cdecker
plugins: make log prefix the basename.
29f1659 
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
Copy link
Member

cdecker commented Dec 4, 2018

Rebased and squashed

ACK 29f1659

@cdecker cdecker merged commit 111d6df into ElementsProject:master Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZmnSCPxj ZmnSCPxj ZmnSCPxj left review comments

@cdecker cdecker cdecker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rustyrussell @cdecker @ZmnSCPxj