Merge lp:~jml/pkgme/user-friendly-backend-error-972329 into lp:pkgme

[Jonathan Lange]

Allow backends to indicate that their errors are for end-user consumption.

Following the advice on bug 972329, this patch allows backends to indicate that their error is meant for end-user consumption by exiting with a pre-defined error code.

It subclasses ScriptFailed with a new exception, ScriptUserError, which essentially forwards the output from the failed script as-is.

The special return code for dispatching is defined in ScriptUserError as a class variable. I just made up the code, but it can be anything. Once we decide on it though, we're stuck with it.

This means that if, say, the binary backend indicated it had an unknown dependency using this mechanism, the error would look like::

  ERROR: Unknown dependency: "libtinfo.so.5"

I also changed the handling on the 'want' script to just re-raise this error, rather than wrapping in AssertionError.

Thanks,
jml

[James Westby (james-w)]

11 +If any of your backend scripts return a non-zero error code, pkgme will treat
12 +that as an error and will abort packaging and dump any output from the failed
13 +script in an error message to the user.

Might be worth saying something like "... because it's an exceptional circumstance that the user shouldn't see, so it's providing the most information to help you debug your backend.", and having the next paragraph explain why you may want to have user-friendly errors?

Thanks,

James

review: Approve

[lp:~jml/pkgme/user-friendly-backend-error-972329]

137. By Jonathan Lange on 2012-08-23

Rephrase docs.

=== modified file 'doc/backends/index.txt'
--- doc/backends/index.txt	2012-04-27 16:54:25 +0000
+++ doc/backends/index.txt	2012-08-23 10:43:19 +0000
@@ -171,6 +171,33 @@
 
 Keys may be omitted from the output if the information cannot be provided.
 
+Error handling
+~~~~~~~~~~~~~~
+
+Backends generally should not raise errors.  Instead, they should use their
+``want`` scripts to figure out whether they could package something
+successfully.
+
+Thus, if any of your backend scripts return a non-zero error code, pkgme will
+treat that as a serious and fatal error. It will abort packaging and dump any
+output from the failed script to the user, with an ugly error message and a
+traceback.  This is intended to provide you with the most information we can
+to help you debug your backend, and is an exceptional circumstance that users
+should never see.
+
+However, sometimes backends cannot guarantee in their ``want`` script that
+they can actually package something.  Perhaps the check would be very
+expensive, or perhaps packaging relies on some network operation that
+fails.  In these cases, the user ought to get a nice error message, rather
+than an ugly traceback.
+
+To provide the user with such an error, exit a backend script with return
+code 42.  Then the user will simply be shown ``ERROR:``, followed by any
+output printed by the script.  This gives you full control over the error
+message that the user sees, and allows you to make the message as friendly or
+as nasty as you would like.
+
+
 Making your backend available to ``pkgme``
 ------------------------------------------
 

=== modified file 'pkgme/backend.py'
--- pkgme/backend.py	2012-04-27 17:19:05 +0000
+++ pkgme/backend.py	2012-08-23 10:43:19 +0000
@@ -16,7 +16,12 @@
     MultipleExternalHelpersInfo,
     SingleExternalHelperInfo,
     )
-from pkgme.run_script import run_script, ScriptMissing, ScriptProblem
+from pkgme.run_script import (
+    run_script,
+    ScriptMissing,
+    ScriptProblem,
+    ScriptUserError,
+    )
 from pkgme import trace
 
 
@@ -193,6 +198,8 @@
     def want(self, path):
         try:
             out = run_script(self.basepath, self.WANT_SCRIPT_NAME, path)
+        except ScriptUserError:
+            raise
         except ScriptMissing:
             raise AssertionError(
                 "Backend %s (%s) has no '%s' script"

=== modified file 'pkgme/run_script.py'
--- pkgme/run_script.py	2011-11-25 01:52:28 +0000
+++ pkgme/run_script.py	2012-08-23 10:43:19 +0000
@@ -50,6 +50,21 @@
                     for line in self.output_lines if line))
 
 
+class ScriptUserError(ScriptFailed):
+    """An error from a script meant for end-user consumption."""
+
+    # Scripts exit with this return code to indicate that the error is
+    # intended for end users.
+    RETURN_CODE = 42
+
+    def __init__(self, command, output_lines):
+        super(ScriptUserError, self).__init__(
+            command, self.RETURN_CODE, output_lines)
+
+    def __str__(self):
+        return ''.join(self.output_lines)
+
+
 def run_subprocess(cmd, cwd=None, env=None, to_write=None):
     trace.debug("Running %s" % ' '.join(cmd))
     # "Python ignores SIGPIPE on startup, because it prefers to check every
@@ -80,9 +95,12 @@
             else:
                 break
         retcode = proc.wait()
-        if retcode != 0:
+        if retcode == 0:
+            return output
+        elif retcode == ScriptUserError.RETURN_CODE:
+            raise ScriptUserError(cmd, output)
+        else:
             raise ScriptFailed(cmd, retcode, output)
-        return output
     finally:
         signal.signal(signal.SIGPIPE, old_sig_pipe)
 

=== modified file 'pkgme/tests/test_backend.py'
--- pkgme/tests/test_backend.py	2012-04-27 17:19:05 +0000
+++ pkgme/tests/test_backend.py	2012-08-23 10:43:19 +0000
@@ -23,6 +23,7 @@
     MultipleExternalHelpersInfo,
     SingleExternalHelperInfo,
     )
+from pkgme.run_script import ScriptUserError
 from pkgme.testing import (
     ExecutableFileFixture,
     FileFixture,
@@ -163,6 +164,15 @@
             % (name, tempdir.path, ExternalHelpersBackend.WANT_SCRIPT_NAME),
             str(e))
 
+    def test_want_script_returns_user_error(self):
+        script = "#!/bin/sh\necho foo\nexit %s" % (
+            ScriptUserError.RETURN_CODE,)
+        tempdir = self.make_want_script(script)
+        name = self.getUniqueString()
+        backend = ExternalHelpersBackend(name, tempdir.path)
+        e = self.assertRaises(ScriptUserError, backend.want, tempdir.path)
+        self.assertEqual("foo\n", str(e))
+
     def test_want_script_returns_list(self):
         script = (
             "#!/usr/bin/python\n"

=== modified file 'pkgme/tests/test_debuild.py'
--- pkgme/tests/test_debuild.py	2012-07-27 08:06:50 +0000
+++ pkgme/tests/test_debuild.py	2012-08-23 10:43:19 +0000
@@ -20,7 +20,7 @@
 
     def setUp(self, with_binary_data=False):
         super(DebianTempDirFixture, self).setUp()
-        self.pkgdir = os.path.join(self.path, "testpkg")
+        self.pkgdir = self.abspath("testpkg")
         self.icon_path = os.path.join(self.pkgdir, "debian", "icons")
         self.debian_source_path = os.path.join(self.pkgdir, "debian", "source")
         # setup fake env
@@ -50,7 +50,7 @@
             "testpkg_0.1.orig.tar.gz", os.path.basename(result_path))
         with tarfile.open(result_path) as f:
             self.assertEqual(
-                ["testpkg-0.1", "testpkg-0.1/canary"], 
+                ["testpkg-0.1", "testpkg-0.1/canary"],
                 [m.name for m in f.getmembers()])
 
 
@@ -67,13 +67,17 @@
         bin_files = _find_binary_files_in_dir(os.path.join(
                 tempdir.pkgdir, "debian"))
         self.assertEqual(
-            ["icons/foo.png", "icons/bar.png"], bin_files)
+            sorted(["icons/foo.png", "icons/bar.png"]),
+            sorted(bin_files))
 
     def test_build_debian_source_include_binaries_content(self):
         tempdir = self.useFixture(DebianTempDirFixture())
         self._make_icons(tempdir)
         build_debian_source_include_binaries_content(tempdir.pkgdir)
-        self.assertEqual(
-            "debian/icons/foo.png\ndebian/icons/bar.png", 
-            open(os.path.join(tempdir.debian_source_path, "include-binaries")).read())
-
+        expected_binaries = sorted(
+            ['debian/icons/foo.png', 'debian/icons/bar.png'])
+        include_binaries = os.path.join(
+            tempdir.debian_source_path, "include-binaries")
+        with open(include_binaries) as f:
+            found_binaries = sorted(line.strip() for line in f.readlines())
+        self.assertEqual(expected_binaries, found_binaries)

=== modified file 'pkgme/tests/test_run_script.py'
--- pkgme/tests/test_run_script.py	2011-11-23 16:26:39 +0000
+++ pkgme/tests/test_run_script.py	2012-08-23 10:43:19 +0000
@@ -2,8 +2,10 @@
 
 from testtools import TestCase
 
-
-from pkgme.run_script import ScriptFailed
+from pkgme.run_script import (
+    ScriptFailed,
+    ScriptUserError,
+    )
 
 
 class TestErrors(TestCase):
@@ -21,3 +23,9 @@
         e = ScriptFailed(['foo', 'bar'], 1, ['line 1\n', 'line 2\n'])
         e1 = pickle.loads(pickle.dumps(e))
         self.assertEqual(str(e), str(e1))
+
+    def test_script_user_error(self):
+        e = ScriptUserError(['foo', 'bar'], ['line 1\n', 'line 2\n'])
+        self.assertEqual(
+            'line 1\n'
+            'line 2\n', str(e))