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

Should we delete the _distutils directory from our setuptools stubs ? #9520

Closed
Avasam opened this issue Jan 13, 2023 · 10 comments · Fixed by #9795
Closed

Should we delete the _distutils directory from our setuptools stubs ? #9520

Avasam opened this issue Jan 13, 2023 · 10 comments · Fixed by #9795
Labels
project: policy Organization of the typeshed project

Comments

@Avasam
Copy link
Collaborator

Avasam commented Jan 13, 2023

From @AlexWaygood :
#9460 (comment)

I sort of wonder if we shouldn't just delete the _distutils directory from our setuptools stubs. The frequency of the changes to that directory (and the leading underscore in the name) indicates to me that the maintainers see the whole directory as an implementation detail, and it just causes work for us to keep the directory up to date. I'm also not sure it's very useful keeping the directory around for type-checking purposes, since setuptools does some terrible importlib hack so that you can import the directory just by doing import distutils at runtime -- so there's little reason to ever import anything from setuptools._distutils directly.

(also distutils being deprecated)

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 13, 2023

The distutils situation on 3.10+ at the moment is a complete mess.

There's currently two versions of distutils: the dead, decaying version in the stdlib, and the version maintained by setuptools that lives in the setuptools/_distutils directory, which is undergoing a lot of code churn at the moment. Because of the importlib hack setuptools does, if you do import distutils on 3.10+, you're actually getting setuptools._distutils rather than the stdlib distutils. But there's more! For bootstrapping reasons, setuptools comes pre-installed in most builds of Python. That means that if you have a fresh Python install, without having downloaded anything from pip, you get the third-party version of distutils rather than the stdlib distutils whenever you do import distutils.

Needless to say, stubtest doesn't understand any of this, and is very confused by the whole thing. Most patch releases of 3.10 these days bump the bundled version of setuptools they come with. That then leads to our stdlib stubtest exploding with errors every time a 3.10 patch release comes along, because stubtest thinks it's the stdlib distutils that's changed, when in actual fact it's stuff in setuptools/_distutils that's changed.

Practically speaking, it's now pretty difficult to import the stdlib distutils on 3.10 (you get the setuptools version whether you like it or not!).

@AlexWaygood AlexWaygood added the project: policy Organization of the typeshed project label Jan 14, 2023
@srittau
Copy link
Collaborator

srittau commented Jan 16, 2023

If I remember correctly, some setuptools classes are also subclasses of distutils, which could it make difficult to remove it.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 10, 2023

If I understand correctly. Unless python/cpython#101039 is backported, typeshed will have to wait at least until Python 3.10 (the first version where it's deprecated) is the oldest python version supported by type-checkers to even consider removing stdlib/distutils. Or at least have stdlib/distutils reflect stubs/setuptools/setuptools/_distutils. Which would be around 2026 (end of 2025 + a buffer)

If it is backported, then typeshed can avoid installing setuptools in its stdlib tests and start validating stdlib/distutils against the standard lib version of it.


Here's an idea:
Keep stdlib/distutils and type it appropriately to the standard lib version (can't be tested within typeshed, but w/e, that's already the case).
Move stubs/setuptools/setuptools/_distutils to stubs/setuptools/distutils. This will have the effect of adding distutils-stubs to the sites-packages if installing types-setuptool.

Effectively giving distutils / setuptools users the power to use the appropriate distutils stubs and have correct typing when doing import distutils.

(not sure if importing from distutils is also considered deprecated if you're using the setuptools version? If it is, only validate stubs/setuptools/distutils up to python 3.9. Reducing maintenance cost)

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Feb 10, 2023

I'm confused; what is the problem we're looking to solve? The quote from Alex seems to point to an issue of improving the maintainability of types-setuptools; I'd be totally fine with us stubtest ignoring everything in setuptools._distutils and distutils

@hauntsaninja
Copy link
Collaborator

This is all mostly dead code. Let's only do things in response to user requests here.

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 10, 2023

I'm confused; what is the problem we're looking to solve? The quote from Alex seems to point to an issue of improving the maintainability of types-setuptools; I'd be totally fine with us stubtest ignoring everything in setuptools._distutils and distutils

I guess there's two things:

  1. Maintainability of distutils and setuptools/_distutils .
    1.1 For the current foreseeable future, there's no way of validating stdlib/distutils against the source in the standard lib (because of venv/ensurepip/setuptools shenanigans mentioned above). So it's pretty much a given that it can't be validated with stubtest, unless you decide to just go with the setuptools version no matter what.
    1.2 setuptools/_distutils is technically internal implementation details. The only use I see for it is for users to get appropriate typing by doing from setuptools import _distutils, which I believe they shouldn't do, and I believe there's a better way to tackle this (see point 2). So just remove the folder entirely?
  2. Type accuracy of distutils stubs.
    2.1 As mentioned previously, import distutils give different results depending on if you have setuptools installed or not. Users are currently stuck with whatever version typeshed decided to go with. The idea I had may solve that. (and reinforces point 1.2 of removing setuptools/_distutils, even if it's by moving it)
    2.2 Unless you feel like just removing distutils entirely. (I guess because it shouldn't be used and the stubs are likely incorrect anyway?). It just feels off since it's not marked as deprecated in 3.7, 3.8, 3.9, but 🤷

@Avasam
Copy link
Collaborator Author

Avasam commented Feb 10, 2023

This is all mostly dead code. Let's only do things in response to user requests here.

What I don't know (and am hoping for), is whether using distutil at all with setuptools installed is considered bad practice, even with older python versions. In which case, ignore point 1.2, 2.1, drop setuptools/_distutils and mark stdlib/distutils as not testable with stubtest and call it a day.

Edit: found my answer https://setuptools.pypa.io/en/latest/deprecated/distutils-legacy.html#prefer-setuptools

@FFY00
Copy link
Member

FFY00 commented Mar 4, 2023

Sorry for the drop by comment, this whole situation is quite involved, but if I understand correctly with #9795 you are now shipping the setuptools._distutils types for distutils, right? Is this a compromise, or the actual desired behavior? It's difficult for me to judge here because this involves both your test suite bug and the maintainability of the stubs.

The test suite bug is a relatively easy fix, so if this is a compromise, we can get you back to the desired behavior. If this is already the actual desired behavior, then it's all good anyway, pay no attention to this comment 😅

@hauntsaninja
Copy link
Collaborator

No, it looks like this got resolved the other way round... 1) distutils stdlib stubs are stubs for actual distutils, 2) the setuptools._distutils stubs have just been deleted.

While more truthful, this is likely to not match what most users see. But all of this code is dying and I didn't see any mypy primer hits either way, so not sure anything matters.

@FFY00
Copy link
Member

FFY00 commented Mar 4, 2023

Okay, that's what I am missing then! I am having trouble seeing how that fixes the issue, so that explains why I was so confused.

Anyway, if you wanna help fixing the tests issue, making sure the expected disututils module is imported, please let me know. Don't know how much, if anything, resolving that issue changes the outcome, though.
For future reference, if you have issues regarding import stuff, feel free to ping me, happy to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants