neovim / pynvim

Python client and plugin host for Nvim
http://pynvim.readthedocs.io/en/latest/
Apache License 2.0
1.47k stars 118 forks source link

fix: fix broken dynamic import of rplugin modules #534

Closed wookayin closed 9 months ago

wookayin commented 9 months ago

The removal of imp package (#461) in order to supprot Python 3.12 had a bug where rplugins can't be loaded and the ImportError exception was silenced, making the remote provider throwing lots of errors. This commit fixes broken dynamic import of python modules from the registered rplugins.

We add tests for Host._load, with loading rplugins consisting of:

(1) single-file module (e.g., rplugins/simple_plugin.py) (2) package (e.g., rplugins/mymodule/__init__.py)

Note: As per https://docs.python.org/3.12/whatsnew/3.12.html#imp, I also tried something like:

def _handle_import(path, name):
    # First try to import module
    spec = importlib.machinery.PathFinder.find_spec(name, [path])

    # or single-file package
    if spec is None:
        filename = os.path.join(path, name + '.py')
        loader = importlib.machinery.SourceFileLoader(name, filename)
        spec = importlib.util.spec_from_file_location(
            name, location=filename, loader=loader)

    if spec is not None:
        mod = importlib.util.module_from_spec(spec)
        spec.loader.exec_module(mod)
        return mod
    raise ImportError(name=name, path=path)

but this fails to resolve relative import within the package to unlike the previous implementation using imp (see the unit test fixtures). Possible more addition of loader would be needed. If you think this is the right way, please let me know so we can have additional work. Actually the remote host python should be able to import packages, so I don't see any reason why we couldn't use the approach of manipulating sys.path and import the module.

wookayin commented 9 months ago

/cc @justinmk @farisachugthai

BTW CI fails on 3.12 so I didn't include the commit https://github.com/wookayin/pynvim/compare/fix-import...wookayin:pynvim:ci-py312 in this PR. No idea why it fails to find the python3 host when python 3.12 is installed in the runner (py3.11 works fine). Possibly related to #528.

Also fails on windows but this PR is not accountable.

justinmk commented 9 months ago

Huge thanks, this is tricky, especially the tests.

Also fails on windows but this PR is not accountable.

Yep known issue: https://github.com/neovim/pynvim/pull/526#issuecomment-1636349839 (we had no CI at all for awhile before that 😅 ).

BTW CI fails on 3.12 so I didn't include the commit wookayin/pynvim@fix-import...wookayin:pynvim:ci-py312 in this PR. No idea why it fails to find the python3 host when python 3.12 is installed in the runner (py3.11 works fine). Possibly related to #528.

Yes that's the other remaining mystery.