-
Notifications
You must be signed in to change notification settings - Fork 896
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
plugins: unique command line options, small fixes and plugin documentation #5343
plugins: unique command line options, small fixes and plugin documentation #5343
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
I need to return on it!
About halve of the builtin plugins are "dynamic", is this deliberate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great cleanup of some longstanding issues 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, these comments were sitting pending in GH for a while :(
…hen non-existent Otherwise different relative paths (e.g. /dir/plugin and /dir/../dir/plugin) to same plugin executable would not be recognized as the same plugin
…getmanifest Otherwise we hangs forever in startup when it was the last plugin, we would miss destroy_plugin --> check_plugins_manifests --> io_break e.g. when a plugin tries to register a bool option with a string as default value.
The extra entry in opt_table would never be called, leaving plugins clueless why options keep defaulting. Note that option registration outside startup does nothing. Instead, dynamic plugins can use `plugin start [second_parameter]` to pass options.
CHANGELOG: add "dynamic" field to plugin list
c322133
to
d267f27
Compare
Applied the feedback and some minor fixes, squashed and rebased. |
The most serious test fail seems a memleak in
Note the Will try to reproduce, but I don't think the problem lies in this PR but maybe it triggered a race. edit I cannot reproduce above memleak
compared to
|
This is failing elsewhere, is a general regression on master. |
ACK d267f27 Test failures are unrelated, i've saved the logs from the |
Some issues I came across looking at #5294 (that discussion is ongoing, this PR adds some context)
Converts
plugin->cmd
to a full path immediately (couldn't find a reason not to), ensures only one instance can run.Plugin command line options should have unique names (and no aliases).
Update plugin documentation.
edit
Last 28be7693 and cf2dbbbcommits is an idea for plugins that do configs internally, not fully worked out.edit Add "dynamic" field to
plugin list
result, some builtin plugins are dynamic !