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 18 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 @@ -15,6 +15,10 @@ Release date: TBA

Closes #1512

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

Closes #1103

* Allowed ``AstroidManager.clear_cache`` to reload necessary brain plugins.

* Rename ``ModuleSpec`` -> ``module_type`` constructor parameter to match attribute
Expand Down
18 changes: 2 additions & 16 deletions astroid/interpreter/_import/spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ def contribute_to_path(self, spec, processed):
# Builtin.
return None

if _is_setuptools_namespace(spec.location):
if util._is_setuptools_namespace(Path(spec.location)):
# extend_path is called, search sys.path for module/packages
# of this name see pkgutil.extend_path documentation
path = [
Expand Down Expand Up @@ -179,7 +179,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 Expand Up @@ -256,20 +256,6 @@ def contribute_to_path(self, spec, processed):
)


def _is_setuptools_namespace(location):
try:
with open(os.path.join(location, "__init__.py"), "rb") as stream:
data = stream.read(4096)
except OSError:
return None
else:
extend_path = b"pkgutil" in data and b"extend_path" in data
declare_namespace = (
b"pkg_resources" in data and b"declare_namespace(__name__)" in data
)
return extend_path or declare_namespace


@lru_cache()
def _cached_set_diff(left, right):
result = set(left)
Expand Down
74 changes: 62 additions & 12 deletions astroid/interpreter/_import/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,65 @@
# 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]


def is_namespace(modname):
return (
pkg_resources is not None
and hasattr(pkg_resources, "_namespace_packages")
and modname in pkg_resources._namespace_packages
)
from __future__ import annotations

import pathlib
from functools import lru_cache
from importlib.machinery import BuiltinImporter
from importlib.util import _find_spec_from_path


def _is_setuptools_namespace(location: pathlib.Path) -> bool:
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
try:
with open(location / "__init__.py", "rb") as stream:
data = stream.read(4096)
except OSError:
return False
else:
extend_path = b"pkgutil" in data and b"extend_path" in data
declare_namespace = (
b"pkg_resources" in data and b"declare_namespace(__name__)" in data
)
return extend_path or declare_namespace


@lru_cache(maxsize=4096)
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.
processed_components = []
last_parent = None
for component in modname.split("."):
processed_components.append(component)
working_modname = ".".join(processed_components)
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

if found_spec.loader is BuiltinImporter:
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
# TODO(Py39): remove, since found_spec.origin will be "built-in"
return False

if found_spec.origin is None:
return True

if found_spec.submodule_search_locations is not None:
for search_location in found_spec.submodule_search_locations:
if any(
_is_setuptools_namespace(directory)
for directory in pathlib.Path(search_location).iterdir()
if directory.is_dir()
):
return True
Copy link
Member Author

@jacobtylerwalls jacobtylerwalls May 15, 2022

Choose a reason for hiding this comment

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

Aaaand with better distinguishing between builtins and namespace packages, we ... don't even need this anymore? Highlighting in a comment in case we pop the hood on this again.


return False
3 changes: 2 additions & 1 deletion astroid/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

from astroid.const import BRAIN_MODULES_DIRECTORY
from astroid.exceptions import AstroidBuildingError, AstroidImportError
from astroid.interpreter._import import spec
from astroid.interpreter._import import spec, util
from astroid.modutils import (
NoSourceFile,
_cache_normalize_path_,
Expand Down Expand Up @@ -382,6 +382,7 @@ def clear_cache(self) -> None:
for lru_cache in (
LookupMixIn.lookup,
_cache_normalize_path_,
util.is_namespace,
ObjectModel.attributes,
):
lru_cache.cache_clear()
Expand Down
1 change: 0 additions & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,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.7.2
Expand Down
22 changes: 13 additions & 9 deletions tests/unittest_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@
from collections.abc import Iterator
from contextlib import contextmanager

import pkg_resources

import astroid
from astroid import manager, test_utils
from astroid.const import IS_JYTHON
from astroid.exceptions import AstroidBuildingError, AstroidImportError
from astroid.interpreter._import import util
from astroid.modutils import is_standard_module
from astroid.nodes import Const
from astroid.nodes.scoped_nodes import ClassDef
Expand Down Expand Up @@ -111,6 +110,16 @@ def test_ast_from_namespace_pkgutil(self) -> None:
def test_ast_from_namespace_pkg_resources(self) -> None:
self._test_ast_from_old_namespace_package_protocol("pkg_resources")

def test_identify_old_namespace_package_protocol(self) -> None:
# Like the above cases, this package follows the old namespace package protocol
# astroid currently assumes such packages are in sys.modules, so import it
# pylint: disable-next=import-outside-toplevel
import tests.testdata.python3.data.path_pkg_resources_1.package.foo as _ # noqa

self.assertTrue(
util.is_namespace("tests.testdata.python3.data.path_pkg_resources_1")
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
)
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved

def test_implicit_namespace_package(self) -> None:
data_dir = os.path.dirname(resources.find("data/namespace_pep_420"))
contribute = os.path.join(data_dir, "contribute_to_namespace")
Expand All @@ -131,7 +140,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 @@ -141,18 +149,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 @@ -161,12 +165,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 Expand Up @@ -323,6 +325,7 @@ def test_clear_cache_clears_other_lru_caches(self) -> None:
lrus = (
astroid.nodes.node_classes.LookupMixIn.lookup,
astroid.modutils._cache_normalize_path_,
util.is_namespace,
astroid.interpreter.objectmodel.ObjectModel.attributes,
)

Expand All @@ -332,6 +335,7 @@ def test_clear_cache_clears_other_lru_caches(self) -> None:
# Generate some hits and misses
ClassDef().lookup("garbage")
is_standard_module("unittest", std_path=["garbage_path"])
util.is_namespace("unittest")
astroid.interpreter.objectmodel.ObjectModel().attributes()

# Did the hits or misses actually happen?
Expand Down