twine icon indicating copy to clipboard operation
twine copied to clipboard

Restore support for pkginfo 1.11

Open jaraco opened this issue 1 year ago • 11 comments

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 avatar Jun 25 '24 16:06 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.

jaraco avatar Jun 25 '24 18:06 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.

woodruffw avatar Jun 26 '24 13:06 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.

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 https://github.com/pypa/twine/issues/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 avatar Jun 26 '24 14:06 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 avatar Sep 06 '24 20:09 jaraco

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

jaraco avatar Sep 06 '24 20:09 jaraco

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

stefanor avatar Sep 14 '24 08:09 stefanor

To get this moving again, perhaps can just reduce the coverage target at https://github.com/pypa/twine/blob/ae71822a3cb0478d0f6a0cccb65d6f8e6275ece5/tox.ini#L22 to 96 since this does not appear to demonstrably reduce the coverage? Maybe just a rounding difference?

chadlwilson avatar Sep 25 '24 08:09 chadlwilson

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 avatar Sep 25 '24 09:09 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)}."
                )

stefanor avatar Sep 25 '24 09:09 stefanor

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!

satmandu avatar Oct 04 '24 15:10 satmandu

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 https://github.com/chadlwilson/twine/pull/1 / https://github.com/chadlwilson/twine/actions/runs/11183953472

chadlwilson avatar Oct 04 '24 17:10 chadlwilson

Any progress on this? It would be nice to be able to use twine from a tagged release as opposed to having to build from this bugfix branch.

satmandu avatar Nov 11 '24 22:11 satmandu

Well you can get apt install twine on debian and it's patched.

ltworf avatar Nov 13 '24 16:11 ltworf

Well you can get apt install twine on debian and it's patched.

It certainly looks like you have patched it downstream! Thank you!

As a Maintainer for Chromebrew I'm just hoping for upstream here to patch it in a tagged release so we can stop shipping a patched release/having to maintain a patch against upstream, especially since we use twine for our own binary pip mirror.

https://metadata.ftp-master.debian.org/changelogs//main/t/twine/twine_5.1.1-3_changelog

twine (5.1.1-3) unstable; urgency=low

  * Team upload.
  * Add a real manpage for twine, and move the existing one to section 3.
    (Closes: #1029494)
  * Move the generated manpage to section 3.

 -- Salvo 'LtWorf' Tomaselli <[email protected]>  Sat, 14 Sep 2024 12:41:14 +0200

twine (5.1.1-2) unstable; urgency=medium

  * Patch: Restore support for pkginfo 1.11.
  * Revert the Build-Depends restriction on python-pkginfo.
    (Closes: #1081549)

 -- Stefano Rivera <[email protected]>  Sat, 14 Sep 2024 10:13:45 +0200

twine (5.1.1-1) unstable; urgency=high

  * Team upload.
  * New upstream release.
    + Fix compatibility with python-importlib-metadata 8.x.
  * debian/control: Add an upper version requirement for build-dependency
    python-pkginfo (<< 1.11.0~) due to upstream issue 1116. This limit
    shall be solved with the next upstream release.

 -- Boyuan Yang <[email protected]>  Sun, 30 Jun 2024 12:13:27 -0400

satmandu avatar Nov 13 '24 16:11 satmandu

Any progress on this? It would be nice to be able to use twine from a tagged release as opposed to having to build from this bugfix branch.

is there a way to use this branch within the gh-action-pypi-publish action?

maturin switched to metadata 2.4 (released 16 hours ago) and so all rust package releases will start breaking in the coming days unless they revert to 2.3 (which I guess they can't because ~"Metadata-Version 2.3 is invalid and now gets rejected by PyPI"~ ref) or this PR merges and releases

ddelange avatar Nov 29 '24 09:11 ddelange

is there a way to use this branch within the gh-action-pypi-publish action?

The easiest way would probably be to have it land in a release. It might alternatively be possible to patch it in, but @webknjaz should probably opine on whether he'd like that 🙂

Taking a step back: apologies if I'm missing anything, but is there a reason twine can't go to pkginfo >= 1.11 and avoid the backwards compat complexity here? Distributions that need the older version could still use patch on this branch, but the twine that lives on PyPI/gets used by the GHA publishing action doesn't need it.

woodruffw avatar Nov 29 '24 16:11 woodruffw

Taking a step back: apologies if I'm missing anything, but is there a reason twine can't go to pkginfo >= 1.11 and avoid the backwards compat complexity here?

See this comment.

I observed that there were already objections to pinning to pkginfo>=1.10 (#1070), so chose to avoid that issue and provide a backward-compatible change. I was interested in solving that issue also and clean up the compatibility code, but planned to do it subsequently after providing an intermediate compatible release. I'd even consider making two releases in the same day, one that provides backward compatibility and one that removes that compatibility, so that users and integrators would have more flexibility.

Since @sigmavirus24 has been unresponsive to my request for clarification, this work is essentially blocked.

jaraco avatar Nov 29 '24 17:11 jaraco

The profile page for @sigmavirus24 suggests that he is “Taking a break from F/0SS indefinitely” — As such, is there another maintainer who may be approached about moving this forward?

satmandu avatar Nov 29 '24 17:11 satmandu

@woodruffw I'd rather not have Git-based deps. If we need to pull this in, we'd have to consider vendoring+patching a-la what pip does. But I'd also need to account for pip-tools based pins management.

webknjaz avatar Nov 29 '24 17:11 webknjaz

I'm guessing a PEP440 direct reference like pip install 'twine @ https://github.com/pypa/twine/archive/refs/heads/bugfix/1116-pkginfo-warnings.zip' should be supported by pip-tools?

ddelange avatar Nov 29 '24 18:11 ddelange

The profile page for @sigmavirus24 suggests that he is “Taking a break from F/0SS indefinitely” — As such, is there another maintainer who may be approached about moving this forward?

If that's the case, I also believe @bhrutledge has stepped away as well, in which case I'm the last remaining active contributor on this project. I looked at the profile page of sigmavirus24, but didn't see the "taking a break" message. Can you provide a more definitive reference?

jaraco avatar Nov 29 '24 18:11 jaraco

The profile page for @sigmavirus24 suggests that he is “Taking a break from F/0SS indefinitely” — As such, is there another maintainer who may be approached about moving this forward?

If that's the case, I also believe @bhrutledge has stepped away as well, in which case I'm the last remaining active contributor on this project. I looked at the profile page of sigmavirus24, but didn't see the "taking a break" message. Can you provide a more definitive reference?

Screenshot_20241129-134059.png

satmandu avatar Nov 29 '24 18:11 satmandu

I see now I missed the opportunity to add a changelog entry for this change. I'll do that in a separate PR.

jaraco avatar Nov 29 '24 19:11 jaraco

I see now I missed the opportunity to add a changelog entry for this change. I'll do that in a separate PR.

If you would be so kind as to tag a release thereafter. 🙏

satmandu avatar Nov 29 '24 19:11 satmandu

v6.0.0 has been released, thanks for the quick action here! :boom:

ddelange avatar Dec 01 '24 09:12 ddelange