-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add distutils
as a top-level package included with types-setuptools
#10948
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
Conversation
And, I see that the test cases I added fail with pyright... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I got stubtest working, with a few hacks. If we're happy with the hacks, I can ask the pyright maintainers to look at why the test cases I added might be failing. Or we could just delete the test cases for now, and try to figure out later why it isn't working for pyright. I don't think this PR would cause any regressions for pyright users; it just wouldn't offer the same benefits as it's offering for mypy users |
This comment has been minimized.
This comment has been minimized.
setuptools._distutils.versionpredicate | ||
|
||
# Reexported from setuptools._distutils; problems should be fixed there | ||
distutils\..+ |
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.
Good call. Should make stubtest errors clearer and contributors will be less tempted to try to apply a fix in stdlib/distutils (I know I have in the past)
I would prefer if we copy the distutils stubs into setuptools. From what I understand, they are different from stdlib's distutils at this point anyway, and may evolve further. I'd even argue that copied stubs might be easier to maintain: We probably won't need to update stdlib's distutils much, but might need to update setuptools's from time to time. |
We've already done that. We copied them into At runtime, |
Does it still use an import hack? At least the freshly installed version of setuptools (68.2.2) vendors distutils and does seem to use it:
|
|
But System Python:
Virtualenv with locally updated setuptools:
|
Not
And yes, it gets you the same thing as |
@srittau, this is the behaviour I'm trying to reflect in this PR: (fresh-venv) C:\Users\alexw\coding>python -VV
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct 2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)]
(fresh-venv) C:\Users\alexw\coding>python -m pip install --upgrade pip
Requirement already satisfied: pip in c:\users\alexw\coding\fresh-venv\lib\site-packages (23.2.1)
Collecting pip
Obtaining dependency information for pip from https://files.pythonhosted.org/packages/47/6a/453160888fab7c6a432a6e25f8afe6256d0d9f2cbd25971021da6491d899/pip-23.3.1-py3-none-any.whl.metadata
Using cached pip-23.3.1-py3-none-any.whl.metadata (3.5 kB)
Using cached pip-23.3.1-py3-none-any.whl (2.1 MB)
Installing collected packages: pip
Attempting uninstall: pip
Found existing installation: pip 23.2.1
Uninstalling pip-23.2.1:
Successfully uninstalled pip-23.2.1
Successfully installed pip-23.3.1
(fresh-venv) C:\Users\alexw\coding>python -c "import setuptools"
Traceback (most recent call last):
File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'setuptools'
(fresh-venv) C:\Users\alexw\coding>python -c "import distutils"
Traceback (most recent call last):
File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'distutils'
(fresh-venv) C:\Users\alexw\coding>python -m pip install setuptools
Collecting setuptools
Downloading setuptools-68.2.2-py3-none-any.whl.metadata (6.3 kB)
Downloading setuptools-68.2.2-py3-none-any.whl (807 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 807.9/807.9 kB 4.3 MB/s eta 0:00:00
Installing collected packages: setuptools
Successfully installed setuptools-68.2.2
(fresh-venv) C:\Users\alexw\coding>python -c "import setuptools, distutils"
(fresh-venv) C:\Users\alexw\coding>python
Python 3.12.0 (tags/v3.12.0:0fb18b0, Oct 2 2023, 13:03:39) [MSC v.1935 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import distutils
>>> distutils
<module 'distutils' (C:\Users\alexw\coding\fresh-venv\Lib\site-packages\setuptools\_distutils\__init__.py)> |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In pywin32 I just hit I think this PR would resolve that issue as it would add |
@erictraut Is there some special-casing in pyright that prevents detecting a would-be |
@Avasam, no, pyright has no special-case knowledge of |
@erictraut, the VERSIONS file only gives type checkers information regarding the stdlib stubs (this is why the file is in our This situation arises because the |
@AlexWaygood, that's right. On python 3.11 and earlier, pyright is resolving the import to the stdlib stub. On python 3.12 and newer, the VERSIONS file indicates that it should no longer use the stdlib stub for resolution. Since there is no other I'm not sure what you mean by "provided by the third-party |
@erictraut, I mean the behaviour I described in this comment here: #10948 (comment). |
Setuptools adds their own vendored distsutils to the path. Of course this means it can't be analyzed statically and pyright users have to turn off |
This comment has been minimized.
This comment has been minimized.
Huh, I see that this seems to be fixed on the latest version of pyright; all CI is now passing on this PR. I wonder what changed, and when. Sorry for the noise, @erictraut -- thanks for your time, as ever! |
I think the only thing still blocking this PR is the question of whether we're happy with the hacks I'm introducing to |
@@ -0,0 +1,5 @@ | |||
import distutils.command.sdist | |||
import distutils.config | |||
|
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.
Maybe we should also try to use something exclusive to setuptool's version?
For example (the first one I found):
from distutils.util import split_version | |
s = split_version("") |
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.
Whelp, it showed that checkers (at least within the context of test_cases) still prefer the stdlib if it's found. (this now only passes on 3.12).
Since those weren't in stdlib anyway, I can live with not supporting them rather than adding more hacky workarounds to the test suite. But that's up to you.
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.
That fails mypy/pyright on Python <3.12, because both of them choose the stdlib distutils
stubs over the third-party distutils
stubs. That's the correct behaviour for any normal case where two packages of the same name are simultaneously provided by the stdlib and a third-party distribution; setuptools
only does its "override the stdlib distutils
if we're installed" magic through some importlib hacks, and unfortunately I think it's unreasonable to expect type checkers to add support for that
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.
You could write this as if sys.version_info >= (3, 12): split_version("")
. No opinion on whether it's needed though.
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.
You could write this as
if sys.version_info >= (3, 12): split_version("")
.
Eh, I feel like that could be more confusing than clarifying, since it is actually available on <py312 if you have setuptools
installed, there's just no way of reasonably getting type checkers to understand that
Co-authored-by: Avasam <[email protected]>
This comment has been minimized.
This comment has been minimized.
This reverts commit 5663525.
This comment has been minimized.
This comment has been minimized.
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 special-casing in tests/stubtest_third_party.py
isn't too wild, I can't think of a better solution, and it should resolve actual issues I've encountered in the wild, so this has my approval. (let's get at least another maintainer's approval though)
@@ -0,0 +1,5 @@ | |||
import distutils.command.sdist | |||
import distutils.config | |||
|
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.
You could write this as if sys.version_info >= (3, 12): split_version("")
. No opinion on whether it's needed though.
Huh, seems like the stubtest issue has also been magically fixed by one of our mypy upgrades since I originally filed this PR in October? Or maybe the fact that we now run third-party stubtest on py311 rather than py310? No idea. Anyway, looks like we don't even need the stubtest hacks anymore! |
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
This breaksstubtest_third_party.py
when you run it on Python <3.12 (as we do in CI), unfortunately, as it gets very confused at there being both astdlib/distutils
package and astubs/setuptools/distutils
package. Not sure how to fix that...Closes #10255