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 _vendor directory #288

Merged
merged 3 commits into from
Sep 2, 2024
Merged

Remove _vendor directory #288

merged 3 commits into from
Sep 2, 2024

Conversation

abravalheri
Copy link
Contributor

@abravalheri abravalheri commented Aug 27, 2024

Setuptools recently changed its vendoring method to address the criticism regarding the proliferation of _vendor directories and multiple copies of the same files being distributed in the same wheel/sdist: pypa/setuptools#4455.

That change however do not address the existence of setuptools/_distutils/_vendor and therefore do not close the original issue 100%. I think there is not much point in distutils maintaining its own vendoring system, so this PR proposes simply removing it, and relying on the dependencies that come installed with setuptools.

In pypa/setuptools#4594, we also got a report regarding external tools expecting setuptools/_distutils/_vendor/*.dist-info folders to behave in a certain way (despite it being a private directory and not added to sys.path). Although that issue is not really a bug in setuptools (in my opinion), simply removing distutils/_vendor would also help with that.

distutils/dist.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@jaraco jaraco closed this Sep 2, 2024
@jaraco jaraco reopened this Sep 2, 2024
@jaraco
Copy link
Member

jaraco commented Sep 2, 2024

The diffcov errors are weird, probably there due to the broken main brought about by a nitpicky change in ruff import sorting, which is now fixed in 7ee6a6e.

@jaraco jaraco merged commit de0bb44 into pypa:main Sep 2, 2024
27 of 40 checks passed
@abravalheri
Copy link
Contributor Author

abravalheri commented Sep 2, 2024

Thank you very much for the review and improvements @jaraco.

Adding this comment pushes me a little closer to releasing distutils as its own package (with a caveat that it's not meant to be installed anywhere), and then including it as a vendored package in Setuptools (just like any other, with its dependencies). I'm not prepared to do that yet, but it's more tempting now that distutils has dependencies.

There is another approach as well which is to "just let setuptools do its thing" and remove this concern from distutils (instead, just do a weaker/simpler canonicalization that does not require 3rd party packages1).
Currently setuptools patches DistutilsMetadata because there is no mechanism for using a subclass when setuptools.dist.Distribution is instantiated... So for now we are stuck with this approach, and therefore adding dependencies to distutils has little pay off (patching is necessary anyway).

In the future we either start moving code from setuptools to distutils to remove the need for patching, or add a method that allows using DistutilsMetadata subclasses when instantiating setuptools.dist.Distribution2.

Footnotes

  1. We can safely assume that by the stage that we need the canonicalization, name and version have already been validated, so we could use some regex for the alpha|beta|rc => a|b|c and .|- => _ replacements.

  2. For example in https://github.com/pypa/distutils/blob/de0bb446b5a953e8fa48998980c07496533968bc/distutils/dist.py#L149, instead of having self.metadata = DistributionMetadata() we could do self.metadata = self._create_metdata(), so that we can extend DistutilsMetadata via subclassing instead of patching.

@abravalheri abravalheri deleted the remove-vendor branch September 2, 2024 13:05
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.

2 participants