conchoecia / pauvre

Pauvre: QC and genome browser plotting Oxford Nanopore and PacBio long reads.
52 stars 12 forks source link

Problems in setup.py #40

Closed merwok closed 4 years ago

merwok commented 4 years ago

Hello! Heard about this project on a Debian mailing list and I noticed issues in setup.py:

  1. The requirements for scikit-learn is scikit-learn, not sklearn (that’s the importable module name) — this is the original bug I saw fixed in the Debian package

  2. requires is deprecated and de facto useless (its job is done by install_requires for dependencies and python_requires for the Python version itself)

  3. adding scripts to packages is wrong and may not get the things installed at the right location or at all; for this case (scripts/test.sh) I think no packaging is needed, assuming your CI clones the whole repo and can run the file directly; the file is not useful for end-users.

For reference, entry_points is a good mechanism for Python scripts, scripts is another param useful for non-Python executables, package_data is for data files that the code needs to access at runtime, the dreaded MANIFEST.in file is used to get extra files in sdists that don’t get installed (like a tox.ini or doc file). Note that data_files is under-specified and best left to platform-specific packaging formats (i.e. trying to install things to /etc or /usr/share using setup.py is fraught with trouble).

Extra notes that you may find interesting to save future trouble:

conchoecia commented 4 years ago

Thanks for your comments, @merwok! I haven't done any development on this as a package in a while (read, years) - I'm happy to consider any PRs your might have, but for now I'll put this in my queue.

merwok commented 4 years ago

PR #42 fixes the issues.

Moving info to setup.cfg or switching tools should be a project decision, I didn’t change that. You may also be interested in some other fixes in the Debian package: https://salsa.debian.org/med-team/python-pauvre/-/tree/master/debian/patches