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

Remove dependency on pkg_resources from setuptools #1536

Merged
merged 22 commits into from
May 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ Release date: TBA

Closes #1512

* Remove dependency on ``pkg_resources`` from ``setuptools``.

Closes #1103

* Rename ``ModuleSpec`` -> ``module_type`` constructor parameter to match attribute
name and improve typing. Use ``type`` instead.

Expand Down
2 changes: 1 addition & 1 deletion astroid/interpreter/_import/spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def contribute_to_path(self, spec, processed):


class ExplicitNamespacePackageFinder(ImportlibFinder):
"""A finder for the explicit namespace packages, generated through pkg_resources."""
"""A finder for the explicit namespace packages."""

def find_module(self, modname, module_parts, processed, submodule_path):
if processed:
Expand Down
37 changes: 27 additions & 10 deletions astroid/interpreter/_import/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,32 @@
# For details: https://github.com/PyCQA/astroid/blob/main/LICENSE
# Copyright (c) https://github.com/PyCQA/astroid/blob/main/CONTRIBUTORS.txt

try:
import pkg_resources
except ImportError:
pkg_resources = None # type: ignore[assignment]
from importlib.util import _find_spec_from_path


def is_namespace(modname):
return (
pkg_resources is not None
and hasattr(pkg_resources, "_namespace_packages")
and modname in pkg_resources._namespace_packages
)
def is_namespace(modname: str) -> bool:
found_spec = None

# find_spec() attempts to import parent packages when given dotted paths.
# That's unacceptable here, so we fallback to _find_spec_from_path(), which does
# not, but requires instead that each single parent ('astroid', 'nodes', etc.)
# be specced from left to right.
working_modname = ""
last_parent = None
for component in modname.split("."):
if working_modname:
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
working_modname += "." + component
else:
# First component
working_modname = component
try:
found_spec = _find_spec_from_path(working_modname, last_parent)
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
except ValueError:
# executed .pth files may not have __spec__
return True
last_parent = working_modname

if found_spec is None:
return False

return found_spec.origin == "namespace"
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
1 change: 0 additions & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ packages = find:
install_requires =
lazy_object_proxy>=1.4.0
wrapt>=1.11,<2
setuptools>=20.0
typed-ast>=1.4.0,<2.0;implementation_name=="cpython" and python_version<"3.8"
typing-extensions>=3.10;python_version<"3.10"
python_requires = >=3.6.2
Expand Down
9 changes: 0 additions & 9 deletions tests/unittest_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
from contextlib import contextmanager
from typing import Iterator

import pkg_resources

import astroid
from astroid import manager, test_utils
from astroid.const import IS_JYTHON
Expand Down Expand Up @@ -128,7 +126,6 @@ def test_implicit_namespace_package(self) -> None:
def test_namespace_package_pth_support(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these tests still fail without certain code? I thought these lines were needed to make these packages namespace packages in a hacky way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if you just make is_namespace() unconditionally return False, these tests fail. My understanding is that the hacks were just for the reliance on the private variables of pkg_resources. (Part of the reason pkg_resources is discouraged now, I take it!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gotten around to testing it, but are we sure this still catches namespace packages defined in the pkg_resources way? I know I had made a test package when I initially worked on this and was then able to keep the same tests. See: https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#pkg-resources-style-namespace-packages

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestPackage.zip

The above zip contains a package as defined by that guideline. With the following diff:

diff --git a/astroid/interpreter/_import/spec.py b/astroid/interpreter/_import/spec.py
index 7a1de6cdb..cc963d722 100644
--- a/astroid/interpreter/_import/spec.py
+++ b/astroid/interpreter/_import/spec.py
@@ -328,6 +328,9 @@ def _find_spec_with_path(search_path, modname, module_parts, processed, submodul
         spec = finder.find_module(modname, module_parts, processed, submodule_path)
         if spec is None:
             continue
+        if spec.type == ModuleType.PY_NAMESPACE:
+            print(spec)
+            print(finder)
         return finder, spec
 
     raise ImportError(f"No module named {'.'.join(module_parts)}")

And the following command: pylint ../namespace_package/myotherpackage

main returns:

ModuleSpec(name='myotherpackage', type=<ModuleType.PY_NAMESPACE: 10>, location=None, origin=None, submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/myotherpackage'])
<astroid.interpreter._import.spec.PathSpecFinder object at 0x1062e12d0>
ModuleSpec(name='mynamespace', type=<ModuleType.PY_NAMESPACE: 10>, location='', origin='namespace', submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace'])
<astroid.interpreter._import.spec.ExplicitNamespacePackageFinder object at 0x1062e17b0>
ModuleSpec(name='mynamespace', type=<ModuleType.PY_NAMESPACE: 10>, location='', origin='namespace', submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace'])
<astroid.interpreter._import.spec.ExplicitNamespacePackageFinder object at 0x1062e18d0>
ModuleSpec(name='mynamespace', type=<ModuleType.PY_NAMESPACE: 10>, location='', origin='namespace', submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace'])
<astroid.interpreter._import.spec.ExplicitNamespacePackageFinder object at 0x1062e1000>

Whereas this PR returns:

ModuleSpec(name='myotherpackage', type=<ModuleType.PY_NAMESPACE: 10>, location=None, origin=None, submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/myotherpackage'])
<astroid.interpreter._import.spec.PathSpecFinder object at 0x1053054b0>

Without the if:

ModuleSpec(name='myotherpackage', type=<ModuleType.PY_NAMESPACE: 10>, location=None, origin=None, submodule_search_locations=['/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/myotherpackage'])
<astroid.interpreter._import.spec.PathSpecFinder object at 0x10df415a0>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df41570>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df412a0>
ModuleSpec(name='subpackage_a', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace/subpackage_a', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df40340>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df41c60>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df41e40>
ModuleSpec(name='subpackage_a', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace/subpackage_a', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df40340>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df417b0>
ModuleSpec(name='subpackage_a', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace/subpackage_a', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df41e40>
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='/Users/daniel/DocumentenLaptop/Programming/Github/namespace_package/mynamespace', origin=None, submodule_search_locations=None)
<astroid.interpreter._import.spec.ImportlibFinder object at 0x10df41e70>

mynamespace is thus interpreted as a different type of package. I don't know if this is actually a bug or if we are actually fixing a bug here, but I would need to do some more investigation before signing off on this. Perhaps you already know why this is different and whether this is a good thing from your own investigations?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this. I see the same output on main vs. the PR. Would you mind checking one more time? I also tried varying my cwd and invoking pylint differently.

I see PKG_DIRECTORY: 3 for mynamespace and PY_NAMESPACE: 10 for myotherpackage on both:

% python3 -m pylint mynamespace --disable=all 
ModuleSpec(name='mynamespace', type=<ModuleType.PKG_DIRECTORY: 3>, location='./mynamespace', origin=None, submodule_search_locations=None)

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 0.00/10, +10.00)

% python3 -m pylint myotherpackage --disable=all
ModuleSpec(name='myotherpackage', type=<ModuleType.PY_NAMESPACE: 10>, location=None, origin=None, submodule_search_locations=['/Users/myuser/TestPackage/./myotherpackage'])

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 0.00/10, +10.00)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you did say cd /tmp/TestPackage. Weird.

Is misread your question and thought you asked for my config. Well, there you have it 😄

I believe cd into the folder is required for this "test" to work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH. This is the part I wasn't expecting:

python -m pip install -e .

So the test case has to do with installing the package under site-packages. I wasn't expecting that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I guess that is required for importlib to figure out that mynamespace is indeed a namespace package (although I don't know the specifics).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible it's the other way around. Installing it under site-packages with pip is enough to get astroid's ImportlibFinder to find it first rather than ExplicitNamespacePackageFinder. And in that case, if installed with pip, it is a regular package, not a namespace package. So we might have fixed something here if this wasn't happening before.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hm, that might actually be true. I just assumed changed behaviour was a regression.

I am not completely sure though. So I'll need to play around with this a little bit more to be sure. Should find time for this this week!

pth = "foogle_fax-0.12.5-py2.7-nspkg.pth"
site.addpackage(resources.RESOURCE_PATH, pth, [])
pkg_resources._namespace_packages["foogle"] = []

jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
try:
module = self.manager.ast_from_module_name("foogle.fax")
Expand All @@ -138,18 +135,14 @@ def test_namespace_package_pth_support(self) -> None:
with self.assertRaises(AstroidImportError):
self.manager.ast_from_module_name("foogle.moogle")
finally:
del pkg_resources._namespace_packages["foogle"]
sys.modules.pop("foogle")

def test_nested_namespace_import(self) -> None:
pth = "foogle_fax-0.12.5-py2.7-nspkg.pth"
site.addpackage(resources.RESOURCE_PATH, pth, [])
pkg_resources._namespace_packages["foogle"] = ["foogle.crank"]
pkg_resources._namespace_packages["foogle.crank"] = []
try:
self.manager.ast_from_module_name("foogle.crank")
finally:
del pkg_resources._namespace_packages["foogle"]
sys.modules.pop("foogle")

def test_namespace_and_file_mismatch(self) -> None:
Expand All @@ -158,12 +151,10 @@ def test_namespace_and_file_mismatch(self) -> None:
self.assertEqual(ast.name, "unittest")
pth = "foogle_fax-0.12.5-py2.7-nspkg.pth"
site.addpackage(resources.RESOURCE_PATH, pth, [])
pkg_resources._namespace_packages["foogle"] = []
try:
with self.assertRaises(AstroidImportError):
self.manager.ast_from_module_name("unittest.foogle.fax")
finally:
del pkg_resources._namespace_packages["foogle"]
sys.modules.pop("foogle")

def _test_ast_from_zip(self, archive: str) -> None:
Expand Down