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

Clearly specify that the name and version should be normalized for .dist-info and .data directories in wheels #1789

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brettcannon
Copy link
Member

@brettcannon brettcannon commented Jan 15, 2025

brettcannon and others added 2 commits January 15, 2025 13:14
@brettcannon
Copy link
Member Author

@brettcannon brettcannon marked this pull request as ready for review January 15, 2025 22:36
@encukou
Copy link
Contributor

encukou commented Jan 16, 2025

Please add a line to the History section, too.

@ncoghlan ncoghlan self-assigned this Jan 17, 2025
@ncoghlan
Copy link
Member

As per my comment on the Discourse thread: I do plan to approve & merge this, I'm just waiting until early next week to give folks a chance to object if they consider this a spec change, rather than just a clarification.

Comment on lines +153 to +156
equivalent to regular :ref:`name normalization <name-normalization>` followed
by replacing ``-`` with ``_``. Tools consuming wheels must be prepared to accept
``.`` (FULL STOP) and uppercase letters, however, as these were allowed by an earlier
version of this specification.
Copy link
Member

Choose a reason for hiding this comment

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

Note for myself: this is just a formatting change. I compared it word-for-word to make sure I didn't miss anything.

Comment on lines +179 to +182
``beaglevote`` and {version} is replaced
with its :ref:`normalized version <version-specifiers-normalization>`,
e.g. ``1.0.0``, (with dash/``-`` characters replaced with underscore/``_`` characters
in both fields) consist of:
Copy link
Member

Choose a reason for hiding this comment

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

This wording makes it sound that the normalized version can have underscores. But it links https://python-packaging-user-guide--1789.org.readthedocs.build/en/1789/specifications/version-specifiers/#version-specifiers-normalization which says

Pre-releases should allow a ., -, or _ separator between the release segment and the pre-release segment. The normal form for this is without a separator. This allows versions such as 1.1.a1 or 1.1-a1 which would be normalized to 1.1a1.

(emphasis mine)

Was this the intention?

Another thought, reading this — how should it handle local versions specified by PEP 440? The + char isn't explicitly listed, but I'd expect it to also be normalized, I suppose…

Copy link
Member

Choose a reason for hiding this comment

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

The main thing I wanted to make clear when I suggested the parenthesised addition was that while the linked "Name normalisation" rule uses hyphens, we need to use underscores here. I don't care that much about the actual wording, as long as it's clear that neither the {distribution} nor the {version} part of the name is allowed to contain hyphens.

The awkwardness of having the normalisation rule in "Names and normalisation" use hyphens, where the wheel format uses hyphens to separate components (and as a result must use underscores rather than hyphens within components) is something we really should find a way of fixing at some point, because having to continually add "but with dashes replaced by underscores" is annoying, and easy to forget 🙁

Another thought, reading this — how should it handle local versions specified by PEP 440? The + char isn't explicitly listed, but I'd expect it to also be normalized, I suppose…

My reading of PEP 440 suggests that the + character is valid in a normalised version, and there's no reason not to allow it in dist-info names. Why do you think otherwise? Changing the spec to require replacing the + with another character would be a spec change, not a clarification, though, so if you do want to do this it should be a separate proposal.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't realize the primary objective was to disambiguate the semantics of dashes, so pluses seemed in the scope. But it sounds like it's not. Thanks for the clarification!

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.

5 participants