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

Important plugin #3890

Merged
merged 7 commits into from
Aug 4, 2020
Merged

Conversation

ZmnSCPxj
Copy link
Collaborator

Closes #3213

Builtin plugins are automatically made important.

@ZmnSCPxj ZmnSCPxj requested a review from cdecker as a code owner July 30, 2020 07:11
@ZmnSCPxj ZmnSCPxj force-pushed the important-plugin branch 3 times, most recently from 58c0493 to 7ea27bd Compare July 31, 2020 02:39
@ZmnSCPxj
Copy link
Collaborator Author

Fix documentation (we need to say --unimportant-plugin before --disable-plugin) and also make sure test for bcli replacement uses --unimportant-plugin.

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

Mostly copy suggestions et al.

The API for this seems a bit clunky, especially the requirement to list both unimportant and disable (and ordered, yikes) as well as the important-plugin + plugin.

Minimal IF* states that we should do the minimal amount possible to achieve the goal, which I think would be just the addition of important-plugin. If the need for an unimportant-plugin disparate from disable arises later, it can be added.

* This is a fanciful interpretation of MINIMAL_IF

lightningd/lightningd.h Outdated Show resolved Hide resolved
lightningd/options.c Outdated Show resolved Hide resolved
lightningd/plugin.c Outdated Show resolved Hide resolved
lightningd/plugin.c Outdated Show resolved Hide resolved
lightningd/plugin.c Outdated Show resolved Hide resolved
lightningd/plugin.h Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
tests/test_plugin.py Outdated Show resolved Hide resolved
@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Aug 2, 2020

Updated as per @niftynei feedback.

@ZmnSCPxj ZmnSCPxj force-pushed the important-plugin branch 3 times, most recently from ba37732 to d93db11 Compare August 3, 2020 01:29
@rustyrussell
Copy link
Contributor

Looks good, but Travis is deeply unhappy...

…lightningd also terminates.

Changelog-Added: New option `--important-plugin` loads a plugin is so important that if it dies, `lightningd` will exit rather than continue.  You can still `--disable-plugin` it, however, which trumps `--important-plugin` and it will not be started at all.
Changelog-Changed: plugin: Builtin plugins are now marked as important, and if they crash, will cause C-lightning to stop as well.
…velopers who want to mess around with the builtin plugins.
@ZmnSCPxj
Copy link
Collaborator Author

ZmnSCPxj commented Aug 4, 2020

Fixed the valgrind bug that was making travis unhappy (why did travis keep rerunning the test tho???) and added --dev-builtin-plugins-unimportant option.

Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK f69c694

@niftynei niftynei merged commit 1ded3fc into ElementsProject:master Aug 4, 2020
@ZmnSCPxj ZmnSCPxj deleted the important-plugin branch August 5, 2020 02:07
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.

"Crucial" plugins
4 participants