Remove distutils path patching
#1321
Conversation
|
@cdce8p once we merge this we can release astroid 2.9.1, would you mind having a quick look ? :) |
There was a problem hiding this comment.
I tried to reproduce the issue mentioned in pylint-dev/pylint#73 and the example from pylint-dev/pylint#2955 both with v59 and v60 of setuptools, but wasn't able too. With and without any changes.
It seems to have been fixed in venv in the meantime. We could consider adding a regression test in pylint though. This should be enough. If it doesn't raise an ImportError, everything is good.
import distutils.utilExample from pylint-dev/pylint#2955
astroid/interpreter/_import/spec.py
Outdated
| elif spec.name == "distutils" and _distutils: | ||
| # distutils is patched in setuptools >= 60.0.0 to _distutils | ||
| path = list(_distutils.__path__) |
There was a problem hiding this comment.
Tbh I wonder if we should just always use distutils.__path__. We do import it earlier, so it's most likely the one we actually want. From what I can tell, the first condition is equal to the else case anyway.
elif spec.name == "distutils":
path = list(distutils.__path__)Defaulting to the _distutils path from setuptools alone can however be dangerous. It was added in v48.0.0 but only became the default in v60.0.0. So just in theory, if the first condition wouldn't hit, it would pick the wrong path. In that case you probably should add an additional condition to make sure the setuptools distutils is actual the distutils we are using. With that we likely end up with the simplified case from above again.
elif spec.name == "distutils" and _distutils and _distutils.__path__ == distutils.__path__:
path = list(distutils.__path__)There was a problem hiding this comment.
Additionally, just doing elif spec.name == "distutils" would remove a setuptools dependency. -> #1320
Maybe we should just test it in production and see if we get any new bug reports 🤔
There was a problem hiding this comment.
I'm not completely following you here. Especially with all the latest hot fixes and releases I'm not sure what actually needs to be fixed.
For pylint everything seems fine. We might be incompatible with setuptools~=60.0 but due to all the recent releases it would be quite a big effort to find the specific release which fixed the bug that made the tbump upgrade fail. I feel like we can let that be, right?
For astroid this test is still failing for setuptools==60.2.0. I agree that the first if seems to do the same as the else. That can likely be removed, leaving only a if for _distutils to be necessary. Do you have any idea how to do that if without depending on setuptools but being compatible with both v47 and v60?
Please correct me where I'm wrong. This has been a difficult issue to keep track off and its getting late 😄
There was a problem hiding this comment.
This has been a difficult issue to keep track off and its getting late 😄
Same 😴 I can try to summarize what I think is going on here.
What do we know?
Starting with v60 setuptools patches (or better "does some sketchy things") to replace the stdlib version of distutils with their own copy. This is vendored from pypa/distutils and "should" be functionally identical with the stdlib version. If at all, it might include some more bugfixes that won't get included into stdlib.
Why?
distuils has been deprecated and is slated for removal in 3.12. Everyone who is still using it should either switch to the setuptools version (or another similar one) or even better replace those function calls altogether. Most, but not all, functions already have equivalents in the stdlib. https://www.python.org/dev/peps/pep-0632/#migration-advice
Longterm, the maintainers of setuptools also plan to discontinue distutils, but for the meantime it will continue to work.
What is actually failing?
As best as I can tell, only the astroid test case. Since the included distutils copy is functional identical to the stdlib it doesn't really matter too much that the spec path points towards the stdlib while the module is coming from setuptools._distutils.
What failed previously?
I only have an educated guess here. There seems to be a habit among certain packages to ship with their own version of distutils. In particular a (presumably old) version of virtualenv seem to have done so. It seems astroid had inferred the virtualenv version as spec.location instead of the stdlib one. That caused ImportErrors as the virtualenv version wasn't a complete copy.
Is that still an issue?
I don't know 🤷🏻♂️
What should the goal here be for astroid?
Pylint shouldn't report a false-positive ImportError for things like distutils.util.
What path for distutils should astroid return?
I'm uncertain about this one. If the goal is to prevent the false-positive and enable inference, any "good" copy of distutils would probably work. It doesn't really matter too much if it's the one from the stdlib or setuptools, since I don't think the main interface is going to change.
What do we do now?
Some options
- Disable the test. As explained in the section before it doesn't matter to much that it's inferred as the stdlib version although technically it's using the setuptools one.
- Patch the test. Instead of comparing it with the actual
distutils.__path__, we could use a good python file to find the stdlib location and built the expected stdlib distutils path from there.
import os
[f"{os.__file__.rsplit('/', 1)[0]}/distutils"]- Use the actual setuptools path if it's imported from setuptools. Basically the proposed fix. We try to import distutils twice, once from
distutilsand another time fromsetuptools._distutils. If both match, we choose to use the setuptools one for the path. Although probably the "most correct one", it requires us to do an expensive import twice. - YOLO it. We just remove all custom handling for
distutilsand see if we get any bug reports. I suspect / hope not but if we do, we at least would know what needs fixing.
Is the first conditional for distutils still needed?
🤷🏻♂️
My preference?
I believe all of the 4 options above should work. If we don't want to risk it all with (4), option (2) might be an idea. We can always come back to it in case we start getting bug reports eventually.
Did I miss something?
Likely 😄 At this point I don't even know if I've fully understood what's going on. It's also much too late to be writing such long posts 😴
I didn't check but could the crash seen here be of any help? pylint-dev/pylint#5572 |
Not sure. Do you know if the issue still occurs with the latest setuptools version? There have been a lot of small (bugfix?) release lately. I would guess after they made their version the default, they noticed some more issues and fixed them in subsequent patches. We don't pin There is also pylint-dev/pylint#5600 which does look fine, aside from the fact that they dropped support for Python 3.6 which causes some tests to fail. |
Following change in the way distutil is stored in setuptools See pylint-dev/astroid#1321
Or does the first import fail very fast because there is nothing to import, and the second one execute normally ? If not, this is another thing we could explore in #1320, it's often that we guard import to be able to handle multiple version of the same library.
At this point, it seems like an acceptable solution seeing distutils is deprecated and also seeing how often we fix bug in pylint but just removing custom handling and simplifying the code (Daniel will know what I mean by that 😄). I added the regression test in pylint (pylint-dev/pylint#5620) and I'll do some tests to see if things are green without specific handling with setuptools 47 or 60. (4 tests cases not the whole 47 to 60 versions one by one with each version of the code). It should be noted also that the error in 73 was a fatal (pylint itself can't import distutils), not a false positive |
|
@cdce8p Thanks for this good sum-up. It's quite in-depth for that time of day 😄 I think we can try and go ahead with option 4: we know what to revert to when things do eventually go wrong and the three of us (and others) are quite active lately so we should be able to push out a hotfix quickly if things really go south. (I won't respond tonight though probably 😄 🎆 ). Saving the import and deprecation warnings are nice added benefits which we would have to solve at some point anyway. |
|
@cdce8p Is the latest commit what you had in mind? Please make a suggestion so I can get you as co-author on this commit, you did most of the work 😄 |
astroid/interpreter/_import/spec.py
Outdated
| @@ -162,11 +161,6 @@ def contribute_to_path(self, spec, processed): | |||
| if os.path.isdir(os.path.join(p, *processed)) | |||
| ] | |||
| # We already import distutils elsewhere in astroid, | |||
There was a problem hiding this comment.
For example, to remove this comment 😅
There was a problem hiding this comment.
You can add Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com> when cleaning the squash message up :)
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@cdce8p See latest commits 😄 |
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
cdce8p
changed the title
distutils path patching for setuptools >= 60.0.0distutils path patching
Following change in the way distutil is stored in setuptools See pylint-dev/astroid#1321
Following change in the way distutil is stored in setuptools See pylint-dev/astroid#1321
Following change in the way distutil is stored in setuptools See pylint-dev/astroid#1321
|
For future reference: |
This reverts commit 51f552c.
Steps
Description
This passed locally. I wonder if this works here as well 😄
Type of Changes
Related Issue
For future reference, see the original commit that added path patching for
distutilshere: 6ca01e0Closes #1313