Restore support for pkginfo 1.11 #1123

jaraco · 2024-06-25T16:47:51Z

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 · 2024-06-25T18:24:14Z

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.

twine/package.py

twine/repository.py

woodruffw · 2024-06-26T13:55:44Z

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 · 2024-06-26T14:01:41Z

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.

Ref #1116

…rsions.

…e signature.

jaraco · 2024-09-06T20:32:52Z

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 · 2024-09-06T20:52:48Z

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

stefanor · 2024-09-14T08:16:03Z

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

chadlwilson · 2024-09-25T08:46:13Z

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 · 2024-09-25T09:03:58Z

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 · 2024-09-25T09:07:37Z

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 · 2024-10-04T15:19:21Z

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 · 2024-10-04T17:10:14Z

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 · 2024-06-26T15:26:22Z

twine/package.py

+
+    @staticmethod
+    def _pkginfo_before_1_11() -> bool:
+        ver = packaging.version.Version(importlib_metadata.version("pkginfo"))


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

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

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.

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!)

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>

tests: Validate custom error message for pkginfo < 1.11

jaraco force-pushed the bugfix/1116-pkginfo-warnings branch 5 times, most recently from 53f0216 to 71346e0 Compare June 25, 2024 18:11

jaraco marked this pull request as ready for review June 25, 2024 18:13

jaraco mentioned this pull request Jun 25, 2024

Rely on .get for possibly undefined metadata. #1124

Closed

henryiii mentioned this pull request Jun 25, 2024

fix: use correct location for URL #1126

Closed

jaraco requested review from woodruffw, sigmavirus24 and bhrutledge and removed request for woodruffw June 25, 2024 22:54

jaraco mentioned this pull request Jun 26, 2024

Use the stdlib importlib.metadata when available #1024

Merged

woodruffw reviewed Jun 26, 2024

View reviewed changes

twine/package.py Outdated Show resolved Hide resolved

woodruffw reviewed Jun 26, 2024

View reviewed changes

twine/repository.py Outdated Show resolved Hide resolved

jaraco force-pushed the bugfix/1116-pkginfo-warnings branch 2 times, most recently from 1ca93e2 to a0fdb54 Compare June 26, 2024 14:29

jaraco added 7 commits June 26, 2024 10:38

Split test for no_metadata and unrecognized_version.

a320607

Pin to pkginfo<1.11 to address test failures.

708738a

Ref #1116

Add logic capturing when the declared metadata version is not supported.

03e3795

Only emit the supported metadata versions notice for older pkginfo ve…

2191d47

…rsions.

Remove workaround now that pkginfo 1.11 is supported.

759c9ff

Cast name and version to strings.

7f201a0

Cast the Distribution to a CheckedDistribution, capturing the new typ…

a0da730

…e signature.

jaraco force-pushed the bugfix/1116-pkginfo-warnings branch from a0fdb54 to a0da730 Compare June 26, 2024 14:39

Merge branch 'main' into bugfix/1116-pkginfo-warnings

e23ffaa

jaraco mentioned this pull request Jun 26, 2024

Twine should require pkginfo > 1.10 #1070

Open

1 task

jaraco mentioned this pull request Sep 6, 2024

Twine fails to upload packages with latest metadata-version #1146

Open

1 task

Merge branch 'main' into bugfix/1116-pkginfo-warnings

c577765

agriyakhetarpal mentioned this pull request Sep 29, 2024

Implement PEP 639: support for Metadata-Version: 2.4 ziglang/zig-pypi#24

Merged

chadlwilson mentioned this pull request Oct 4, 2024

tests: Validate custom error message for pkginfo < 1.11 #1156

Merged

sigmavirus24 requested changes Oct 5, 2024

View reviewed changes

sigmavirus24 and others added 3 commits October 5, 2024 06:31

Merge branch 'main' into bugfix/1116-pkginfo-warnings

706144b

tests: Validate custom error message for pkginfo < 1.11

d9c409d

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>

Merge pull request #1156 from chadlwilson/1116-pkginfo-warnings-coverage

d53a8a6

tests: Validate custom error message for pkginfo < 1.11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore support for pkginfo 1.11 #1123

Restore support for pkginfo 1.11 #1123

jaraco commented Jun 25, 2024 •

edited

Loading

jaraco commented Jun 25, 2024

woodruffw commented Jun 26, 2024

jaraco commented Jun 26, 2024 •

edited

Loading

jaraco commented Sep 6, 2024

jaraco commented Sep 6, 2024

stefanor commented Sep 14, 2024

chadlwilson commented Sep 25, 2024

stefanor commented Sep 25, 2024

stefanor commented Sep 25, 2024

satmandu commented Oct 4, 2024

chadlwilson commented Oct 4, 2024 •

edited

Loading

sigmavirus24 Jun 26, 2024

chadlwilson Oct 5, 2024

jaraco Oct 5, 2024

satmandu Oct 11, 2024

Restore support for pkginfo 1.11 #1123

Are you sure you want to change the base?

Restore support for pkginfo 1.11 #1123

Conversation

jaraco commented Jun 25, 2024 • edited Loading

jaraco commented Jun 25, 2024

woodruffw commented Jun 26, 2024

jaraco commented Jun 26, 2024 • edited Loading

jaraco commented Sep 6, 2024

jaraco commented Sep 6, 2024

stefanor commented Sep 14, 2024

chadlwilson commented Sep 25, 2024

stefanor commented Sep 25, 2024

stefanor commented Sep 25, 2024

satmandu commented Oct 4, 2024

chadlwilson commented Oct 4, 2024 • edited Loading

sigmavirus24 Jun 26, 2024

Choose a reason for hiding this comment

chadlwilson Oct 5, 2024

Choose a reason for hiding this comment

jaraco Oct 5, 2024

Choose a reason for hiding this comment

satmandu Oct 11, 2024

Choose a reason for hiding this comment

jaraco commented Jun 25, 2024 •

edited

Loading

jaraco commented Jun 26, 2024 •

edited

Loading

chadlwilson commented Oct 4, 2024 •

edited

Loading