Merge lp:~jml/pkgme/reason-for-want into lp:pkgme

[Jonathan Lange]

Allow backends to provide a reason for wanting or not wanting to package a project.

Sometimes, when using pkgme on a project that you are *certain* should be packageable, pkgme says that there are no eligible backends. In those cases, it's really difficult to figure out why a backend has decided that it can't package something.

This patch allows backends to specify an optional reason in their 'want' script. It does this by allowing 'want' to output a JSON dict instead of just an integer. The JSON dict must have a 'score' key and may have a 'reason' key. The value of 'reason' is displayed in NoEligibleBackends if provided.

[lp:~jml/pkgme/reason-for-want]

119. By Jonathan Lange on 2012-04-27

Clean up XXXs that have since been fixed.

[James Westby (james-w)]

Hi,

291 + def test_want_script_with_missing_score(self):

should be ..._missing_reason I think?

Aside from that they only change I might make would be to extract a
method for running a python script with a given want response as
that is repeated in a few tests. I'm happy for it to land without
that change though.

Thanks,

James

review: Approve

[lp:~jml/pkgme/reason-for-want]

120. By Jonathan Lange on 2012-04-27

Handle empty want scripts too.

=== modified file 'doc/backends/index.txt'
--- doc/backends/index.txt	2011-11-16 16:28:02 +0000
+++ doc/backends/index.txt	2012-04-27 17:22:19 +0000
@@ -46,7 +46,7 @@
 the generic Rails backend when appropriate.
 
 Assigning all the scores up-front isn't a good idea though, so we use the following
-guildelines:
+guidelines:
 
   * 0 - no information can be provided about the project (e.g. a Ruby backend with a Python project).
   * 10 - some information can be provided, but the backend is generic (e.g. Ruby backend).
@@ -106,6 +106,13 @@
 current directory the root of the project, and should return a number on stdout that is
 how well the backend will work for the project (see `Selecting a backend`_).
 
+The ``want`` script can also return a JSON array instead of a number.  This
+array **must** have a ``score`` key with a number that is how well the backend
+will work for the project.  It may also have a ``reason`` key with a text
+value that explains why this score was given.  This reason will be shown when
+``pkgme`` cannot find any eligible backends for a project.  Ideally, such
+reasons are short and have no formatting.
+
 After implementing that script you have a choice of how to provide the rest of the
 information.
 

=== modified file 'pkgme/backend.py'
--- pkgme/backend.py	2012-02-01 18:04:19 +0000
+++ pkgme/backend.py	2012-04-27 17:22:19 +0000
@@ -1,3 +1,4 @@
+import json
 import os
 import sys
 
@@ -120,12 +121,31 @@
         return self.name
 
     def want(self, path):
+        """How much this backend wants to be *the* backend.
+
+        :return: (score, reason). 'reason' can be None for older backends.
+        """
         raise NotImplementedError(self.want)
 
     def get_info(self, path):
         raise NotImplementedError(self.get_info)
 
 
+class WantError(Exception):
+
+    def __init__(self, name, basepath, error, data=None):
+        self.name = name
+        self.basepath = basepath
+        self.error = error
+        self.data = data
+        message = (
+            "Backend %s (%s) %s from '%s' script"
+            % (name, basepath, error, ExternalHelpersBackend.WANT_SCRIPT_NAME))
+        if data is not None:
+            message = "%s: %r" % (message, data)
+        super(WantError, self).__init__(message)
+
+
 class ExternalHelpersBackend(Backend):
 
     WANT_SCRIPT_NAME = "want"
@@ -137,6 +157,39 @@
     def describe(self):
         return "%s (%s)" % (self.name, self.basepath)
 
+    def _parse_want_output(self, output):
+        result = output.strip()
+        # ints and dicts can be parsed as JSON.
+        try:
+            result = json.loads(result)
+        except ValueError:
+            raise WantError(
+                self.name, self.basepath,
+                "returned invalid score", str(result))
+        try:
+            # If it's just an int, return that with no reason.
+            return int(result), None
+        except TypeError:
+            # Do we have a score?  Is 'result' even a dict?
+            try:
+                score = result['score']
+            except KeyError:
+                raise WantError(
+                    self.name, self.basepath,
+                    "did not return a score", result)
+            except TypeError:
+                raise WantError(
+                    self.name, self.basepath,
+                    "returned invalid score", result)
+            # Is the score an integer?
+            try:
+                score = int(score)
+            except ValueError:
+                raise WantError(
+                    self.name, self.basepath,
+                    "returned non-integer score", result['score'])
+            return score, result.get('reason', None)
+
     def want(self, path):
         try:
             out = run_script(self.basepath, self.WANT_SCRIPT_NAME, path)
@@ -149,16 +202,7 @@
                 "Backend %s (%s) has no usable '%s' script: %s"
                 % (self.name, self.basepath, self.WANT_SCRIPT_NAME,
                     str(e)))
-        result = out.strip()
-        try:
-            score = int(result)
-        except ValueError:
-            raise AssertionError(
-                "Backend %s (%s) returned non-integer score from "
-                "'%s' script: %s"
-                % (self.name, self.basepath,
-                    self.WANT_SCRIPT_NAME, str(result)))
-        return score
+        return self._parse_want_output(out)
 
     def get_info(self, path):
         cls = MultipleExternalHelpersInfo
@@ -171,7 +215,7 @@
 
 class StaticBackend(Backend):
 
-    def __init__(self, name, score, info, expected_path=None):
+    def __init__(self, name, score, info, expected_path=None, reason=None):
         """Create a StaticBackend.
 
         :param name: The name of the backend.
@@ -179,20 +223,22 @@
             call. (But see expected_path).
         :param info: A fixed dict of packaging information always returned in
             response to a 'want' call. (But see expected_path).
-        :expected_path: If provided, then only return 'score' and 'info' if
-            the backend is being called for this path.
+        :param expected_path: If provided, then only return 'score' and
+            'info' if the backend is being called for this path.
+        :param reason: An optional reason for the score we're giving.
         """
         super(StaticBackend, self).__init__(name)
         self.score = score
         self.info = info
         self.expected_path = expected_path
+        self.reason = reason
 
     def want(self, path):
         if self.expected_path is not None:
             assert self.expected_path == path, (
                 "Called with cwd of %s, expected %s"
                 % (path, self.expected_path))
-        return self.score
+        return self.score, self.reason
 
     def get_info(self, path):
         if self.expected_path is not None:
@@ -216,7 +262,14 @@
 
     def __init__(self, path, tried_backends,
             disallowed_backends=None):
-        backend_names = sorted([b.name for b in tried_backends])
+        backend_names = []
+        for backend, reason in tried_backends:
+            if reason:
+                name = '%s (%s)' % (backend.name, reason)
+            else:
+                name = backend.name
+            backend_names.append(name)
+        backend_names = sorted(backend_names)
         msg = ("No eligible backends for %s. Tried %s"
                 % (path, ', '.join(backend_names)))
         if disallowed_backends is not None:
@@ -270,15 +323,18 @@
             raise NoBackend()
         disallowed, backends = self._split_disallowed_backends(self.backends)
         eligible = []
+        ineligible = []
         for backend in backends:
-            score = backend.want(path)
+            score, reason = backend.want(path)
             trace.debug('  %s scored %s' % (backend.name, score))
             if score > 0:
                 eligible.append((score, backend))
+            else:
+                ineligible.append((backend, reason))
         if not eligible:
             # FIXME: some info about the problem. Bug 809423.
             raise NoEligibleBackend(
-                path, backends,
+                path, ineligible,
                 disallowed_backends=disallowed)
         return sorted(
             eligible, key=lambda (score, backend): (-score, backend.name))

=== modified file 'pkgme/backends/dummy/want'
--- pkgme/backends/dummy/want	2011-12-07 21:03:08 +0000
+++ pkgme/backends/dummy/want	2012-04-27 17:22:19 +0000
@@ -3,5 +3,5 @@
 if [ -e is_pkgme_test ]; then
   echo 100
 else
-  echo 0
+  echo '{"score": 0, "reason": "\"is_pkgme_test\" does not exist"}'
 fi

=== modified file 'pkgme/backends/python/want'
--- pkgme/backends/python/want	2011-11-25 01:50:29 +0000
+++ pkgme/backends/python/want	2012-04-27 17:22:19 +0000
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 
+import json
 import os
 
 
@@ -7,4 +8,4 @@
     if os.path.isfile("setup.py"):
         print 20
     else:
-        print 0
+        print json.dumps({'score': 0, 'reason': "Couldn't find a setup.py"})

=== modified file 'pkgme/tests/test_backend.py'
--- pkgme/tests/test_backend.py	2012-02-01 14:38:48 +0000
+++ pkgme/tests/test_backend.py	2012-04-27 17:22:19 +0000
@@ -17,6 +17,7 @@
     NoEligibleBackend,
     StaticBackend,
     StaticLoader,
+    WantError,
     )
 from pkgme.project_info import (
     MultipleExternalHelpersInfo,
@@ -61,7 +62,71 @@
         script = "#!/bin/sh\necho 10\n"
         tempdir = self.make_want_script(script)
         backend = ExternalHelpersBackend(self.getUniqueString(), tempdir.path)
-        self.assertEqual(10, backend.want(tempdir.path))
+        self.assertEqual((10, None), backend.want(tempdir.path))
+
+    def test_empty_want_script(self):
+        script = "#!/bin/sh\n"
+        tempdir = self.make_want_script(script)
+        backend = ExternalHelpersBackend(self.getUniqueString(), tempdir.path)
+        e = self.assertRaises(WantError, backend.want, tempdir.path)
+        self.assertEqual(
+            "Backend %s (%s) returned invalid score from 'want' script: ''"
+            % (backend.name, tempdir.path),
+            str(e))
+
+    def test_want_script_with_reason(self):
+        reason = self.getUniqueString()
+        script = (
+            "#!/usr/bin/python\n"
+            "import json\n"
+            "print json.dumps({'score': 10, 'reason': %r})"
+            % (reason,))
+        tempdir = self.make_want_script(script)
+        backend = ExternalHelpersBackend(self.getUniqueString(), tempdir.path)
+        self.assertEqual((10, reason), backend.want(tempdir.path))
+
+    def test_want_script_with_reason_and_bad_score(self):
+        reason = self.getUniqueString()
+        script = (
+            "#!/usr/bin/python\n"
+            "import json\n"
+            "print json.dumps({'score': 'foo', 'reason': '%s'})"
+            % (reason,))
+        name = self.getUniqueString()
+        tempdir = self.make_want_script(script)
+        backend = ExternalHelpersBackend(name, tempdir.path)
+        e = self.assertRaises(WantError, backend.want, tempdir.path)
+        self.assertEqual(
+            "Backend %s (%s) returned non-integer score from '%s' "
+            "script: u'foo'"
+            % (name, tempdir.path, ExternalHelpersBackend.WANT_SCRIPT_NAME),
+            str(e))
+
+    def test_want_script_with_no_score(self):
+        reason = self.getUniqueString()
+        script = (
+            "#!/usr/bin/python\n"
+            "import json\n"
+            "print json.dumps({'reason': '%s'})"
+            % (reason,))
+        name = self.getUniqueString()
+        tempdir = self.make_want_script(script)
+        backend = ExternalHelpersBackend(name, tempdir.path)
+        e = self.assertRaises(WantError, backend.want, tempdir.path)
+        self.assertEqual(
+            "Backend %s (%s) did not return a score from '%s' script: %s"
+            % (name, tempdir.path, ExternalHelpersBackend.WANT_SCRIPT_NAME,
+               "{u'reason': u'%s'}" % (reason,)),
+            str(e))
+
+    def test_want_script_with_missing_reason(self):
+        script = (
+            "#!/usr/bin/python\n"
+            "import json\n"
+            "print json.dumps({'score': 10})")
+        tempdir = self.make_want_script(script)
+        backend = ExternalHelpersBackend(self.getUniqueString(), tempdir.path)
+        self.assertEqual((10, None), backend.want(tempdir.path))
 
     def test_missing_want_script(self):
         tempdir = self.useFixture(TempdirFixture())
@@ -92,12 +157,27 @@
         tempdir = self.make_want_script(script)
         name = self.getUniqueString()
         backend = ExternalHelpersBackend(name, tempdir.path)
-        e = self.assertRaises(AssertionError, backend.want, tempdir.path)
+        e = self.assertRaises(WantError, backend.want, tempdir.path)
         self.assertEqual(
-            "Backend %s (%s) returned non-integer score from '%s' script: foo"
+            "Backend %s (%s) returned invalid score from '%s' script: 'foo'"
             % (name, tempdir.path, ExternalHelpersBackend.WANT_SCRIPT_NAME),
             str(e))
 
+    def test_want_script_returns_list(self):
+        script = (
+            "#!/usr/bin/python\n"
+            "import json\n"
+            "print json.dumps([1, 2, 3])")
+        tempdir = self.make_want_script(script)
+        name = self.getUniqueString()
+        backend = ExternalHelpersBackend(name, tempdir.path)
+        e = self.assertRaises(WantError, backend.want, tempdir.path)
+        self.assertEqual(
+            ("Backend %s (%s) returned invalid score "
+             "from '%s' script: [1, 2, 3]"
+             % (name, tempdir.path, ExternalHelpersBackend.WANT_SCRIPT_NAME)),
+            str(e))
+
     def test_want_script_run_in_correct_dir(self):
         cwd_tempdir = self.useFixture(TempdirFixture())
         script = """#!%s
@@ -110,7 +190,7 @@
         backend_tempdir = self.make_want_script(script)
         backend = ExternalHelpersBackend(
             self.getUniqueString(), backend_tempdir.path)
-        self.assertEqual(10, backend.want(cwd_tempdir.path))
+        self.assertEqual((10, None), backend.want(cwd_tempdir.path))
 
     def test_get_info_with_multiple(self):
         helpers_path = self.getUniqueString()
@@ -165,6 +245,21 @@
         selector = BackendSelector(backends)
         self.assertRaises(NoEligibleBackend, selector.get_info, path)
 
+    def test_reasons_for_ineligbility(self):
+        backend1 = StaticBackend('no-reason', 0, None)
+        backend2 = StaticBackend(
+            'a', 0, None, reason=self.getUniqueString())
+        backend3 = StaticBackend(
+            'b', 0, None, reason=self.getUniqueString())
+        backends = [backend1, backend2, backend3]
+        selector = BackendSelector(backends)
+        e = self.assertRaises(
+            NoEligibleBackend, selector.get_info, '/path')
+        self.assertEqual(
+            'No eligible backends for /path. Tried a (%s), b (%s), '
+            'no-reason' % (backend2.reason, backend3.reason),
+            str(e))
+
     def test_selects_first_name_in_tie(self):
         info1 = self.getUniqueString()
         info2 = self.getUniqueString()

=== modified file 'pkgme/tests/test_python_backend.py'
--- pkgme/tests/test_python_backend.py	2011-11-25 01:52:28 +0000
+++ pkgme/tests/test_python_backend.py	2012-04-27 17:22:19 +0000
@@ -1,5 +1,3 @@
-import os
-
 from fixtures import TestWithFixtures
 from testtools import TestCase
 
@@ -16,12 +14,13 @@
         tempdir = self.useFixture(TempdirFixture())
         tempdir.touch('setup.py')
         backend = ExternalHelpersBackend("python", backend_dir)
-        self.assertEqual(20, backend.want(tempdir.path))
+        self.assertEqual((20, None), backend.want(tempdir.path))
 
     def test_want_without_setup_py(self):
         tempdir = self.useFixture(TempdirFixture())
         backend = ExternalHelpersBackend("python", backend_dir)
-        self.assertEqual(0, backend.want(tempdir.path))
+        self.assertEqual(
+            (0, "Couldn't find a setup.py"), backend.want(tempdir.path))
 
     def test_all_info(self):
         tempdir = self.useFixture(TempdirFixture())

=== modified file 'pkgme/tests/test_vala_backend.py'
--- pkgme/tests/test_vala_backend.py	2011-07-15 09:22:47 +0000
+++ pkgme/tests/test_vala_backend.py	2012-04-27 17:22:19 +0000
@@ -47,31 +47,31 @@
         self.tempdir.create_file("configure.ac", "")
         self.tempdir.create_file("main.vala", "")
         backend = self.get_backend()
-        self.assertEqual(20, backend.want(self.tempdir.path))
+        self.assertEqual((20, None), backend.want(self.tempdir.path))
 
     def test_want_one_deep(self):
         self.tempdir.create_file("configure.ac", "")
         self.tempdir.create_file(os.path.join("one", "main.vala"), "")
         backend = self.get_backend()
-        self.assertEqual(20, backend.want(self.tempdir.path))
+        self.assertEqual((20, None), backend.want(self.tempdir.path))
 
     def test_want_two_deep(self):
         self.tempdir.create_file("configure.ac", "")
         self.tempdir.create_file(
             os.path.join("one", "two", "main.vala"), "")
         backend = self.get_backend()
-        self.assertEqual(0, backend.want(self.tempdir.path))
+        self.assertEqual((0, None), backend.want(self.tempdir.path))
 
     def test_want_without_configure_ac(self):
         self.tempdir.create_file("main.vala", "")
         backend = self.get_backend()
-        self.assertEqual(0, backend.want(self.tempdir.path))
+        self.assertEqual((0, None), backend.want(self.tempdir.path))
 
     def test_want_with_configure_in(self):
         self.tempdir.create_file("configure.in", "")
         self.tempdir.create_file("main.vala", "")
         backend = self.get_backend()
-        self.assertEqual(20, backend.want(self.tempdir.path))
+        self.assertEqual((20, None), backend.want(self.tempdir.path))
 
     def test_architecture(self):
         info = self.get_info()