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

fix: fix broken dynamic import of rplugin modules #534

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

wookayin
Copy link
Member

@wookayin wookayin commented Sep 22, 2023

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.

The removal of `imp` package (neovim#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`)
@wookayin
Copy link
Member Author

wookayin commented Sep 22, 2023

/cc @justinmk @farisachugthai

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.

Also fails on windows but this PR is not accountable.

@justinmk
Copy link
Member

justinmk commented Sep 22, 2023

Huge thanks, this is tricky, especially the tests.

Also fails on windows but this PR is not accountable.

Yep known issue: #526 (comment) (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.

@justinmk justinmk merged commit f244597 into neovim:master Sep 22, 2023
5 of 7 checks passed
@wookayin wookayin deleted the fix-import branch September 23, 2023 02:12
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.

2 participants