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

Plugin cwd cleanup #2892

Merged
merged 5 commits into from Aug 3, 2019

Conversation

@rustyrussell
Copy link
Contributor

commented Aug 2, 2019

Now we have dynamic plugins, the difference in cwd becomes an issue. Make it uniform, and bandaid for now.

Makefile: set PYTHONPATH to be absolute when running tests.
My test machine started failing on dynamic plugin tests, unable to
find the lightning module:

  ModuleNotFoundError: No module named 'lightning'

This is because plugins at startup are run from whatever directory
you're in, but on refresh are run from the lightning-dir.

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

@rustyrussell rustyrussell requested a review from darosior Aug 2, 2019

@rustyrussell rustyrussell requested a review from cdecker as a code owner Aug 2, 2019

@rustyrussell rustyrussell added this to the 0.7.2 milestone Aug 2, 2019

@rustyrussell rustyrussell added the plugin label Aug 2, 2019

@cdecker
Copy link
Member

left a comment

Tests now fail because we print logs when starting with --help, and the os import needs to be made explicit. Other than that I think this looks good.

@@ -1131,8 +1131,8 @@ def test_onchain_multihtlc_our_unilateral(node_factory, bitcoind):
# In fact, they'll fail them with WIRE_TEMPORARY_NODE_FAILURE.
# TODO Remove our reliance on HTLCs failing on startup and the need for
# this plugin
nodes[0].daemon.opts['plugin'] = 'tests/plugins/fail_htlcs.py'
nodes[-1].daemon.opts['plugin'] = 'tests/plugins/fail_htlcs.py'
nodes[0].daemon.opts['plugin'] = os.path.join(os.getcwd(), 'tests/plugins/fail_htlcs.py')

This comment has been minimized.

Copy link
@cdecker

cdecker Aug 2, 2019

Member

Might be worth making this a function somewhere. The directory fixture seems like a good place for it. But we can do that as a cleanup.

@darosior
Copy link
Collaborator

left a comment

Besides the above-mentioned fixs, looks good to me too.

rustyrussell added some commits Aug 3, 2019

pytest: use absolute paths for plugin arguments.
We're going to change this in the next patch.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
lightningd: chdir as soon as we know lightning dir.
This is easy since we did the option parsing cleanup, but it has the
effect that plugins are launched from the lightning-dir.  Now
we have dynamic plugins, this means startup and post-startup plugins
experience the same environment.

This is absolutely a desirable thing: they can just drop files in
their cwd rather than having to move (including, I might note, core
files!).

We also highlight the change in various places (and a drive-up update
of PLUGINS.md which says you have to use --plugin).

The next patch adds a backwards compatibility wedge for old users of
relative plugin paths.

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

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-cwd branch from cc3d370 to 4a54eb9 Aug 3, 2019

plugins: detect and fixup old relative paths.
Note that we move adding the plugin to the plugins list to the end, otherwise
the hook from logging can examine the (uninitialized) plugin.

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

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-cwd branch from 4a54eb9 to 8d71c46 Aug 3, 2019

lightningd: handle --version before trying to move to lightning-dir.
Otherwise it creates the lightning-dir.  This can't be helped for --help
(at least, if plugins are present), but --version simply prints and exits.

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

@rustyrussell rustyrussell force-pushed the rustyrussell:guilt/plugin-cwd branch from 8d71c46 to faf4a36 Aug 3, 2019

@rustyrussell rustyrussell merged commit 4c9bfa3 into ElementsProject:master Aug 3, 2019

1 of 2 checks passed

ackbot Need at least 1 ACKs
continuous-integration/travis-ci/pr The Travis CI build passed
Details

cdecker added a commit to cdecker/lightning that referenced this pull request Aug 5, 2019

pytest: Run plugin --help tests in the test directory
This is a followup to ElementsProject#2892. Since we now attempt to lock the PID file before
starting plugins we need to make sure that we actually use a unique lightning
directory for anything that attempts to call `--help`. If not we may be
conflicting with a `lightningd` that is running against that directory.

Notice that this still means that we will be unable to call `--help` on
`lightningd` if we have a running instance, but isolation in this case is
good, otherwise we'd be reading the default config anyway.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
@cdecker cdecker referenced this pull request Aug 5, 2019

rustyrussell added a commit to cdecker/lightning that referenced this pull request Aug 6, 2019

pytest: Run plugin --help tests in the test directory
This is a followup to ElementsProject#2892. Since we now attempt to lock the PID file before
starting plugins we need to make sure that we actually use a unique lightning
directory for anything that attempts to call `--help`. If not we may be
conflicting with a `lightningd` that is running against that directory.

Notice that this still means that we will be unable to call `--help` on
`lightningd` if we have a running instance, but isolation in this case is
good, otherwise we'd be reading the default config anyway.

Signed-off-by: Christian Decker <decker.christian@gmail.com>

ZmnSCPxj added a commit that referenced this pull request Aug 6, 2019

pytest: Run plugin --help tests in the test directory
This is a followup to #2892. Since we now attempt to lock the PID file before
starting plugins we need to make sure that we actually use a unique lightning
directory for anything that attempts to call `--help`. If not we may be
conflicting with a `lightningd` that is running against that directory.

Notice that this still means that we will be unable to call `--help` on
`lightningd` if we have a running instance, but isolation in this case is
good, otherwise we'd be reading the default config anyway.

Signed-off-by: Christian Decker <decker.christian@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.