Repository: scrapy/scrapyd

Issue:
Replace distutils.version.LooseVersion with packaging.version Version
In scrapyd/eggstorage.py we use LooseVersion from distutils https://github.com/scrapy/scrapyd/blob/b45c20547b91f34938ee574f86ba966d4b17b05c/scrapyd/eggstorage.py#L37


The distutils package is deprecated and slated for removal in Python 3.12. 

[PEP 632](https://www.python.org/dev/peps/pep-0632/) recommends using [packaging.version](https://packaging.pypa.io/en/latest/version.html). There is one problem, that LooseVersion was designed [as a tool for anarchists](https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Lib/distutils/version.py#L269) and packaging.version is not a tool for anarchists, it raises error when version is not matching [PEP 440](https://www.python.org/dev/peps/pep-0440/). I suspect this may break some projects because many projects don't use semantic versioning but just use version control hashes as version numbers. 

IMO we should just rewrite this sorting in eggstorage to keep old behavior without distutils, I don't see a problem in hashes as version numbers, so we can just keep old way, but maybe worth discussing how to do it? What community expects here? What are your practices and expectations here?

Expected output here:

- [x] patch that replaces LooseVersion with something else
- [x] unit tests that show that current behavior is preserved and does not break people projects, they can go somewhere here: https://github.com/scrapy/scrapyd/blob/b45c20547b91f34938ee574f86ba966d4b17b05c/scrapyd/tests/test_eggstorage.py
or here if we add separate utility for sorting https://github.com/scrapy/scrapyd/blob/b45c20547b91f34938ee574f86ba966d4b17b05c/scrapyd/tests/test_utils.py

Discussion:

Patch:
diff --git a/MANIFEST.in b/MANIFEST.in
index a9674f4..1d44276 100644
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -7,5 +7,6 @@ recursive-include docs *.txt
 recursive-include docs Makefile
 recursive-include scrapyd *.py
 recursive-include scrapyd *.egg
+recursive-include integration_tests *.py
 exclude .pre-commit-config.yaml
 exclude .readthedocs.yaml
diff --git a/docs/api.rst b/docs/api.rst
index 719089e..085f345 100644
--- a/docs/api.rst
+++ b/docs/api.rst
@@ -45,13 +45,13 @@ Example response::
 
     {"status": "ok", "spiders": 3}
 
-.. note:: Scrapyd uses the `distutils LooseVersion`_ to interpret the version numbers you provide.
+.. note:: Scrapyd uses the `packaging Version`_ to interpret the version numbers you provide.
 
 The latest version for a project will be used by default whenever necessary.
 
 schedule.json_ and listspiders.json_ allow you to explicitly set the desired project version.
 
-.. _distutils LooseVersion: http://epydoc.sourceforge.net/stdlib/distutils.version.LooseVersion-class.html
+.. _packaging Version: https://packaging.pypa.io/en/stable/version.html
 
 .. _scrapyd-schedule:
 
diff --git a/docs/news.rst b/docs/news.rst
index 866c4bc..1e53ed8 100644
--- a/docs/news.rst
+++ b/docs/news.rst
@@ -20,6 +20,7 @@ Removed
 Fixed
 ~~~~~
 
+- Use ``packaging.version.Version`` instead of ``distutils.LooseVersion``.
 - Print Scrapyd's version instead of Twisted's version with ``--version`` (``-v``) flag.
 - Override Scrapy's ``LOG_STDOUT`` to ``False`` to suppress logging output for listspiders.json webservice.
 
diff --git a/docs/overview.rst b/docs/overview.rst
index fc3934b..7d47658 100644
--- a/docs/overview.rst
+++ b/docs/overview.rst
@@ -12,7 +12,7 @@ spiders.
 A common (and useful) convention to use for the version name is the revision
 number of the version control tool you're using to track your Scrapy project
 code. For example: ``r23``. The versions are not compared alphabetically but
-using a smarter algorithm (the same `distutils`_ uses) so ``r10`` compares
+using a smarter algorithm (the same `packaging`_ uses) so ``r10`` compares
 greater to ``r9``, for example.
 
 How Scrapyd works
@@ -68,7 +68,7 @@ and accessing logs) which can be accessed at http://localhost:6800/
 
 Alternatively, you can use `ScrapydWeb`_ to manage your Scrapyd cluster.
 
-.. _distutils: http://docs.python.org/library/distutils.html
+.. _packaging: https://pypi.org/project/packaging/
 .. _Twisted Application Framework: http://twistedmatrix.com/documents/current/core/howto/application.html
 .. _server command: http://doc.scrapy.org/en/latest/topics/commands.html#server
 .. _ScrapydWeb: https://github.com/my8100/scrapydweb
diff --git a/scrapyd/eggstorage.py b/scrapyd/eggstorage.py
index 5ca8936..5efb67b 100644
--- a/scrapyd/eggstorage.py
+++ b/scrapyd/eggstorage.py
@@ -1,5 +1,4 @@
 import re
-from distutils.version import LooseVersion
 from glob import glob
 from os import listdir, makedirs, path, remove
 from shutil import copyfileobj, rmtree
@@ -7,6 +6,7 @@ from shutil import copyfileobj, rmtree
 from zope.interface import implementer
 
 from scrapyd.interfaces import IEggStorage
+from scrapyd.utils import sorted_versions
 
 
 @implementer(IEggStorage)
@@ -35,7 +35,7 @@ class FilesystemEggStorage(object):
         eggdir = path.join(self.basedir, project)
         versions = [path.splitext(path.basename(x))[0]
                     for x in glob("%s/*.egg" % eggdir)]
-        return sorted(versions, key=LooseVersion)
+        return sorted_versions(versions)
 
     def list_projects(self):
         projects = []
diff --git a/scrapyd/utils.py b/scrapyd/utils.py
index 6baff61..bbfd0f5 100644
--- a/scrapyd/utils.py
+++ b/scrapyd/utils.py
@@ -3,6 +3,7 @@ import os
 import sys
 from subprocess import PIPE, Popen
 
+from packaging.version import InvalidVersion, Version
 from scrapy.utils.misc import load_object
 from twisted.web import resource
 
@@ -156,3 +157,10 @@ def _to_native_str(text, encoding='utf-8', errors='strict'):
                         'object, got %s' % type(text).__name__)
 
     return text.decode(encoding, errors)
+
+
+def sorted_versions(versions):
+    try:
+        return sorted(versions, key=Version)
+    except InvalidVersion:
+        return sorted(versions)
diff --git a/setup.py b/setup.py
index 40237a0..fcf0742 100644
--- a/setup.py
+++ b/setup.py
@@ -23,6 +23,7 @@ setup(
     # The scrapyd command requires the txapp.py to be decompressed. #49
     zip_safe=False,
     install_requires=[
+        'packaging',
         'twisted>=17.9',
         'scrapy>=2.0.0',
         'setuptools',


Test patch:
diff --git a/scrapyd/tests/test_dont_load_settings.py b/scrapyd/tests/test_dont_load_settings.py
index 95806ff..ea40d26 100644
--- a/scrapyd/tests/test_dont_load_settings.py
+++ b/scrapyd/tests/test_dont_load_settings.py
@@ -17,8 +17,11 @@ class SettingsSafeModulesTest(unittest.TestCase):
         for m in self.SETTINGS_SAFE_MODULES:
             __import__(m)
 
-            assert 'scrapy.conf' not in sys.modules, \
+            self.assertNotIn(
+                'scrapy.conf',
+                sys.modules,
                 "Module %r must not cause the scrapy.conf module to be loaded" % m
+            )
 
 
 if __name__ == "__main__":
diff --git a/scrapyd/tests/test_eggstorage.py b/scrapyd/tests/test_eggstorage.py
index 9bbedee..4b8ba3b 100644
--- a/scrapyd/tests/test_eggstorage.py
+++ b/scrapyd/tests/test_eggstorage.py
@@ -1,4 +1,5 @@
 from io import BytesIO
+from unittest.mock import patch
 
 from twisted.trial import unittest
 from zope.interface import implementer
@@ -39,8 +40,8 @@ class TestConfigureEggStorage(unittest.TestCase):
         app = application(config)
         app_eggstorage = app.getComponent(IEggStorage)
 
-        assert isinstance(app_eggstorage, SomeFakeEggStorage)
-        assert app_eggstorage.list_projects() == ['hello_world']
+        self.assertIsInstance(app_eggstorage, SomeFakeEggStorage)
+        self.assertEqual(app_eggstorage.list_projects(), ['hello_world'])
 
 
 class EggStorageTest(unittest.TestCase):
@@ -53,6 +54,18 @@ class EggStorageTest(unittest.TestCase):
     def test_interface(self):
         verifyObject(IEggStorage, self.eggst)
 
+    @patch('scrapyd.eggstorage.glob', new=lambda x: ['ddd', 'abc', 'bcaa'])
+    def test_list_hashes(self):
+        versions = self.eggst.list('any')
+
+        self.assertEqual(versions, ['abc', 'bcaa', 'ddd'])
+
+    @patch('scrapyd.eggstorage.glob', new=lambda x: ['9', '2', '200', '3', '4'])
+    def test_list_semantic_versions(self):
+        versions = self.eggst.list('any')
+
+        self.assertEqual(versions, ['2', '3', '4', '9', '200'])
+
     def test_put_get_list_delete(self):
         self.eggst.put(BytesIO(b"egg01"), 'mybot', '01')
         self.eggst.put(BytesIO(b"egg03"), 'mybot', '03/ver')
diff --git a/scrapyd/tests/test_sqlite.py b/scrapyd/tests/test_sqlite.py
index 01faad8..c04f6dd 100644
--- a/scrapyd/tests/test_sqlite.py
+++ b/scrapyd/tests/test_sqlite.py
@@ -24,11 +24,11 @@ class JsonSqliteDictTest(unittest.TestCase):
     def test_in(self):
         d = self.dict_class()
 
-        self.assertFalse('test' in d)
+        self.assertNotIn('test', d)
 
         d['test'] = 123
 
-        self.assertTrue('test' in d)
+        self.assertIn('test', d)
 
     def test_keyerror(self):
         d = self.dict_class()
diff --git a/scrapyd/tests/test_utils.py b/scrapyd/tests/test_utils.py
index dc0b0f0..7e1805d 100644
--- a/scrapyd/tests/test_utils.py
+++ b/scrapyd/tests/test_utils.py
@@ -11,7 +11,7 @@ from twisted.trial import unittest
 
 from scrapyd import get_application
 from scrapyd.interfaces import IEggStorage
-from scrapyd.utils import UtilsCache, get_crawl_args, get_spider_list
+from scrapyd.utils import UtilsCache, get_crawl_args, get_spider_list, sorted_versions
 
 
 def get_pythonpath_scrapyd():
@@ -30,14 +30,14 @@ class UtilsTest(unittest.TestCase):
         cargs = get_crawl_args(msg)
 
         self.assertEqual(cargs, ['lala', '-a', 'arg1=val1'])
-        assert all(isinstance(x, str) for x in cargs), cargs
+        self.assertTrue(all(isinstance(x, str) for x in cargs), cargs)
 
     def test_get_crawl_args_with_settings(self):
         msg = {'_project': 'lolo', '_spider': 'lala', 'arg1': u'val1', 'settings': {'ONE': 'two'}}
         cargs = get_crawl_args(msg)
 
         self.assertEqual(cargs, ['lala', '-a', 'arg1=val1', '-s', 'ONE=two'])
-        assert all(isinstance(x, str) for x in cargs), cargs
+        self.assertTrue(all(isinstance(x, str) for x in cargs), cargs)
 
 
 class GetSpiderListTest(unittest.TestCase):
@@ -119,3 +119,12 @@ class GetSpiderListTest(unittest.TestCase):
         with mock.patch('scrapyd.utils.Popen', wraps=popen_wrapper):
             exc = self.assertRaises(RuntimeError, get_spider_list, 'mybot3', pythonpath=pypath)
         self.assertRegex(str(exc).rstrip(), r'Exception: This should break the `scrapy list` command$')
+
+
+@pytest.mark.parametrize("versions,expected", [
+    (['zzz', 'b', 'ddd', 'a', 'x'], ['a', 'b', 'ddd', 'x', 'zzz']),
+    (["10", "1", "9"], ["1", "9", "10"]),
+    (["2.11", "2.01", "2.9"], ["2.01", "2.9", "2.11"])
+])
+def test_sorted_versions(versions, expected):
+    assert sorted_versions(versions) == expected