Correctly change the working directory in python #296

Leandros · 2018-01-19T10:03:32Z

This is implementing #295 and neovim/neovim#3636.

justinmk · 2018-01-19T10:34:45Z

Thanks! Can you add a test?

Leandros · 2018-01-19T10:53:46Z

@justinmk Sure, I've had a test written anyway.

mhinz · 2018-01-19T11:12:52Z

neovim/plugin/script_host.py

@@ -65,7 +65,9 @@ def setup(self, nvim):
        sys.stderr = RedirectStream(lambda data: nvim.err_write(data))
        # Setup the DirChanged listener
        cid = nvim.channel_id
-        nvim.command('au DirChanged * call rpcrequest(%d, "python_chdir", v:event)' % cid)
+        cmd = 'au DirChanged * call rpcrequest(%d, "python_chdir", v:event)' \


Please use autocmd instead of au, which makes it easier go grep for when searching for autocmd-related things.

Leandros · 2018-01-19T11:38:57Z

Alright, all tests ran through. Fixed the last linter warning.

bfredl · 2018-01-19T12:25:32Z

neovim/plugin/script_host.py

@@ -63,6 +63,12 @@ def setup(self, nvim):
        self.saved_stderr = sys.stderr
        sys.stdout = RedirectStream(lambda data: nvim.out_write(data))
        sys.stderr = RedirectStream(lambda data: nvim.err_write(data))
+        # Setup the DirChanged listener


Hmm maybe we can use the host autocmd infrastructure here. (We need to add a flag to not start the host for the autocmd, but that would be useable in general)

Actually that would require a bit bigger refactor than I thought, so let's keep this as is for now (and maybe revisit at UpdateRemotePlugins refactor)

mhinz · 2018-01-19T13:02:28Z

@bfredl

What is happening here? https://travis-ci.org/neovim/python-client/jobs/330770820#L512

I get this locally as well. Sometimes.

Error detected while processing function provider#python3#Call:
line   18:
no request handler registered for "python_execute"

It starts happening at a8a98b8. This culprit seems to be this line:

nvim.command('au DirChanged * call rpcrequest(%d, "python_chdir", v:event)' % cid)

bfredl · 2018-01-19T13:09:43Z

you mean you don't see it on master?

bfredl · 2018-01-19T13:10:03Z

neovim/plugin/script_host.py

@@ -63,6 +63,12 @@ def setup(self, nvim):
        self.saved_stderr = sys.stderr
        sys.stdout = RedirectStream(lambda data: nvim.out_write(data))
        sys.stderr = RedirectStream(lambda data: nvim.err_write(data))
+        # Setup the DirChanged listener
+        cid = nvim.channel_id


unnecessary temporary

bfredl · 2018-01-19T13:11:01Z

neovim/plugin/script_host.py

+        # Setup the DirChanged listener
+        cid = nvim.channel_id
+        cmd = 'autocmd DirChanged *' \
+            + ' call rpcrequest(%d, "python_chdir", v:event)' \


also we prefer to use new-style .format()

I, too, prefer .format(). I simply followed what I've seen being used before. Will change it.

mhinz · 2018-01-19T13:13:15Z

Yup. I just do:

$ git checkout a8a98b8
$ nvim
:python3 import os

Commenting out the following line makes it work again:

nvim.command('au DirChanged * call rpcrequest(%d, "python_chdir", v:event)' % cid)

mhinz · 2018-01-19T13:25:55Z

Any nvim.command() in setup() will lead to that error.

bfredl · 2018-01-19T13:32:12Z

Hmm, maybe it shouldn't be done in __init__. I generally recommend against doing any RPC there, but I don't see the specific problem right here. Maybe the issue is that it is un-synchorized in general, using nvim.command(...,async=True) might fix it even if its not pretty either.

mhinz · 2018-01-19T13:34:34Z

Yup, nvim.command(..., async=True) fixes it for me.

bfredl · 2018-01-19T13:48:06Z

It is a bit racy, if a vim script does

py3 import blah # first call, so starts the host
cd mydir

the cd might come before the async registration. One can of course also invoke it in the async command to be sure in this case, but generally we want some mechanism for "run this vimL code right when the host starts".

Leandros · 2018-01-19T13:50:54Z

@mhinz Hmm. That is interesting. I'm using it already, and couldn't see any errors. Which nvim version are you using?

NVIM v0.2.2
Build type: Release
LuaJIT 2.1.0-beta3
Compilation: /usr/local/Homebrew/Library/Homebrew/shims/super/clang -Wconversion -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 -DNVIM_MSGPACK_HAS_FLOAT32 -DNVIM_UNIBI_HAS_VAR_FROM -DNDEBUG -DMIN_LOG_LEVEL=3 -Wall -Wextra -pedantic -Wno-unused-parameter -Wstrict-prototypes -std=gnu99 -Wimplicit-fallthrough -Wvla -fstack-protector-strong -fdiagnostics-color=auto -DINCLUDE_GENERATED_DECLARATIONS -I/tmp/neovim-20171118-53412-9ielcr/neovim-0.2.2/build/config -I/tmp/neovim-20171118-53412-9ielcr/neovim-0.2.2/src -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/include -I/usr/local/opt/gettext/include -I/usr/include -I/tmp/neovim-20171118-53412-9ielcr/neovim-0.2.2/build/src/nvim/auto -I/tmp/neovim-20171118-53412-9ielcr/neovim-0.2.2/build/include
Compiled by brew@Sierra-2.local

Features: +acl +iconv +jemalloc +tui
See ":help feature-compile"

   system vimrc file: "$VIM/sysinit.vim"
  fall-back for $VIM: "/usr/local/Cellar/neovim/0.2.2/share/nvim"

Run :checkhealth for more info

mhinz · 2018-01-19T14:36:32Z

@Leandros

Hm, you're right. I can only reproduce it with master and this repo is using nightlies during CI tests.

I can't reproduce it with the 0.2.2 release.

I'll bisect nvim later.

mhinz · 2018-01-20T13:32:33Z

@bfredl

This issue starts happening after neovim/neovim@1ebc96fe1.

bfredl · 2018-01-20T18:40:22Z

Yes, it changes processing of RPC job messages, but not more than to how they were before RPC jobs gained stderr support neovim/neovim#4723 (and to how RPC sockets always have worked), so they are less "forgiving" to potentially racy patterns.

Synchronous initialization should be initiated by one party only. In the case of plugin hosts, this is already nvim, by using one of poll, specs or in the case of script_host by calling one of the script methods directly.

bfredl · 2018-01-20T18:45:49Z

neovim/plugin/script_host.py

@@ -44,6 +44,14 @@ def __init__(self, nvim):
        exec('import sys', self.module.__dict__)
        self.legacy_vim = LegacyVim.from_nvim(nvim)
        sys.modules['vim'] = self.legacy_vim
+        # Since the 'DirChanged' autocmd is established asynchronously, import
+        # os manually already, to prevent a potential race condition
+        exec('import os', self.module.__dict__)


Why is this needed? The autocmd handler method is executed in the context of script_host.py which already has the import.

I tried to work-around the race you described, by importing os before, so that Vim can see the current working directory without creating a race.

But the race has nothing to do with import os. Quick fix would rather be to do
nvim.command('call rpcrequest({}, "python_chdir", {"cwd": getcwd()}', async=True) after registering the autocmd

The quickfix doesn't seem to work, at least for me, it results in a python error from my plugins.

Leandros · 2018-01-22T08:35:46Z

@bfredl What's your suggestion of implementing this? It's definitely required, we need to watch for the DirChange somehow. I've just looked at the ruby plugin, and found they're doing it quite similar.

bfredl · 2018-01-22T09:09:33Z

Lets use the current solution for now (with the fix I just posted), until we improve host autocmd registration.

justinmk · 2018-04-12T23:29:55Z

nvim.command('call rpcrequest({}, "python_chdir", {"cwd": getcwd()}', async=True)

seems to be a chicken-egg problem, instead I went with this:

os.chdir(nvim.eval('getcwd()', async=False))

Pushed an update to @Leandros branch, will merge after CI.

close neovim#295 close neovim/neovim#3636

This release adds support for Python 3.7 which now is in beta. `async` is a keyword in python3.7, and it should be replaced with `async_` when used as a keyword argument to API methods. `async` is still supported with python 2.7 and 3.6 for the moment, but considered deprecated. Also, integration with the in-process lua interpreter in Nvim is now supported. Changes since 0.2.4: * debcde0 clean up remote object implementation (#313) * e880fe7 Guidelines for local plugin development (#317) * 1ab98e8 Update the working directory on DirChanged for legacy plugins (#296) * d53415d Fix SyntaxError in py3.7: use `async_` instead of `async` (#274) * b65f62d Use `pytest` as `nosetests` are not longer maintained (#266) * d9aed96 Support using lua functions. `buf.update_higlights` as an internal example (#325)

blueyed · 2018-04-30T14:46:51Z

This appears to cause problems with initial use of :py3 for me: #337.

Leandros mentioned this pull request Jan 19, 2018

Files not opening anymore after changing working directory Yggdroot/LeaderF#101

Closed

mhinz reviewed Jan 19, 2018

View reviewed changes

bfredl reviewed Jan 19, 2018

View reviewed changes

bfredl reviewed Jan 20, 2018

View reviewed changes

justinmk force-pushed the develop branch from b802db9 to 5811fad Compare April 12, 2018 23:28

justinmk pushed a commit to Leandros/python-client that referenced this pull request Apr 12, 2018

set process-global CWD on DirChanged neovim#296

5811fad

justinmk force-pushed the develop branch from 5811fad to 01ec74b Compare April 12, 2018 23:33

justinmk pushed a commit to Leandros/python-client that referenced this pull request Apr 12, 2018

set process-global CWD on DirChanged neovim#296

01ec74b

set process-global CWD on DirChanged neovim#296

1ab98e8

close neovim#295 close neovim/neovim#3636

justinmk force-pushed the develop branch from 01ec74b to 1ab98e8 Compare April 12, 2018 23:53

justinmk merged commit 1ab98e8 into neovim:master Apr 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly change the working directory in python #296

Correctly change the working directory in python #296

Leandros commented Jan 19, 2018

justinmk commented Jan 19, 2018

Leandros commented Jan 19, 2018

mhinz Jan 19, 2018

Leandros Jan 19, 2018

Leandros commented Jan 19, 2018

bfredl Jan 19, 2018

bfredl Jan 19, 2018

mhinz commented Jan 19, 2018 •

edited

bfredl commented Jan 19, 2018

bfredl Jan 19, 2018

bfredl Jan 19, 2018

Leandros Jan 19, 2018

mhinz commented Jan 19, 2018

mhinz commented Jan 19, 2018

bfredl commented Jan 19, 2018

mhinz commented Jan 19, 2018 •

edited

bfredl commented Jan 19, 2018

Leandros commented Jan 19, 2018 •

edited

mhinz commented Jan 19, 2018

mhinz commented Jan 20, 2018

bfredl commented Jan 20, 2018

bfredl Jan 20, 2018

Leandros Jan 22, 2018

bfredl Jan 22, 2018

Leandros Jan 24, 2018

Leandros commented Jan 22, 2018

bfredl commented Jan 22, 2018

justinmk commented Apr 12, 2018

blueyed commented Apr 30, 2018

Correctly change the working directory in python #296

Correctly change the working directory in python #296

Conversation

Leandros commented Jan 19, 2018

justinmk commented Jan 19, 2018

Leandros commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leandros commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhinz commented Jan 19, 2018 • edited

bfredl commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhinz commented Jan 19, 2018

mhinz commented Jan 19, 2018

bfredl commented Jan 19, 2018

mhinz commented Jan 19, 2018 • edited

bfredl commented Jan 19, 2018

Leandros commented Jan 19, 2018 • edited

mhinz commented Jan 19, 2018

mhinz commented Jan 20, 2018

bfredl commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leandros commented Jan 22, 2018

bfredl commented Jan 22, 2018

justinmk commented Apr 12, 2018

blueyed commented Apr 30, 2018

mhinz commented Jan 19, 2018 •

edited

mhinz commented Jan 19, 2018 •

edited

Leandros commented Jan 19, 2018 •

edited