pypa / twine

Utilities for interacting with PyPI
https://twine.readthedocs.io/
Apache License 2.0
1.62k stars 309 forks source link

Twine check fails to warn on invalid classifier #976

Open pombredanne opened 1 year ago

pombredanne commented 1 year ago

Your Environment

Using twine:

$ twine --version
twine version 4.0.2 (importlib-metadata: 6.0.0, keyring: 23.13.1, pkginfo: 1.9.6, requests:
2.28.1, requests-toolbelt: 0.10.1, urllib3: 1.26.13)

I have a setup.cfg with this classifier: Development Status :: 4 - Beta there is an extra space before the 4

If I build a wheel and run twine check, it passes (see attached wheel where the .zip extension has been added to make this tracker happy) tracecode_toolkit_strace-0.21.0-py3-none-any.whl.zip

$ twine check dist/tracecode_toolkit_strace-0.21.0-py3-none-any.whl --strict
Checking dist/tracecode_toolkit_strace-0.21.0-py3-none-any.whl: PASSED

If I try to upload this to PyPI I get this error:

$ twine upload dist/tracecode_toolkit_strace-0.21.0-py3-none-any.whl 
Uploading distributions to https://upload.pypi.org/legacy/
Uploading tracecode_toolkit_strace-0.21.0-py3-none-any.whl
100% ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 97.6/97.6 kB • 00:00 • 458.3 kB/s
WARNING  Error during upload. Retry with the --verbose option for more details.                     
ERROR    HTTPError: 400 Bad Request from https://upload.pypi.org/legacy/                            
         Invalid value for classifiers. Error: Classifier 'Development Status ::  4 - Beta' is not a
         valid classifier.                           

I would expect the check to fail if the upload would fail.

sigmavirus24 commented 1 year ago

Spiritually I think this is a duplicate of #430. Regardless it's part of the overall theme of "twine doesn't check things that we have to reimplement because PyPI doesn't have a shared library with these validations"

pombredanne commented 1 year ago

Spiritually I think this is a duplicate of #430. Regardless it's part of the overall theme of "twine doesn't check things that we have to reimplement because PyPI doesn't have a shared library with these validations"

@sigmavirus24 You may have missed that PyPI now has a shared library for classifiers with https://github.com/pypa/trove-classifiers which is also used in warehouse since @di merged it in https://github.com/pypi/warehouse/pull/7582 ?

bhrutledge commented 1 year ago

@pombredanne I wasn't aware of that library, which I think does re-open the door for implementing this in Twine. That said, there's been a lot of discussion about how to add check behavior (e.g. https://github.com/pypa/twine/issues/848), and there's a whole project for check improvements.

That said: given that PyPI rejects the upload, I guess I'm not sure how much value there is in adding it to twine check. Perhaps naively, it seems like the only savings is the upload time, and the remediation would be the same. By contrast, IIRC the README validation in twine check is behavior that doesn't exist in Warehouse.

pombredanne commented 1 year ago

@bhrutledge Thanks for reopening!

You wrote:

That said: given that PyPI rejects the upload, I guess I'm not sure how much value there is in adding it to twine check. Perhaps naively, it seems like the only savings is the upload time, and the remediation would be the same.

FWIW, the problem for me is that when PyPI rejects an upload, it is too late: the code has already been validated, tests have run, the repo is tagged, a release has been created and wheels/sdist built, typically all automatically from a tag.

A failure to upload means I need to update classifiers and respin a mostly ugly dot release for the sole purpose of fixing the classifiers. And I eventually want to yank the previous release or tag so there is no dangling tag that's not published to PyPI.

I would much prefer a twine check failing during a test run in the CI.

sigmavirus24 commented 1 year ago

Many people will create a pre-release tag that will upload to Test PyPI. Then you can make changes before making a final release. This too can be automated the same way your normal release flow is. You can even see if it uploads to Test PyPI and then optionally release to PyPI as well for people to install and test from if you want.

This doesn't need to be solved by Twine.

There is far too much warehouse checks that we cannot and every inch we move towards doing that, is more maintenance burden and momentum we have to resist reimplementing (poorly) those checks. (Especially since Warehouse doesn't externalize those often or ever)

bhrutledge commented 1 year ago

@pombredanne Thanks for the details of your scenario. That makes sense, and I see how an upload failure is cumbersome.

There is far too much warehouse checks that we cannot and every inch we move towards doing that, is more maintenance burden and momentum we have to resist reimplementing (poorly) those checks. (Especially since Warehouse doesn't externalize those often or ever)

This occurred to me as well. Even if we enhance check (which I'm open to), I don't think it will ever be comprehensive, unless Warehouse completely externalizes its validation. Thus, there's always a non-trivial chance that an upload will fail, and require the remediation that you described.

Many people will create a pre-release tag that will upload to Test PyPI. Then you can make changes before making a final release. This too can be automated the same way your normal release flow is. You can even see if it uploads to Test PyPI and then optionally release to PyPI as well for people to install and test from if you want.

I think this is the most robust solution, though it would require re-working your release automation. I think you could even upload a pre-release to PyPI.

di commented 1 year ago

By contrast, IIRC the README validation in twine check is behavior that doesn't exist in Warehouse.

Unless it's changed significantly since it was implemented, the README validation is the same as PyPI does on upload.

I would much prefer a twine check failing during a test run in the CI.

I agree, and this is precisely the use case for twine check: to ensure a given project could be uploaded to PyPI successfully at any given time.

There is far too much warehouse checks that we cannot and every inch we move towards doing that, is more maintenance burden and momentum we have to resist reimplementing (poorly) those checks. (Especially since Warehouse doesn't externalize those often or ever)

The current plan is to externalize them into a reusable library, it just hasn't happened yet: https://github.com/pypa/packaging/issues/147

browniebroke commented 1 month ago

I've just been bitten by this issue. As the conversation seems to indicate that it would be a welcome addition, I went ahead with: https://github.com/pypa/twine/pull/1166