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 most of setuptools._distutils #9795

Merged
merged 9 commits into from
Feb 26, 2023

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Feb 22, 2023

Just like regular implementation details, private or vendored modules, we only keep what's used in our stubs. Because it's easier to reference and find the source if the structure is the same.
I could have imported from distutils to remove setuptools._distutils completely. But that's not a good idea for two reasons imo:

  1. Eventually stdlib/distutils won't exist anymore and that'll cause issues.
  2. May as well use the version setuptools vendors, with the correct types, that we can test with stubtest.

Closes #9520
Also resolves overuse of Any in the leftover setuptools._distutils classes for #9550

I added an extra_description which I believe can be very valuable to users of types-setuptools who may not be aware of distutils's situation. Feel free to wordsmith.
Preview:
image

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 22, 2023

apprise is a types-babel issue. babel and mypyc are the same issue: They could be importing from either distutils or setuptools.
It used to be fine because types-setuptools classes used to (incorrectly) subclass distutils classes. But now it's correctly showing that the types have no direct relation.

mypyc is fixable, it's voluntarily importing setuptools for its monkey patching. I think it can just change the return type of get_extension https://github.com/python/mypy/blob/master/mypyc/build.py#L56

As for apprise, well, babel doesn't directly depend on setuptools, so we can't just update the type there. https://github.com/python-babel/babel/blob/master/babel/messages/frontend.py#L41
Unless you consider that having installed babel means you've had to have setuptools installed because of https://github.com/python-babel/babel/blob/master/setup.py#L4

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM (other than my one comment). Will leave open for a bit in case any other maintainers object, though.

Comment on lines 2 to 10
extra_description = """\
Having [setuptools](https://pypi.org/project/setuptools/) installed results in incorrect \
[distutils](https://docs.python.org/3/distutils/introduction.html) typing. \
`types-setuptools` makes no attempt at correcting this issue. As of \
`setuptools>=60.0.0`, you [should not be using `distutils` directly](\
https://setuptools.pypa.io/en/latest/deprecated/distutils-legacy.html). \
`distutils` is deprecated as of Python 3.10 and will be fully removed in Python 3.12. \
(see [PEP 632](https://peps.python.org/pep-0632/))
"""
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is necessary -- it sort of feels like it's setuptools's problem rather than ours. If we do want some kind of a note here, I'd prefer it if it were slightly more concise.

Copy link
Collaborator Author

@Avasam Avasam Feb 25, 2023

Choose a reason for hiding this comment

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

Does this look better to you? Same links, and all the same info can be found at those locations.

Having setuptools installed results in incorrect distutils typing. types-setuptools makes no attempt at correcting this issue. See Porting from Distutils and PEP 632 – Deprecate distutils module.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, the more I think about it, the more I lean towards not including this note at all. It feels like unnecessary finger-pointing at setuptools from us. The distutils situation is very frustrating for me, a typeshed maintainer, but as far as I know we haven't received any complaints from users of our stubs about incorrect distutils typing, so it sort of feels like this note is attempting to solve something that may not actually be much of an issue for users. If we do get complaints or queries from users, we can maybe rethink this, but for now I think we can probably do without this :)

Copy link
Collaborator Author

@Avasam Avasam Feb 25, 2023

Choose a reason for hiding this comment

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

I can see what you mean. I also expect the amount of users who'll actually land on the PyPI page to be taught about the issue of mixing setuptools and distutils together to be really small. (pyright ships stubs, and the average mypy user probably installs stubs from mypy's CLI suggestion).
I'll omit the message for now. Can be revised later if needed or if other maintainers feel differently about it.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 22, 2023

AlexWaygood added a commit that referenced this pull request Feb 23, 2023
Fixes #9799.

Stubtest started failing on our `setuptools` stubs because of a default value added to our stdlib `distutils` stubs. The reason for this is because our `setuptools` stubs erroneously claim here that the `setuptools._distutils.core.Distribution` class is the same as the stdlib `distutils.dist.Distribution` class:

https://github.com/python/typeshed/blob/06755e10ba0d39e7e4c18fcc2663d9da564a71ad/stubs/setuptools/setuptools/_distutils/core.pyi#L3

In actual fact, they're not, and they have different default values for this parameter. But reverting the addition of the default value is the simplest short-term fix for now.

(An alternative fix would be to just merge #9795.)
AlexWaygood added a commit that referenced this pull request Feb 23, 2023
Fixes #9799.

Stubtest started failing on our `setuptools` stubs because of a default value added to our stdlib `distutils` stubs. The reason for this is because our `setuptools` stubs erroneously claim here that the `setuptools._distutils.core.Distribution` class is the same as the stdlib `distutils.dist.Distribution` class:

https://github.com/python/typeshed/blob/06755e10ba0d39e7e4c18fcc2663d9da564a71ad/stubs/setuptools/setuptools/_distutils/core.pyi#L3

In actual fact, they're not, and they have different default values for this parameter. But reverting the addition of the default value is the simplest short-term fix for now.

(An alternative fix would be to just merge #9795.)
@AlexWaygood
Copy link
Member

mypy (https://github.com/python/mypy)
+ mypyc/build.py:64: error: Incompatible import of "Extension" (imported name has type "Type[setuptools.extension.Extension]", local name has type "Type[distutils.extension.Extension]")  [assignment]

I think you're correct that this one is easy to fix, but it is going to cause mypy's CI to suddenly and unexpectedly start failing once this PR is merged, since they don't pin types-setuptools in CI. Either you or I should probably go fix it as soon as this PR is merged :)

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Two more nits (sorry!)

stubs/setuptools/setuptools/_distutils/dist.pyi Outdated Show resolved Hide resolved
@@ -8,7 +10,8 @@ class upload_docs(upload):
user_options: ClassVar[list[tuple[str, str | None, str]]]
boolean_options: ClassVar[list[str]]
def has_sphinx(self): ...
sub_commands: Any
# The callable parameter is self: Self, but using Self still trips up mypy
sub_commands: ClassVar[list[tuple[str, Callable[[Self], bool] | None]]] # type: ignore[misc, assignment]
Copy link
Collaborator Author

@Avasam Avasam Feb 25, 2023

Choose a reason for hiding this comment

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

I didn't need to modify this file (leaving it as Incomplete or Any would be enough for this PR), but I wanted to show what I think may be a mypy issue with Self in Callable. I spotted. pyright looks correct to me.

image

Example with sub_commands removed and inferred from superclass:
image

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. I think using Self in ClassVars might be underspecified/I'm not sure exactly what the semantics should be really. But this looks fine for now imo.

@github-actions

This comment has been minimized.

@Avasam Avasam requested a review from AlexWaygood February 25, 2023 21:30
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

mypy (https://github.com/python/mypy)
+ mypyc/build.py:64: error: Incompatible import of "Extension" (imported name has type "Type[setuptools.extension.Extension]", local name has type "Type[distutils.extension.Extension]")  [assignment]

apprise (https://github.com/caronc/apprise)
+ setup.py:74: error: Argument "cmdclass" to "setup" has incompatible type "Dict[str, Type[babel.messages.frontend.Command]]"; expected "Mapping[str, Type[setuptools._distutils.cmd.Command]]"  [arg-type]

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you!

@AlexWaygood AlexWaygood merged commit fee5f1b into python:main Feb 26, 2023
JelleZijlstra pushed a commit to python/mypy that referenced this pull request Feb 26, 2023
A simple type-only change since mypyc explicitely uses setuptools'
monkeypatching. And `setup.py` imports from setuptools.

Tests should stay the same. This should fix a sudden failure of tests
once python/typeshed#9795 is merged.
AlexWaygood added a commit to AlexWaygood/stubdefaulter that referenced this pull request Feb 26, 2023
Following python/typeshed#9795, stubtest now passes for typeshed's `setuptools` stubs even if the correct default parameter is added to the stdlib stub for `distutils.dist.Distribution.announce`
JelleZijlstra pushed a commit to JelleZijlstra/stubdefaulter that referenced this pull request Feb 26, 2023
Following python/typeshed#9795, stubtest now passes for typeshed's `setuptools` stubs even if the correct default parameter is added to the stdlib stub for `distutils.dist.Distribution.announce`
AlexWaygood pushed a commit to AlexWaygood/mypy that referenced this pull request Mar 2, 2023
A simple type-only change since mypyc explicitely uses setuptools'
monkeypatching. And `setup.py` imports from setuptools.

Tests should stay the same. This should fix a sudden failure of tests
once python/typeshed#9795 is merged.
hauntsaninja added a commit to hauntsaninja/typeshed that referenced this pull request Jul 3, 2023
Linking python#10255

There are cases of these being used, including in mypy
This is reverts a small part of python#9795
@Avasam Avasam deleted the cleanup-_distutils branch October 20, 2023 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should we delete the _distutils directory from our setuptools stubs ?
2 participants