-
-
Notifications
You must be signed in to change notification settings - Fork 278
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 distutils tests following setuptools 60.0.0 release #1316
Conversation
With v60.0.0 setuptools started monkeypatching the distuils module to use their own vendored version. Longterm any uses of distuils should be replaced or removed. pypa/setuptools#2896
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're trying to test find_spec
in this test, distutils was used because it was supposed to be always present i guess. We might as well use another dependency we'll keep, like wrapt
, lazy_object_proxy
(or setuptools
, but it might become problematic when we adopt pyproject.toml and something else than setuptools or if we dump setuptools.).
I think changing the package used for this test makes sense! |
wrapt is a dependency with more probability to stay
tests/unittest_modutils.py
Outdated
def test_find_distutils_submodules_in_virtualenv(self) -> None: | ||
found_spec = spec.find_spec(["distutils", "version"]) | ||
self.assertEqual(found_spec.location, distutils.version.__file__) | ||
def test_find_wrapt_submodules_in_virtualenv(self) -> None: | ||
found_spec = spec.find_spec(["wrapt", "decorators"]) | ||
self.assertEqual(found_spec.location, wrapt.decorators.__file__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was specifically designed to test the location of distutils
. So this change wouldn't fix it
I retract my earlier statement, see https://github.com/PyCQA/astroid/pull/672/files. We actually want to test Edit: Look likes @cdce8p was a little bit quicker than me 😄 |
Seeing the issue in pylint-dev/pylint#73 the result could be false positive for distutil in pylint ? |
This reverts commit 8b9b7f0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good temporary solution even if we regress on pylint #73 as distutil is old and deprecated.
I think I might have found a fix. I'll create a new PR. |
See #1321. I think we can close this in favour of that one, test seem to be passing there 😄 |
Amazing fix @DanielNoord ! |
With v60.0.0 setuptools started monkeypatching the distuils module
to use their own vendored version. Longterm any uses of distuils
should be replaced or removed.
pypa/setuptools#2896
https://setuptools.pypa.io/en/latest/history.html#v60-0-0
--
This change isn't ideal as it might hide an error, but for now it will unblock the CI pipeline.
Furthermore, at least for now, the setuptools copy should be identically with the stdlib one.
Closes #1313
References:
distutils
import #1282setuptools
anddistutil
#1103