Skip to content

Stop best_effort_version duplicating part of a v-prefixed version - #5339

Open
feiiiiii5 wants to merge 2 commits into
pypa:mainfrom
feiiiiii5:hunt/pkg_resources
Open

feiiiiii5 wants to merge 2 commits into
pypa:mainfrom
feiiiiii5:hunt/pkg_resources

Conversation

@feiiiiii5

Copy link
Copy Markdown

Summary of changes

best_effort_version slices the trailing part of an unparseable version with v[len(safe):], but _PEP440_FALLBACK consumes the optional leading v outside the safe group:

_PEP440_FALLBACK = re.compile(
    r"^v?(?P<safe>(?:[0-9]+!)?[0-9]+(?:\.[0-9]+)*)", re.IGNORECASE
)

so len(safe) is one short of the real match end. With a v prefix the slice starts a character late and the last digit of the numeric prefix is copied into the local version segment.

v is legal PEP 440 and safe_version already accepts it through packaging.version.Version, which normalizes it away — so a prefix must not change the result. It does, for 6 of the 9 inputs I sampled:

input without v with v on main
1.2-foo 1.2.dev0+sanitized.foo 1.2.dev0+sanitized.2.foo
1.2.3-2-gabc 1.2.3.dev0+sanitized.2.gabc 1.2.3.dev0+sanitized.3.2.gabc
0.23- 0.23.dev0+sanitized 0.23.dev0+sanitized.3
3.11.4-1-gdeadbee 3.11.4.dev0+sanitized.1.gdeadbee 3.11.4.dev0+sanitized.4.1.gdeadbee

The other three are already consistent because they parse as valid PEP 440, so the fallback is never reached. The function's own docstring documents best_effort_version("0.23-") -> '0.23.dev0+sanitized', so the v0.23- form contradicts the documented behaviour for the same input.

This is not only about malformed input: safer_best_effort_version is what command/dist_info.py calls, so the wrong string reaches a .dist-info directory name — 78.1.0.dev0+sanitized.0.2.g3a3144f0d.dist-info instead of 78.1.0.dev0+sanitized.2.g3a3144f0d.dist-info.

The fix slices from the end of the whole match. The pattern is anchored with ^ and not re.MULTILINE, so match.start() == 0 always and match.end() is the correct absolute offset:

            rest = v[match.end() :]

On #4948

This is the same misalignment #4948 fixes as a side effect, by moving v? inside the safe group instead of changing the slice. I ran both regexes side by side and the two produce the same output here, and #4948's doctest already asserts the corrected value:

    >>> best_effort_version("v78.1.0-2-g3a3144f0d")
    '78.1.0.dev0+sanitized.2.g3a3144f0d'

So there is no disagreement to resolve. I filed this separately because #4948 is a feature PR that adds a template parameter, changes the signature, is currently CONFLICTING, and is still asking whether the approach is acceptable — this gives the off-by-one a path to land on its own. If you would rather it ride along with #4948, the regression test carries over unchanged and I am happy to close this one.

Closes #5338

Tests and gates

Tests

New setuptools/tests/test_normalization.py: 12 cases, of which the load-bearing ones assert the invariant rather than a literal — best_effort_version("v" + s) == best_effort_version(s) — so they pin the property instead of over-specifying what the sanitizer should do with malformed input. Two of them also check the v-less result stays exactly what the existing doctests already document, so the fix cannot quietly change that.

Against unmodified source:

E  AssertionError: assert '1.2.dev0+sanitized.2.foo' == '1.2.dev0+sanitized.foo'
E    - 1.2.dev0+sanitized.foo
E    + 1.2.dev0+sanitized.2.foo
7 failed, 5 passed in 0.05s
check result
setuptools/tests/test_normalization.py 12 passed
pytest --doctest-modules setuptools/_normalization.py 8 passed — addopts contains --doctest-modules, so this is a gate
fast subset (6 files) 94 passed, unchanged from baseline
mutation: revert to v[len(safe) :] 7 failed
ruff check / ruff format --check clean on both files

Not run: mypy (not installed), tox -e docs, and the rest of setuptools/tests — 1028 is a collection count, and most of the remainder build wheels or install into throwaway environments. test_namespaces.py also works but costs ~8s per test, so I left it out of the loop.

News fragment

newsfragments/5338.bugfix.rst:

Stopped duplicating part of the version when sanitizing an unparseable version
that starts with a ``v`` prefix.

Per newsfragments/README.rst: past tense, end-user-facing, one sentence. I will renumber to the PR number if that is preferred over the issue number.

Pull Request Checklist

_PEP440_FALLBACK consumes an optional leading `v` outside the `safe`
group, so `len(safe)` is one short of the real match end. Slicing with
v[len(safe):] therefore started a character late and copied the last
digit of the numeric prefix into the local version segment.

`v` is legal PEP 440 and safe_version already accepts it, so a prefix
must not change the result: best_effort_version("v1.2-foo") returned
1.2.dev0+sanitized.2.foo where best_effort_version("1.2-foo") returned
1.2.dev0+sanitized.foo. Six of nine sampled inputs were affected, and
safer_best_effort_version feeds the .dist-info directory name.

Slice from the end of the whole match instead.

Fixes pypa#5338
@mergify

mergify Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

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.

best_effort_version duplicates part of the version when the input has a v prefix

1 participant