Restore support for pkginfo 1.11

[jaraco]

For now, this approach retains the current behavior that an unrecognized metadata version will fail with an error.

This approach splits out the detection based on the version of pkginfo. Starting with pkginfo 1.11, it now properly detects when a metadata version is unrecognized and errors on that condition explicitly. For backward compatibility, until pkginfo 1.11 is the minimum, the check will still provide guidance to consider the metadata version when fields are missing.

Closes #1116

[jaraco]

Well, shoot. It seems that after allowing pkginfo 1.11, the type checks are failing. It seems that pkginfo.Distribution.name was str and now is str | None. Same for .version.

[woodruffw]

Random thought: is there a reason this can't constrain pkginfo ~= 1.11 in pyproject.toml? That would avoid the need for the run-time version check + some of the extra handling in this PR.

[jaraco]

Random thought: is there a reason this can't constrain pkginfo ~= 1.11 in pyproject.toml? That would avoid the need for the run-time version check + some of the extra handling in this PR.

My thought was that there are probably users still reliant on older versions, so it would be nice to have at least one release transition where users aren't forced to the newer version. I absolutely agree that after twine stabilizes, we might bump the dependency on pkginfo (and remove the compatibility logic). I coded it in such a way as to make it easy to remove the compatibility logic.

I also observed that there's already an open issue #1070 tracking the move to pkginfo 1.10, so that issue would need to be addressed first (or simultaneously) with the move to 1.11.

[jaraco]

I don't understand why the coverage tests are failing. They weren't failing before. Maybe the upstream changes pushed the coverage marginally lower and now these changes push them even lower? Since the coverage failures are for total coverage and not diff coverage, it's difficult for me to see what changes in this PR have contributed to the failing check.

[jaraco]

Due to #1121, I'm unable to locally assess the coverage issues.

[stefanor]

Applied this PR as a patch, in Debian's twine.

[chadlwilson]

To get this moving again, perhaps can just reduce the coverage target at

twine/tox.ini

Line 22 in ae71822

python -m coverage report --skip-covered --show-missing --fail-under 97

to 96 since this does not appear to demonstrably reduce the coverage? Maybe just a rounding difference?

[stefanor]

Due to #1121, I'm unable to locally assess the coverage issues.

That's trivially worked around with HOME=/tmp/home/:

--- /tmp/before.coverage	2024-09-25 11:02:57.056573133 +0200
+++ /tmp/after.txt	2024-09-25 11:00:16.951779487 +0200
@@ -8,10 +8,10 @@
 twine/__init__.py             14      1      4      2    83%   39, 46->exit
 twine/cli.py                  34      2      6      2    90%   22, 86
 twine/commands/upload.py      95      1     39      1    99%   103
-twine/package.py             159      5     55      6    95%   27, 111, 139, 227, 306->exit, 312
-twine/repository.py          118      3     46      4    96%   90, 104->exit, 108->exit, 164->exit, 198, 250
+twine/package.py             178      6     67      7    95%   39, 132, 152, 167, 269, 348->exit, 354
+twine/repository.py          118      3     46      4    96%   90, 104->exit, 108->exit, 167->exit, 201, 253
 twine/settings.py             82      4     18      0    96%   333-341
 twine/utils.py               138      1     49      0    99%   330
 twine/wininst.py              38     26     16      0    26%   15-17, 21-25, 28-57
 ----------------------------------------------------------------------
-TOTAL                       2033     48    618     17    97%
+TOTAL                       2058     49    634     18    97%

[stefanor]

The new addition is:

https://github.com/pypa/twine/pull/1123/files#diff-30ee60b48548622c54dfa266e095021f403ad23ab4848c74b2deed5df329ae00R152

            if cls._pkginfo_before_1_11():
                msg += (
                    "\n"
                    "Make sure the distribution includes the files where those fields "
                    "are specified, and is using a supported Metadata-Version: "
                    f"{', '.join(supported_metadata)}."
                )

[satmandu]

Any updates on moving this PR forward? It would be really nice to be able to use a proper release of twine and not have to build twine from this patch branch!

[chadlwilson]

The new addition is:

https://github.com/pypa/twine/pull/1123/files#diff-30ee60b48548622c54dfa266e095021f403ad23ab4848c74b2deed5df329ae00R152

            if cls._pkginfo_before_1_11():
                msg += (
                    "\n"
                    "Make sure the distribution includes the files where those fields "
                    "are specified, and is using a supported Metadata-Version: "
                    f"{', '.join(supported_metadata)}."
                )

Thanks @stefanor

Coverage only falls below 97% on Python 3.8 as there is an unrelated additional branch missed there that predates this change.

Nevertheless, have covered the additional branch in #1156 - but needs @stefanor's #1154 first (or I could cherry-pick?) as CI has broken. Can see it all together at chadlwilson#1 / https://github.com/chadlwilson/twine/actions/runs/11183953472

[sigmavirus24]

I can't believe we really need packaging just for these two lines. This is just excessive

[chadlwilson]

Would you prefer to use python-semver? or just some sort of approach to avoid another dependency?

[jaraco]

I can't believe it either. It also requires importlib_metadata.

Unfortunately, packaging is the "standard" way to parse version numbers in Python. Also, this dependency goes away after dropping support for older pkginfo. It felt like a small compromise to require it for that short-term consideration.

I'm very much not interested in writing a version parser/comparator when one already exists. The whole point of libraries is to publish reusable, tested, robust implementations of behavior.

This comment comes off as a criticism without any advice on how to proceed. It's unclear what "excessive" even means. Would it also be excessive to vendor packaging? What about to copy just the version parsing logic? Any change that doesn't use packaging is almost certainly going to require more lines of code than this change, so in some sense is more involved than this excessive approach. I'm uninterested in fulfilling "bring another rock" just to have it declined. Please make a recommendation on what changes would be acceptable or adapt the change to be acceptable.

[satmandu]

Are there any concerns over this PR that still need to be further addressed?

(I've been building and using twine from this branch and it continues to work correctly!)