Repository navigation
Reject empty/invalid Tag components consistently - #1413
Pitchfork-and-Torch wants to merge 3 commits into
Conversation
Tag() accepted empty interpreter/abi/platform while parse_tag rejected them, and parse_tag allowed trailing junk like `py3-none-any,` because only the interpreter was checked with str.isidentifier(). Validate all three components in both paths.
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
str.isidentifier() is Python-identifier grammar, not compatibility-tag grammar. In particular it rejects a digit-leading ABI token, even though the compatibility-tag spec allows implementation-defined ABI hashes whose base64 text can start with a digit. Could this validate the actual tag grammar instead of identifier syntax?
Wkhanex
left a comment
There was a problem hiding this comment.
I checked the other validation paths: empty components in direct Tag construction and punctuation in each parsed component raise InvalidTag, while compressed py2.py3-none-any still expands to the expected product. This doesn't resolve the digit-leading ABI grammar concern above.
Checked 8bb6234d3eb7 with standalone Python 3.12.14 checks (7 passing assertions). I haven't run the full upstream suite.
str.isidentifier() rejects ABI hashes that the compatibility-tag spec allows to start with a digit. Match ABI and platform with [0-9A-Za-z_]+. Interpreter validation stays isidentifier().
|
Digit-leading ABI is a real spec case. The compatibility tag spec says an unstable ABI may be the first 6 bytes of a hash, written as 8 base64 characters, so the component may start with a digit. str.isidentifier() rejects that. ABI and platform now match Standard base64 also includes
|
|
Thanks for addressing the digit-leading ABI case. I verified |
Unstable ABI hashes are 8 standard base64 characters, so + is legal in ABI. Platform tags stay letters, digits, and underscore. Slash stays rejected.
py3-0abc+EF1-any parses. parse_wheel_filename("foo-1.0-py3-0abc+EF1-any.whl") accepts that filename. The old py3-none+-any fixture is gone, because none+ is a legal ABI once + is allowed. The negative ABI case is now py3-none/-any, and py3-none-any+ still fails as a platform. The PR description no longer says the ABI and platform checks use isidentifier(). Head is a60e074. |
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Rechecked head a60e074. The ABI validator now permits digit-leading values and the '+' character used by standard Base64 ABI hashes, while keeping '+' out of platform tags and rejecting slash, whitespace and punctuation that cannot appear in filename tag components. The wheel filename parser accepts the positive '+' regression, and the invalid-fixture updates match the new grammar. This resolves the compatibility concerns I raised.
Validation on Python 3.12.14/macOS arm64: 327 focused tags/utils tests passed; direct positive/negative tag and wheel-filename cases passed; full repository suite passed (62,446 passed, 1 skipped for glibc-only, 427 deselected). No remaining blocker from my review.
Summary
parse_tag()already raisesInvalidTagfor empty components, butTag()accepted them (Tag('', 'none', 'any')->-none-any). Separately, only the interpreter was checked withstr.isidentifier(), so tags likepy3-none-any,andpy3-none-anyparsed successfully with junk in the platform field.Tag.__init__rejects empty componentsstr.isidentifier()+(8 standard base64 characters may include+)./stays rejectedTest plan
tests/test_tags.pyandtests/test_utils.py(327 tests) pass