-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PEP 770: Add the additional-files table and reserve dist-info dirs #4283
base: main
Are you sure you want to change the base?
Conversation
I believe this proposal constitutes a significant scope change and is not aligned with the original objectives. The initial scope was to add SBOM to packages. However, the proposed changes introduce the addition of arbitrary data to packages, which significantly broadens the scope. This new capability/topic deviates from the original focus and introduces complexities that will progress at a different pace than the initial scope. It appears that this change could be an attempt to shift the focus or slow down the progress by diverting from the original scope. I strongly oppose this PR and the proposed changes. If there is a substantial interest in including arbitrary data in packages, a separate PEP should be initiated. |
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.
Thanks for the quick update @sethmlarson. I reviewed both the diff and the full new text, it looks pretty good to me. This change is a signifcant improvement overall in my opinion.
The checks on what .dist-info
directories are already in use looks thorough, thanks for that.
The sentence under Acknowledgements referencing PEP 639 is out of date now, it still says that this PEP implements the PEP 639-like scheme.
You may want to mention somewhere that using a separate [additional-files]
key means that packages can start using it directly after acceptance of this PEP, rather than have a >1 yr rollout plan that would have been needed with a core metadata version bump.
You may want to mention that SBOM tooling should always check the actual artifact of interest; no assumptions can be made about the sboms
directory having the same contents between different artifacts for a single version of a package.
|
||
* Most subdirectories under ``.dist-info`` are to do with licensing, | ||
one of which (``licenses``) is specified by :pep:`639` and other | ||
(``license_files``) which is being used by the Hatch build backend. |
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 various spellings of the licenses
directory looks like a potential issue (if not very related to this PR). PEP 639 is pretty clear that it must be lower-case licenses
. Hatch always tries following packaging standards, so I assume that this is an oversight. It's also possible though that there's a conflict with REUSE, because https://reuse.software/faq/#multi-licensing requires an upper-case LICENSES
directory. So it's likely that both may have to be allowed (which you already did for license_files
in this PR), and the up-to-date version of PEP 639 language in the packaging user guide made more permissive.
license_files
looks more like a mistake though.
There's a whole bunch of issues on the Hatch issue tracker about licenses, so hard to tell what the current state is. @ofek maybe you can clarify, and check if this Hatch mention needs a tweak?
``src`` 3 | ||
``calmjs_artifacts`` 3 | ||
``.idea`` 2 | ||
====================== =============== |
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.
Most of these look like mistakes. Possible exceptions are the license-related directories and include
. Is it easy to find the artifacts that contained include
?
================= ========== | ||
Directory name PEP | ||
================= ========== | ||
``licenses`` :pep:`639` |
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.
Add LICENSES
to this list of allowed directory names as well (see more extensive comment further down)?
================= ========== | ||
|
||
Build backends MUST NOT create subdirectories in the ``.dist-info`` directory | ||
beyond the names in the registry to avoid collisions with future reserved names. | ||
|
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.
Should the text say anything here about other tools like frontends and publishing tools? I'd expect that build frontends want to do nothing, however twine
and uv publish
MAY want to validate new artifacts before uploading them.
* Other subdirectory names under ``.dist-info`` appear to be either not | ||
widespread or accidental. | ||
|
||
As a result of this query |
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.
Incomplete sentence. I think you want to draw a conclusion here. Probably that it's recommended for frontends to not complain about unknown directory names for the sake of backwards compatibility?
@jkowalleck please don't post the same comment in two places. You seem to be new to Python packaging conversations, so for context: Discourse is the right place for high-level discussion and comments like "I prefer approach X over approach Y because Z"; PEP PRs are more editorial and for initial reviews on correctness/completeness before posting to a broader audience on Discourse. |
@sethmlarson the description of PEP 725 looks pretty good as well. I'd suggest one small tweak to make the first bullet point more clear, since "abstract dependencies" isn't a well-defined term and you're only giving The analogy that I tend to use is "dependencies in |
re: #4283 (review)
Is "additional-files" part of core metadata already? |
It's not a metadata item, it's a key in |
re: #4283 (comment)
Could you explain what this "rather than have a >1 yr rollout plan that would have been needed with a core metadata version bump" is supposed to mean? |
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.
This looks great to me. I think any comments I might have had have already been covered by @rgommers
You've put a lot more details into the specification of additional-files
than I had anticipated, but that's a good thing - it feels like it's well prepared for any future use.
@jkowalleck If we put the data into the core metadata (ignoring for now the problem that we don't have a viable way of doing so that conforms to the rules around dynamic and static metadata) , then we'd have to wait for a new metadata version to be implemented by build backends, recognised by PyPI and other tools, and supported throughout the ecosystem before it would be usable. The experience with the license-files metadata is that (a) this is a slow process, and (b) if tools try to implement support before the infrastructure is in place, we end up causing problems for users that take even longer to fix. Conversely, if we add a new key to But as @rgommers said, can this discussion be relocated to Discourse if you wish to continue it, please? |
PEP 123: Summary of changes
)cc @rgommers
📚 Documentation preview 📚: https://pep-previews--4283.org.readthedocs.build/