Merge lp:~mvo/software-center/polkit-lp785117 into lp:software-center

[Michael Vogt]

This branch adds more error handling in the failure case.

Just like now errors like "NotAuthorized" or "NoReply" are ignored as policykit will already have displayed
something to the user. There is a new error message if the agent failed to launch, this can be tested in e.g.
a fvwm session.

Plus unknown errors are also displayed and the dialog asks for a bugreport, this should give us a idea if
there is anything else hidding here that needs attention. Matthew may want to tweak the error dialog texts,
but I think its ok to have them somewhat technically as this is a pretty rare circumstance when no policykit-agent
is available and lots of stuff will break in this case.

[Evan (ev)]

I believe that recovering from errors while at the same time reporting them is something that we're going to want to do often and consistently.

Matthew had a work item this cycle to create the interface for an application-triggered recoverable error report:
https://wiki.ubuntu.com/ErrorTracker#app-requested

I've implemented this first as a DBus service:
https://code.launchpad.net/~ev/apport/recoverable-errors/+merge/111840

There is an ongoing discussion on picking a communication channel with far fewer dependencies than DBus, but whatever the result we will have something implemented for 12.10.

Michael, I would love your input on that merge request in the hope that we can build something that addresses what you're trying to achieve here.

[Michael Vogt (mvo)]

I updated the code now to use the new apport mechanism for reporting recoverable problems.

[Michael Vogt (mvo)]

I updated the code now to use the new apport mechanism for reporting recoverable problems.

[Evan (ev)]

On Wed, Aug 22, 2012 at 8:57 AM, Michael Vogt <email address hidden> wrote:
> === modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
> --- softwarecenter/backend/installbackend_impl/aptd.py 2012-08-07 13:15:08 +0000
> +++ softwarecenter/backend/installbackend_impl/aptd.py 2012-08-22 07:56:18 +0000
> @@ -973,13 +979,48 @@
> + return
> + if name in ["org.freedesktop.PolicyKit.Error.Failed"]:
> + summary = _("Authentication Error")
> + text = _("Is policykit installed and an authentication "
> + "agent running? Error Details:\n%s") % error
> + self.ui.error(None, summary, text)
> + return

Can you clarify the rationale for creating a software-center dialog
and exiting the function here? I would have thought you'd want to have
graphs of the occurrence of the authentication agent not running, as
it may be more often than we suspect and be evidence of a larger
problem. We won't know until we measure though.

> + # show a apport recoverable error dialog to the user as we want
> + # to know about these issues
> + self._call_apport_recoverable_error(
> + _("There was a error submitting the transaction"), error)

Grammar nit: an before vowel sounds. So it's "there was an error."

> + # mvo: I don't think we need to send "Package\0software-center",
> + # apport should figure this out itself
> + data = "DialogBody\0%(text)s\0Traceback\0%(error)s" % {
> + 'text': text,
> + 'error': error,
> + }
> + # this will be quick as it just writes the report file and the
> + # helper is spawned by apport asyncronsoly

Correct, but to clarify: the report is written, update-notifier has an
inotify watch on /var/crash and it then calls apport to process unseen
reports.

> + APPORT_RECOVERABLE_ERROR = "/usr/share/apport/recoverable_problem"
> + p = Popen(
> + [APPORT_RECOVERABLE_ERROR], stdin=PIPE, stdout=PIPE, stderr=PIPE)
> +
> + (stdout, stderr) = p.communicate(input=data)
> + return_code = p.wait()

subprocess.communicate() calls wait():
Lib/subprocess.py:communicate:751

So you could just grab it off the pid member. There's no harm in
calling wait() again though, so feel free to leave that.

> === modified file 'tests/test_aptd.py'
> --- tests/test_aptd.py 2012-06-28 08:51:44 +0000
> +++ tests/test_aptd.py 2012-08-22 07:56:18 +0000
> @@ -121,5 +143,15 @@
> + @patch("softwarecenter.backend.installbackend_impl.aptd.Popen")
> + def test_recoverable_error(self, mock_popen):
> + mock_popen_instance = Mock()
> + mock_popen_instance.communicate.return_value = ("stdout", "stderr")
> + mock_popen_instance.wait.return_value = 0
> + mock_popen.return_value = mock_popen_instance
> + self.aptd._call_apport_recoverable_error("msg", "error-error")
> + mock_popen_instance.communicate.assert_called()

You could expand this to check the arguments to Popen and put a
StringIO on stdin to ensure that you're getting three nuls and the
right key-value pair...

[Matthew Paul Thomas (mpt)]

Even if you did convert the custom error to an Apport one, though...

+ text = _("Is policykit installed and an authentication "
+ "agent running? Error Details:\n%s") % error

...A big part of USC's day job is to tell whether or not things are installed. Why is it now asking me whether something is installed? :-) (Even I have trouble with this particular question, because I confuse policykit and policykit-1.) Besides, software-center Depends on policykit-1 and policykit-1-gnome, so being uninstalled is unlikely to be the problem, unless it was removed (together with software-center itself) while USC was open.

Instead I suggest something like "Software can’t be installed or removed because the authentication service is not available. (%s)" % error

[lp:~mvo/software-center/polkit-lp785117]

3032. By Michael Vogt on 2012-08-23

address the easy points of evans review

[Michael Vogt (mvo)]

On Wed, Aug 22, 2012 at 11:31:08AM -0000, Matthew Paul Thomas wrote:
> Even if you did convert the custom error to an Apport one, though...
>
> + text = _("Is policykit installed and an authentication "
> + "agent running? Error Details:\n%s") % error
>
> ...A big part of USC's day job is to tell whether or not things are installed. Why is it now asking me whether something is installed? :-) (Even I have trouble with this particular question, because I confuse policykit and policykit-1.) Besides, software-center Depends on policykit-1 and policykit-1-gnome, so being uninstalled is unlikely to be the problem, unless it was removed (together with software-center itself) while USC was open.

Indeed, this part of the text dosn't make a lot of sense, it will be
installed, but it might not be running because its running inside a
environement that does not autostart it or because there is something
wrong with the install.

> Instead I suggest something like "Software can’t be installed or removed because the authentication service is not available. (%s)" % error

Thanks, I updated the text to match what you write.

I will also make this part of what is reported via apport as a
recoverable error so that we can get data on it as Evan suggested.

Cheers,
 Michael

[lp:~mvo/software-center/polkit-lp785117]

3033. By Michael Vogt on 2012-08-23

softwarecenter/backend/installbackend_impl/aptd.py: update policykit error text to match the suggestion of mpt (thanks!)

3034. By Michael Vogt on 2012-08-23

tests/test_aptd.py: improve test (and as a consequence the code) as suggested by Evan

3035. By Michael Vogt on 2012-08-23

tests/test_aptd.py: further improve the test

3036. By Michael Vogt on 2012-08-23

softwarecenter/backend/installbackend_impl/aptd.py: send unavailable polkit agent also to apport

[Michael Vogt (mvo)]

On Wed, Aug 22, 2012 at 11:10:00AM -0000, Evan Dandrea wrote:
> On Wed, Aug 22, 2012 at 8:57 AM, Michael Vogt <email address hidden> wrote:
> > === modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
> > --- softwarecenter/backend/installbackend_impl/aptd.py 2012-08-07 13:15:08 +0000
> > +++ softwarecenter/backend/installbackend_impl/aptd.py 2012-08-22 07:56:18 +0000
> > @@ -973,13 +979,48 @@
> > + return
> > + if name in ["org.freedesktop.PolicyKit.Error.Failed"]:
> > + summary = _("Authentication Error")
> > + text = _("Is policykit installed and an authentication "
> > + "agent running? Error Details:\n%s") % error
> > + self.ui.error(None, summary, text)
> > + return
>
> Can you clarify the rationale for creating a software-center dialog
> and exiting the function here? I would have thought you'd want to have
> graphs of the occurrence of the authentication agent not running, as
> it may be more often than we suspect and be evidence of a larger
> problem. We won't know until we measure though.

You are right, it should be meassured, I now modified the code to do
that.

What is your opinion on showing the internal dialog still in addition
to fire of the report to apport? The assumption here is that the user
might not have update-notifier runnig to pick the report up
immediately and because in this case we know what the problem is its
nice to show it to the user. Alternatively I could make this
conditional, i.e. if update-notifier is availalbe rely on that and if
not display a internal dialog. WDYT?

> > + # show a apport recoverable error dialog to the user as we want
> > + # to know about these issues
> > + self._call_apport_recoverable_error(
> > + _("There was a error submitting the transaction"), error)
>
> Grammar nit: an before vowel sounds. So it's "there was an error."

Fixed.

> > + # mvo: I don't think we need to send "Package\0software-center",
> > + # apport should figure this out itself
> > + data = "DialogBody\0%(text)s\0Traceback\0%(error)s" % {
> > + 'text': text,
> > + 'error': error,
> > + }
> > + # this will be quick as it just writes the report file and the
> > + # helper is spawned by apport asyncronsoly
>
> Correct, but to clarify: the report is written, update-notifier has an
> inotify watch on /var/crash and it then calls apport to process unseen
> reports.

I updated the comment in the code to make this a bit clearer.

> > + APPORT_RECOVERABLE_ERROR = "/usr/share/apport/recoverable_problem"
> > + p = Popen(
> > + [APPORT_RECOVERABLE_ERROR], stdin=PIPE, stdout=PIPE, stderr=PIPE)
> > +
> > + (stdout, stderr) = p.communicate(input=data)
> > + return_code = p.wait()
>
> subprocess.communicate() calls wait():
> Lib/subprocess.py:communicate:751
>
> So you could just grab it off the pid member. There's no harm in
> calling wait() again though, so feel free to leave that.

Good point :) I simply use p.returncode now.

> > === modifie...

[lp:~mvo/software-center/polkit-lp785117]

3037. By Michael Vogt on 2012-08-23

trivial pep8 fixes

[Evan (ev)]

On Thu, Aug 23, 2012 at 1:30 PM, Michael Vogt <email address hidden> wrote:
> What is your opinion on showing the internal dialog still in addition
> to fire of the report to apport? The assumption here is that the user
> might not have update-notifier runnig to pick the report up
> immediately and because in this case we know what the problem is its
> nice to show it to the user. Alternatively I could make this
> conditional, i.e. if update-notifier is availalbe rely on that and if
> not display a internal dialog. WDYT?

I think the cases of update-notifier are sufficiently small to not
burden everyone with redundant dialogs. For what it's worth, using
update-notifier for inotify watches will be replaced by upstart
inotify events once James has finished those.

The rest looks good.

[Evan (ev)]

review: Approve

=== modified file 'softwarecenter/backend/installbackend_impl/aptd.py'
--- softwarecenter/backend/installbackend_impl/aptd.py	2012-08-07 13:15:08 +0000
+++ softwarecenter/backend/installbackend_impl/aptd.py	2012-08-23 12:29:44 +0000
@@ -21,6 +21,12 @@
 import logging
 import os
 import re
+import traceback
+
+from subprocess import (
+    Popen,
+    PIPE,
+    )
 
 from gi.repository import GObject
 
@@ -29,7 +35,11 @@
                                   obfuscate_private_ppa_details,
                                   utf8,
                                   )
-from softwarecenter.enums import TransactionTypes, PURCHASE_TRANSACTION_ID
+from softwarecenter.enums import (
+    TransactionTypes,
+    PURCHASE_TRANSACTION_ID,
+    )
+from softwarecenter.paths import APPORT_RECOVERABLE_ERROR
 
 from aptdaemon import client
 from aptdaemon import enums
@@ -973,13 +983,53 @@
 
         self.emit("transaction-stopped", result)
         if isinstance(error, dbus.DBusException):
+            # ignore errors that the user knows about already (like
+            # that he entered a wrong password or that he does not
+            # care about (like no-reply)
             name = error.get_dbus_name()
             if name in ["org.freedesktop.PolicyKit.Error.NotAuthorized",
                         "org.freedesktop.DBus.Error.NoReply"]:
-                pass
-        else:
-            self._logger.exception("_on_trans_error")
-            #raise error
+                return
+            # we want to give some advice here to the user but also
+            # report this via apport
+            if name in ["org.freedesktop.PolicyKit.Error.Failed"]:
+                summary = _("Authentication Error")
+                text = _("Software can't be installed or removed because "
+                         "the authentication service is not available. "
+                         "(%s") % error
+                # send to apport for reporting
+                self._call_apport_recoverable_error(text, error)
+                # ... and display as a dialog
+                self.ui.error(None, summary, text)
+                return
+        # show a apport recoverable error dialog to the user as we want
+        # to know about these issues
+        self._call_apport_recoverable_error(
+            _("There was an error submitting the transaction"), error)
+
+    def _call_apport_recoverable_error(self, text, error):
+        """Call apports recoverable_problem dialog """
+
+        # ensure we have a proper expection string in the report
+        if isinstance(error, Exception):
+            error = traceback.format_exc(error)
+
+        # mvo: I don't think we need to send "Package\0software-center",
+        #      apport should figure this out itself
+        data = "DialogBody\0%(text)s\0Traceback\0%(error)s" % {
+                'text': text,
+                'error': error,
+                }
+        # This will be quick as it just writes the report file. Then
+        # the report gets picked up asynchronously by a inotify watch
+        # and displayed to the user in a seperate process.
+        p = Popen(
+            [APPORT_RECOVERABLE_ERROR], stdin=PIPE, stdout=PIPE, stderr=PIPE)
+
+        (stdout, stderr) = p.communicate(input=data)
+        if p.returncode != 0:
+            logging.warn("%s returned '%s' ('%s', '%s')" % (
+                    APPORT_RECOVERABLE_ERROR, p.returncode, stdout, stderr))
 
 
 if __name__ == "__main__":

=== modified file 'softwarecenter/paths.py'
--- softwarecenter/paths.py	2012-08-22 08:16:30 +0000
+++ softwarecenter/paths.py	2012-08-23 12:29:44 +0000
@@ -73,7 +73,15 @@
 # OEM
 OEM_CHANNEL_DESCRIPTOR = "/var/lib/ubuntu-dist-channel"
 
-
+<<<<<<< TREE
+
+=======
+
+# apport
+APPORT_RECOVERABLE_ERROR = "/usr/share/apport/recoverable_problem"
+
+
+>>>>>>> MERGE-SOURCE
 # ratings&review
 # relative to datadir
 class RNRApps:

=== modified file 'tests/test_aptd.py'
--- tests/test_aptd.py	2012-06-28 08:51:44 +0000
+++ tests/test_aptd.py	2012-08-23 12:29:44 +0000
@@ -3,18 +3,23 @@
 
 import logging
 import os
+import subprocess
 import time
 import unittest
 
+import dbus
+
 from tests.utils import (
     setup_test_env,
 )
 setup_test_env()
 
+from softwarecenter.backend.installbackend_impl.aptd import AptdaemonBackend
 from softwarecenter.db.application import Application
-from softwarecenter.backend.installbackend_impl.aptd import AptdaemonBackend
+from softwarecenter.paths import APPORT_RECOVERABLE_ERROR
+
 from defer import inline_callbacks
-from mock import Mock
+from mock import Mock, patch
 
 import aptdaemon.loop
 
@@ -24,6 +29,7 @@
 
     def setUp(self):
         self.aptd = AptdaemonBackend()
+        self.aptd.ui = Mock()
         # monkey patch
         self.aptd.aptd_client.install_packages = self._mock_aptd_client_install_packages
         self._pkgs_to_install = []
@@ -105,6 +111,25 @@
         addons_remove = ["gimp-plugin-registry"]
         yield self.aptd.apply_changes(pkgname, appname ,iconname, addons_install, addons_remove)
 
+
+    def test_trans_error_ui_display(self):
+        """ test if the right error ui is displayed for various dbus 
+            errors
+        """
+        error = dbus.DBusException()
+        dbus_name_mock = Mock()
+        with patch.object(self.aptd.ui, "error") as error_ui_mock:
+            for dbus_name, show_error_ui in [
+                ("org.freedesktop.PolicyKit.Error.NotAuthorized", False),
+                ("org.freedesktop.PolicyKit.Error.Failed", True),
+                ("moo.baa.lalala", True),
+                ]:
+                error_ui_mock.reset()
+                dbus_name_mock.return_value = dbus_name
+                error.get_dbus_name = dbus_name_mock
+                self.aptd._on_trans_error(error)
+                self.assertEqual(error_ui_mock.called, show_error_ui)
+
     @inline_callbacks
     def _inline_add_repo_call(self):
         deb_line = "deb https://foo"
@@ -113,7 +138,7 @@
         iconname = "iconname"
         yield self.aptd.add_repo_add_key_and_install_app(
             deb_line, signing_key_id, app, iconname, None, None)
-        
+
     def test_add_repo_add_key_and_install_app(self):
         from mock import patch
         with patch.object(self.aptd._logger, "info") as mock:
@@ -121,5 +146,22 @@
             self.assertTrue(
                 mock.call_args[0][0].startswith("add_repo_add_key"))
 
+    @patch("softwarecenter.backend.installbackend_impl.aptd.Popen")
+    def test_recoverable_error(self, mock_popen):
+        mock_popen_instance = Mock()
+        mock_popen_instance.communicate.return_value = ("stdout", "stderr")
+        mock_popen_instance.returncode = 0
+        mock_popen.return_value = mock_popen_instance
+        self.aptd._call_apport_recoverable_error("msg", "traceback-error")
+        mock_popen.assert_called_with(
+            [APPORT_RECOVERABLE_ERROR], stdin=subprocess.PIPE, 
+            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        # check that we really send the right data 
+        args, kwargs = mock_popen_instance.communicate.call_args
+        self.assertEqual(
+            kwargs["input"].split("\0"),
+            [ 'DialogBody', 'msg','Traceback', 'traceback-error'])
+
+
 if __name__ == "__main__":
     unittest.main()