regebro / pyroma

Rate your Python packages package friendliness
MIT License
213 stars 21 forks source link

Broken with current setuptools version? #71

Closed icemac closed 2 years ago

icemac commented 2 years ago

I installed pyroma using pipx for Python 3.9 and then tried to use it on zopefoundation/BTrees:

$ pipx install pyroma
$ cd BTrees
$ pyroma .
------------------------------
Checking .
/.../python3.9/site-packages/_distutils_hack/__init__.py:17: UserWarning: Distutils was imported before Setuptools, but importing Setuptools also replaces the `distutils` module in `sys.modules`. This may lead to undesirable behaviors or errors. To avoid these issues, avoid using distutils directly, ensure that setuptools is installed in the traditional way (e.g. not an editable install), and/or make sure that setuptools is always imported before distutils.
  warnings.warn(
/.../python3.9/site-packages/_distutils_hack/__init__.py:30: UserWarning: Setuptools is replacing distutils.
  warnings.warn("Setuptools is replacing distutils.")
/.../python3.9/site-packages/setuptools/installer.py:27: SetuptoolsDeprecationWarning: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer.
  warnings.warn(
invalid command name '.'
$ echo $?
1

Trying to use a package name from PyPI leads to:

$ pyroma BTrees
------------------------------
Checking BTrees
Starting new HTTPS connection (1): pypi.org:443
https://pypi.org:443 "GET /pypi/BTrees/json HTTP/1.1" 200 109675
Found BTrees version 4.10.0
Downloading BTrees-4.10.0.tar.gz to verify distribution
Starting new HTTPS connection (1): files.pythonhosted.org:443
https://files.pythonhosted.org:443 "GET /packages/7b/a5/38cb37e0afae6b00392062b78364033354a34c1f73c879e5eeaabf57f709/BTrees-4.10.0.tar.gz HTTP/1.1" 200 195139
/.../python3.9/site-packages/_distutils_hack/__init__.py:17: UserWarning: Distutils was imported before Setuptools, but importing Setuptools also replaces the `distutils` module in `sys.modules`. This may lead to undesirable behaviors or errors. To avoid these issues, avoid using distutils directly, ensure that setuptools is installed in the traditional way (e.g. not an editable install), and/or make sure that setuptools is always imported before distutils.
  warnings.warn(
/.../python3.9/site-packages/_distutils_hack/__init__.py:30: UserWarning: Setuptools is replacing distutils.
  warnings.warn("Setuptools is replacing distutils.")
/.../python3.9/site-packages/setuptools/installer.py:27: SetuptoolsDeprecationWarning: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer.
  warnings.warn(
usage: setup.py [global_opts] cmd1 [cmd1_opts] [cmd2 [cmd2_opts] ...]
   or: setup.py --help [cmd1 cmd2 ...]
   or: setup.py --help-commands
   or: setup.py cmd --help

error: invalid command 'BTrees'
$ echo $?
1

Installed packages in the venv:

$ bin/python -m pip freeze --all
certifi==2021.10.8
charset-normalizer==2.0.12
docutils==0.18.1
idna==3.3
pip==22.0.4
Pygments==2.11.2
pyroma==3.2
requests==2.27.1
setuptools==60.9.3
urllib3==1.26.8
wheel==0.37.1
CAM-Gerlach commented 2 years ago

As I understand it, Pyroma was developed back in the days before PEP 517 and most modern packaging standards, and there's been a lot of change to keep up with since then. In particular, distutils is deprecated, PEP 517 has been widely implemented and is started to be adopted in earnest, direct invocation of setup.py is deprecated, and so are some of the other implementation details from back in the day that Pyroma relied on. Really, the overall approach needs some changes to reflect this or things will keep breaking; at some point, I hope to help revamp it to reflect the modern state of Python packaging and take advantage of the new tools (e.g. importlib_metadata) and APIs (PEP 517) we have available now, but like anything its a matter of finding the time to do so.

regebro commented 2 years ago

I just ran it with setuptools 60.10, so I don't think that's the problem.

Implementing support for PEP517 would be nice, but I have no clue when I will have time for that.

CAM-Gerlach commented 2 years ago

I can also confirm, which I should have before, that I mostly cannot reproduce this with either pyroma . and pipx run pyroma . (in implied -d mode), on both passing and failing packages, on Python 3.9 and Setuptools 60.9.3, at least on Windows. pyroma -f also worked on passing and failing sdists, though wheels are not (yet) supported. Running pyroma BTrees doesn't give me the error above, but neither does it give me any useful Pyroma results.

Pyroma output ```console $ pyroma BTrees ------------------------------ Checking BTrees Starting new HTTPS connection (1): pypi.org:443 https://pypi.org:443 "GET /pypi/BTrees/json HTTP/1.1" 200 119509 Found BTrees version 4.10.0 Downloading BTrees-4.10.0.tar.gz to verify distribution Starting new HTTPS connection (1): files.pythonhosted.org:443 https://files.pythonhosted.org:443 "GET /packages/7b/a5/38cb37e0afae6b00392062b78364033354a34c1f73c879e5eeaabf57f709/BTrees-4.10.0.tar.gz HTTP/1.1" 200 195139 C:\Miniconda3\envs\qtpy-env\lib\site-packages\setuptools\installer.py:27: SetuptoolsDeprecationWarning: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer. warnings.warn( $ echo $? 0 ```

This issue seems to be something to do with something specific to the package, as pyroma setuptools gives the expected output.

Pyroma output ```console $ pyroma setuptools ------------------------------ Checking setuptools Starting new HTTPS connection (1): pypi.org:443 https://pypi.org:443 "GET /pypi/setuptools/json HTTP/1.1" 200 172583 Found setuptools version 60.10.0 Downloading setuptools-60.10.0.tar.gz to verify distribution Starting new HTTPS connection (1): files.pythonhosted.org:443 https://files.pythonhosted.org:443 "GET /packages/af/e8/894c71e914dfbe01276a42dfad40025cd96119f2eefc39c554b6e8b9df86/setuptools-60.10.0.tar.gz HTTP/1.1" 200 2420706 Found setuptools ------------------------------ The classifiers should specify what minor versions of Python you support as well as what major version. You should have three or more owners of the project on PyPI. ------------------------------ Final rating: 9/10 Cottage Cheese ------------------------------ ```

I'd have to investigate the project in question's packaging configuration to guess at what that might be.

regebro commented 2 years ago

OK, so this happens when running Pyroma against a package on PyPI that raises this warning. It means the end result isn't printed out. It's enough of a edge case that I'm not going to fix it now. It will get fixed when/if I get proper PEP 517 support and don't have to mess around with setup() any more.

I'm keeping it open so I don't forget to test.

CAM-Gerlach commented 2 years ago

Seems reasonable, since its an indication of an actual problem with the project.

As to support for the modern Python packaging ecosystem, adding support to Pyroma should be a lot simpler than I initially thought, and greatly simplify the codebase as a whole (since the old hacks, monkeypatching and need to actually build the project are no longer needed, given the approach below works equally with both legacy and PEP 517 projects, and regardless of build system), as well as being generally more efficient. The entirety of pyroma/projectdata.py can be replaced with just:

import build, email, pathlib, tempfile

def get_data(path):
    with tempfile.TemporaryDirectory() as tempdir:
        metadata_dir = build.ProjectBuilder(str(path)).prepare("wheel", tempdir)
        with open(pathlib.Path(metadata_dir) / "METADATA", "rb") as metadata_file:
            metadata = email.message_from_binary_file(metadata_file)
    return metadata

(Of course, you'll need to add build to your dependencies.) Since metadata is a dict-like object supports indexing and get, it should be usable with the existing ratings.py with no other changes.

Likewise, pyroma/distributiondata.py can be updated to support wheels alongside sdists, which looks like:

import email, pathlib, shutil, tarfile, tempfile, zipfile

from pyroma import projectdata

def get_data(path):
    path = pathlib.Path(path).resolve()
    with tempfile.TemporaryDirectory() as tempdir:
        tempdir = pathlib.Path(tempdir)
        if path.suffix == ".whl":
            with zipfile.ZipFile(path, "r") as wheel_file:
                wheel_file.extractall(tempdir)
            metadata_path = list(tempdir.glob("*.dist-info/METADATA"))[0]
            with open(metadata_path, "rb") as metadata_file:
                data = email.message_from_binary_file(metadata_file)
        elif ".tar" in path.name:
            with tarfile.open(name=path, mode="r:*") as tar_file:
                tar_file.extractall(tempdir)
            data = projectdata.get_data(list(tempdir.glob("*"))[0])
        else:
            raise ValueError(f"Unknown file type: {path.suffix}")
    return data

That should be all that is needed in terms of changes; everything else should work as it does now, as the above code preserves API-compatible inputs and outputs for both get_data() functions. If you want me to propose that in a PR, I can.

Also, to take advantage of modern, standard tools and further reduce the amount of bepoke code and maintenance effort:

In total, this will add three direct dependencies (build, packaging and twine), while also removing three (docutils, setuptools and pygments).

regebro commented 2 years ago

Oh, nice. I didn't know about build. That indeed makes it a lot easier, thanks! I'll probably keep the old code as a deprecated backup for a while.

regebro commented 2 years ago

OK, 4.0b1 solves this issue. I'm keeping this issue open as there are discussions about PEP621 support in here.

CAM-Gerlach commented 2 years ago

I'm not sure I see anything directly related to reading metadata from a PEP 621 [project] table in here, and that's more of an opportunistic optimization for certain cases rather than providing any new functionality. However, I do mention some other steps to take, and the need to update distributiondata.py accordingly. I'm happy to create separate issues for each of those, if you'd like.

regebro commented 2 years ago

I'll create a future plans issue, and you can put it there.

regebro commented 2 years ago

OK, I opened #75 .

icemac commented 2 years ago

Thank you for fixing this issue. I can confirm that both variants I had problems with are fixed by 4.0b1.