search

whipper-team / whipper

Python CD-DA ripper preferring accuracy over speed
GNU General Public License v3.0
1.11k stars 88 forks source link

Migrate to PEP517-compliant pyproject.toml build system #622

Open baldurmen opened 1 month ago

baldurmen commented 1 month ago

This PR migrates the build system from the old setup.py to the new PEP517-compliant pyproject.toml-based one. I stuck with setuptools, since this is what we were already using.

Note that sadly, to my knowledge, it is not currently possible to install scripts like it was before, which means that the accuraterip-checksum script has to be installed manually.

The setup.py file isn't removed either, as it is still needed to build the C extension.

Happy to change things if necessary, but AFAIK this works fine, as I'm building the Debian package with it.

Cheers,

MerlijnWajer commented 1 month ago

Thanks for the pull request, on first look, things look fine. Could you elaborate a little more on having to manually install the accuraterip-checksum program? Does that mean that we currently don't install this any more in say the debian package, or did you work around this in the debian/rules file?

baldurmen commented 1 month ago

In the old setup.py script, this bit used to install the scripts/accuraterip-checksum file in /usr/bin/accuraterip-checksum/ when running python3 setup.py install:

    scripts=[
        'scripts/accuraterip-checksum',
    ],

As I said, as far as I understand, this is not possible anymore. This means running pip install against the proposed pyproject.toml file will not install the scripts/accuraterip-checksum file in /usr/bin/accuraterip-checksum/.

For the Debian package, it is not an issue, as we're now manually installing the script. I was mostly concerned for use cases where people install whipper manually on their machines.

MerlijnWajer commented 1 month ago

Thanks for the explanation, I would feel better if we can find a way to install the script any way, we do need it for whipper to function properly, don't we? Or did we turn this into a Python extension as well?

baldurmen commented 1 month ago

You probably know the codebase better than I, but I do not believe the accuraterip-checksum script is needed for whipper to work properly. AFAIU, whipper itself loads the C extension via import accuraterip.

accuraterip-checksum mainly seems to be a "nice to have" cli tool that can give you the accuraterip checksum of individual files without having to run whipper.