Fixes to stubtest's new check for missing stdlib modules #15960
Conversation
mypy/stubtest.py
Outdated
| for finder, module_group in modules_by_finder.items(): | ||
| if ( | ||
| "site-packages" not in Path(finder.path).parents | ||
| and {"json", "_json", "_queue"} & module_group |
There was a problem hiding this comment.
Why are these modules special?
There was a problem hiding this comment.
I added a comment in bf9fdad. If there was a sure-fire way of finding out where the pure-Python and extension stdlib modules are located on any Python build, we could filter according to the .path attribute of the finder, which would be more principled. There might be a sure-fire way of doing so using sysconfig, not sure.
There was a problem hiding this comment.
Maybe these?
>>> sysconfig.get_path("stdlib")
'/Users/jelle/.pyenv/versions/3.11.1/lib/python3.11'
>>> sysconfig.get_path("platstdlib")
'/Users/jelle/py/venvs/py311/lib/python3.11'
There was a problem hiding this comment.
Maybe these?
They don't do the right thing on Windows, unfortunately:
>>> import sysconfig, pkgutil
>>> sysconfig.get_path("stdlib")
'C:\\Users\\alexw\\AppData\\Local\\Programs\\Python\\Python311\\Lib'
>>> sysconfig.get_path("platstdlib")
'C:\\Users\\alexw\\AppData\\Local\\Programs\\Python\\Python311\\Lib'
>>> m = next(m for m in pkgutil.iter_modules() if m.name == "_queue")
>>> m.module_finder.path
'C:\\Users\\alexw\\AppData\\Local\\Programs\\Python\\Python311\\DLLs'There was a problem hiding this comment.
os.path.join(sysconfig.get_config_var("BINDIR"), sysconfig.get_config_var("platlibdir")) might be the incantation I'm looking for...
There was a problem hiding this comment.
os.path.join(sysconfig.get_config_var("BINDIR"), sysconfig.get_config_var("platlibdir"))might be the incantation I'm looking for...
no, that doesn't work on linux.
There's a StackOverflow answer here using distutils.sysconfig.get_python_lib, but it's nontrivial, and distutils is all deprecated anyway: https://stackoverflow.com/a/8992937/13990016. Not sure if it's worth it.
There was a problem hiding this comment.
Yeah, I concluded something similar when I wrote #15729 (review) . I think it's fine and I'm not aware of a better solution.
I think you still need sys.builtin_module_names the way that snippet does though
| @@ -1679,16 +1680,22 @@ def get_importable_stdlib_modules() -> set[str]: | |||
| all_stdlib_modules = sys.stdlib_module_names | |||
| else: | |||
| all_stdlib_modules = set(sys.builtin_module_names) | |||
There was a problem hiding this comment.
I think you still need
sys.builtin_module_namesthe way that snippet does though
@hauntsaninja we start off with the sys.builtin_module_names modules being in the set
There was a problem hiding this comment.
Oh oops, I missed that. Thanks!
There was a problem hiding this comment.
No worries — thanks for the review! :D
I did a trial run with my typeshed fork using GitHub Actions to see what the results of running stubtest on typeshed's stdlib stubs were with mypy's master branch. It's a good job that I did, as it revealed that there were a few flaws in the logic I introduced in #15729:
SystemExitwhen stubtest tries to import them in CI, leading stubtest to instantly exit without logging a message to the terminal.test.*submodules leads to unraisable exceptions being printed to the terminal at the end of the stubtest run, which is somewhat annoying.This PR fixes the bugs. This logic has now been tested locally using venv, locally using conda environments, on GitHub Actions, on py311, py38, Ubuntu, Windows and macOS.