pyroma icon indicating copy to clipboard operation
pyroma copied to clipboard

Check `python_requires` for supported python version (PEP 440)

Open davidandreoletti opened this issue 2 years ago • 8 comments

EDIT: Running pyroma v4.0 with default pyroma settings on a package described below using setuptools, pyroma reports no python version was specified.

It expects to find a python version in the classifiers field (setup(..., classifiers=[SOME PYTHON VERSION(S) HERE])).

There is 2 places to define supported python versions and for now pyroma only support the classifiers one.

setup(
    ....
    python_requires=">=3.8",
    ....
)

davidandreoletti avatar Aug 15 '22 16:08 davidandreoletti

It is rather unclear from the very limited information provided what you are actually asking for. That the Requires-Python field of core metadata (which the bespoke tool-specific configuration above maps to) contains a set of one or more valid PEP 440 Version Specifiers? Or something more specific?

AFAIK, most build backends and certainly setuptools (which you are evidently using) already error out on package build when parsing the metadata if Requires-Python contains invalid PEP 440 specifiers, leaving (in the simple case) the presence of this field with some value the only simple thing to check for, which could be done with just something in ratings.py like:

class RequiresPython(FieldTest):
    weight = 100
    field = "requires_python"

However, if you don't want to rely on build backends erroring on this, this could be accomplished fairly simply by adding a new BaseTest subclass in ratings.py with a test method like

def test(self, data):
    requires_python = data.get("requires_python")
    if not requires_python:
        return False
    try:
        packaging.specifiers.SpecifierSet(requires_python)
    except packaging.specifiers.InvalidSpecifier:
        return False
    return True

CAM-Gerlach avatar Aug 16 '22 03:08 CAM-Gerlach

@CAM-Gerlach Thank you for the prompt response and apologies for the limited issue description.

Running pyroma v4.0 with default pyroma settings on a package described using setuptools as above, pyroma reports no python version was specified.

It expects to find a python version in the classifiers field (setup(..., classifiers=[SOME PYTHON VERSION(S) HERE])).

There is 2 places to define supported python versions and for now pyroma only support the classifiers one. Hopefully this clarifies the scope.

The second proposed implementation is more suitable at catching invalid python version. Assuming this get implemented, probably best to report the "python version" test as succeeded if and only if a valid python version is found in the python_requires or classifiers unless this contradicts some PEP requirement.

davidandreoletti avatar Aug 16 '22 08:08 davidandreoletti

Running pyroma v4.0 with default pyroma settings on a package described using setuptools as above, pyroma reports no python version was specified.

Ah, it seems the message should be fixed, then, to properly describe the issue—that the package is missing the relevant Python version Trove Classifiers, because the actual way to directly specify compatible Python version is with the Requires-Python metadata field; classifiers are merely advisory. Additionally, the missing Python version classifier test weight should probably reduced to something more like 50, since it isn't that critical compared to Requires-Python.

There is 2 places to define supported python versions and for now pyroma only support the classifiers one. Hopefully this clarifies the scope.

Yes, it does, thanks—I appreciate the detailed followup.

Assuming this get implemented, probably best to report the "python version" test as succeeded if and only if a valid python version is found in the python_requires or classifiers unless this contradicts some PEP requirement.

The two metadata fields are independent and serve mostly orthogonal purposes, so both should be specified.

Requires-Python is the more important, as it is the standard, programmatically-readable way of precisely specifying the Python versions your package is compatible with, and is what is actually used by integration frontends (install tools, e.g. pip) to select a compatible version of your package to install. If it is missing or incorrect, things could easily break, as your package can be installed on Python versions it is not actually compatible with, or refuse to install on those it is.

The Python version trove classifiers are less critical, but still serve several uniquely valuable purposes—unlike Requires-Python which is a hard constraint, they serve in a softer Advisory capacity to indicate which Python versions a project is actually tested and officially supported (particularly given that upper caps on Requires-Python are generally not recommended). Furthermore, they allow users on PyPI to browse and filter projects by compatible version, are often easier for users to read at a glance than Requires-Python, and are also used by advisory tooling to track and report progress on officially supporting new Python versions and dropping old ones.

Also, as a sidenote, unlike with classifiers, there isn't a generalizable way to test if a set of PEP 440 version specifiers specifies a set of valid "Python versions", defined in some strict way—rather, you can only reliably check that they are a set of one or more valid PEP 440 specifiers.

CAM-Gerlach avatar Aug 16 '22 10:08 CAM-Gerlach

the message should be fixed, then, to properly describe the issue

Updated. Apologies again.

there isn't a generalizable way to test if a set of PEP 440 version specifiers specifies a set of valid "Python versions", defined in some strict way ...

IMHO, packaging.specifiers.SpecifierSet(requires_python) is a suitable starting point and handles non trivial python version requirements too. Eg: <SpecifierSet('!=1.1,>=1.0,~=1.0')>

davidandreoletti avatar Aug 17 '22 05:08 davidandreoletti

Updated. Apologies again.

No, my apologies :) —I meant Pyroma's error message describing the metadata issue, not your message describing the Pyroma issue. Sorry for the confusion!

IMHO, packaging.specifiers.SpecifierSet(requires_python) is a suitable starting point and handles non trivial python version requirements too. Eg: <SpecifierSet('!=1.1,>=1.0,~=1.0')>

Yep, that's the intention—its also the reference implementation of the standard, and is what build tools will error on too. Would you like to submit a PR based on the suggestion above (assuming @regebro doesn't object)?

As a sidenote, the existing rather lengthy bespoke logic can be updated to just use packaging too—the latter probably didn't exist, or was in its infancy, when it was originally written. I'd meant to do so, but hadn't found the time. Out of the direct scope here, of course, but I figured I'd mention it.

CAM-Gerlach avatar Aug 17 '22 23:08 CAM-Gerlach

Would you like to submit a PR based on the suggestion above ?

@CAM-Gerlach Yes. Waiting on @regebro's green light.

davidandreoletti avatar Aug 18 '22 13:08 davidandreoletti

Yes, that seems reasonable, it should check for both. I'm surprised I missed python_requires, and that no one noticed until now. :-)

I have no strong opinion on the weights, but I would probably set them at the same weight, personally, but if you want something different that's fine too.

regebro avatar Aug 20 '22 09:08 regebro

I'm surprised I missed python_requires, and that no one noticed until now. :-)

The earliest commit I can see of that particular check is 8 years old, and from the commit message, it may be older than that. That's long before my time, but I'm not sure it was at least widely implemented by tools and adopted by users at that point, at least not like it is now.

I have no strong opinion on the weights, but I would probably set them at the same weight, personally, but if you want something different that's fine too.

I'd in turn defer to your judgement as maintainer; its ultimately subjective anyway. My thinking is that since Requires-Python is critical for tools to avoid installing distribution versions that are broken on the user's Python, whereas the version classifiers are just advisory only, the former would seem to be more important, but it certainly helps having both.

CAM-Gerlach avatar Aug 20 '22 17:08 CAM-Gerlach