'check-manifest -p python2.7' is broken

[mgedmin]

Example:

$ check-manifest -p python2.7
could not run ['/tmp/pywt/python2.7', 'setup.py', 'sdist', '-d', '/tmp/check-manifest-4f44XR-sdist']: [Errno 2] No such file or directory

Note how it's calling os.path.abspath() on python2.7. It shouldn't do that.

[mgedmin]

The fix for #36 (87f0ccb) broke this.

[mcepl]

Still not foolproof enough. You need something like this (this is a bad workaround, skipping the test, you should probably do something to fix the test, when python just doesn't exist at all):

---
 tests.py |    4 ++++
 1 file changed, 4 insertions(+)

--- a/tests.py
+++ b/tests.py
@@ -2,6 +2,7 @@ import codecs
 import doctest
 import locale
 import os
+import shutil
 import subprocess
 import sys
 import tarfile
@@ -1368,6 +1369,9 @@ class TestCheckManifest(unittest.TestCas
         # https://github.com/mgedmin/check-manifest/issues/57
         # NB: this test assumes you have a 'python' executable somewhere
         # in your path.
+        # Don't assume, check
+        if shutil.which('python') is None:
+            raise unittest.SkipTest("No 'python' executable present.")
         from check_manifest import check_manifest
         subdir = self._create_repo_with_code_in_subdir()
         self.assertTrue(check_manifest(subdir, python='python'))

[mcepl]

Actually, this is probably even better (works for Python 2 as well):

---
 tests.py |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--- a/tests.py
+++ b/tests.py
@@ -2,6 +2,7 @@ import codecs
 import doctest
 import locale
 import os
+import os.path
 import subprocess
 import sys
 import tarfile
@@ -1368,9 +1369,11 @@ class TestCheckManifest(unittest.TestCas
         # https://github.com/mgedmin/check-manifest/issues/57
         # NB: this test assumes you have a 'python' executable somewhere
         # in your path.
+        # Don't assume, make sure
+        py_exec = os.path.basename(sys.executable)
         from check_manifest import check_manifest
         subdir = self._create_repo_with_code_in_subdir()
-        self.assertTrue(check_manifest(subdir, python='python'))
+        self.assertTrue(check_manifest(subdir, python=py_exec))

     def test_suggestions(self):
         from check_manifest import check_manifest

[mgedmin]

Still not foolproof enough

It took me a while to load all the context.

IIRC you're a distro packager, and I'm guessing you're having trouble with this assumption in the unit test, rather than with the fix itself:

        # NB: this test assumes you have a 'python' executable somewhere
        # in your path.

The suggested fix looks okay-ish:

• I don't think it's safe to assume sys.executable is guaranteed to be on $PATH, somebody could be running tests by invoking e.g. ~/op/t/python3.11alpha/bin/python tests.py. Granted, it's less likely. AFAIU tox always activates the venv, so sys.executable's dirname should be in $PATH as long as people use tox to run the tests. I'm fine with the logic, just not the phrasing of the comment that asserts certainty.

• The import os.path bit is unnecessary, importing os makes all os.path functions available.

Would you care to submit the patch in the form of a pull request?

[mcepl]

Would you care to submit the patch in the form of a pull request?

Sorry, I am maintainer of over 2000 Python-related packages in openSUSE, I really cannot maintain git checkouts for all of them.

And yes, import os.path is not strictly necessary, I just think that “Explicit is better than implicit”.

[mgedmin]

That's fine, I'll make the changes myself then.

[mgedmin]

Wait a second, I already committed a fix for this a while ago (in 2019), in d0b583d. Is that not sufficient? The diff you pasted here doesn't show that fix, so it must be from a quite old release.

[mcepl]

Right, sorry, this was apparently from SLE old package.