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

[BUG] Vendored packages include incomplete dist-info data in wheels #4594

Closed
freakboy3742 opened this issue Aug 25, 2024 · 4 comments
Closed
Labels
bug Needs Triage Issues that need to be evaluated for severity and status.

Comments

@freakboy3742
Copy link

setuptools version

setuptools>=73.0

Python version

3.8+

OS

macOS

Additional environment information

No response

Description

Setuptools contains a number of vendored packages. In Setuptools 71.0.0 and earlier (inclusive), these were "just folders" - the contents of the code for those dependencies, included in the setuptools package so the dependencies could be used without the bootstrapping issue of installing the dependencies of a dependency management tool.

As of 00384a5, released in Setuptools 71.0.1, these vendored packages also include the dist-info data for those packages.

There are also some "transitive" vendor dependencies; the _distutils package itself has a _vendor subpackage. As of 0f6ed20, released in setuptools 73.0, this folder contains a .dist-info folder for the packaging dependency; however, the macOS wheel only contain licenses files in the `_distutils/_vendor/packaging-24.0.dist-info folder. The WHEEL, RECORD, METADATA, INSTALLER and REQUESTED files have not been included in the wheel.

It's not clear what the intended behaviour is here. The inclusion of .dist-info folders may have been intentional; however, it's not immediately obvious to me that it is. AIUI, these dist-info packages can't be used by importib, as the _vendor subfolder isn't directly on the PYTHONPATH. The files in the _vendor folder are included in the manifest of the top-level setuptools package metadata, so they haven't been included to avoid that duplication.

If it was intentional, then the macOS wheels for 73.0+ are incomplete, as the distutils vendored version of packaging has an invalid .dist-info folder.

This problem was observed as beeware/briefcase#1970, which needs to do some post-install processing on installed dependencies. This involves looking for .dist-info folders to find content that need to be post-processed; this has historically been done with a **/*.dist-info glob. As a result, it found the _distutils/_vendor/packaging-24.0.dist-info folder, but then broke because it isn't a valid dist-info folder.

To be clear - this is also a bug in Briefcase, as we should only be looking for top-level dependencies. However, the inconsistent state of dist-info files in the published artefacts also seems like a possible bug.

Expected behavior

Either complete .dist-info packages for all vendored dependencies; or no .dist-info content in vendored dependencies.

How to Reproduce

Compare the setuptools/_distutil/_vendor/packaging-24.0.distinfo folder with the same content macOS wheels.

Output

@abravalheri
Copy link
Contributor

Hi @freakboy3742 please note that the License files are intentionally distributed because it is kind of a legal obligation (or at least moral).

The fact that setuptools/_distutils/_vendor/*.dist-info are incomplete should not cause any problems because the directory is not included in the path... Therefore no tools should be reading those automatically, right? (There is a standard for *.dist-info, sure; but the same standard also tells they should be placed under a directory somewhere in sys.path; so I don't think the standard applies in this particular circumstance).

We could include the other files too, if we verify this is not an isolated problem with a specific tool. But to be honest my preferred approach would be to rework the pypa/distutils repository so that it does not contain the _vendor folder at all... Of course this is something that has to be debated with the maintainers of pypa/distutils.

@freakboy3742
Copy link
Author

@abravalheri As I noted in the ticket description, the deep nested .dist-info folder is causing a problem for Briefcase; but that is also a bug in Briefcase (beeware/briefcase#1970 for the report, beeware/briefcase#1972 for the fix).

I raised this ticket because it wasn't clear if it was an intentional change, or an accident of the tool updating vendored packages.

Including LICENSE files definitely makes sense as a rationale; but these files weren't included at all until 73.0 (in the case of distutils), or 71.0.1 (in the case of packaging and the other packages in _vendored). The PRs don't make any mention of "we're now including license data to maximise compliance" or anything like that - they're just "vendored dependency updates".

Regardless, I agree there is an obligation to include the licenses. My question/counterargument would be whether including the licences in a deep nested .dist-info folder meets these obligations. Yes, the licenses are "technically there", but the location is neither (a) parseable by standard dist-info tooling, because the metadata is incomplete; or (b) in a standard location that would be discovered by dist-info tooling. Including the downstream licenses in setuptools.dist-info would make more sense to me from a formal compliance perspective.

That said - I don't mind much either way what the resolution of this ticket is. As I've noted above, Briefcase has a fix for our usage. I raised this ticket because it was a change that we noticed that did trigger a bug report, and it wasn't obvious the change was completely intentional.

@abravalheri
Copy link
Contributor

This is the change that added the license files, indicating the intentionality of it:

a78df65

Adding license files is kind of an obligation. That is why I added the change when I noticed they where not included (that was a bug in my opinion).

I believe there is nothing in the license file requiring a parseable location however. Please let me know if my interpretation is wrong here.

@abravalheri
Copy link
Contributor

abravalheri commented Aug 27, 2024

I will close this issue now because I believe it to not be a bug in the setuptools codebase.

My long term plan is to liase with distutils maintainers to discuss the possibility of not having a _vendor folder at all. That would be an improvement in many aspects.

@abravalheri abravalheri closed this as not planned Won't fix, can't repro, duplicate, stale Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Needs Triage Issues that need to be evaluated for severity and status.
Projects
None yet
Development

No branches or pull requests

2 participants