Merge lp:~evfool/software-properties/lp1061101 into lp:software-properties

[Robert Roth]

This branch adds a test to check for None signing key fingerprint handling (usually when a user creates his/her first PPA and tries to add it instantaneously with apt-add-repo, this happens, as the signing key fingerprint seems to be correctly returned by launchpad with a delay of up to 25 minutes).

[Didier Roche-Tolomelli (didrocks)]

Thanks for your work Robert ;)

Contrary to the other one, shouldn't this one be an actual dialog to warn the user? I think not a lot of people are looking at stdout, isn't it?

review: Needs Information

[Robert Roth (evfool)]

I don't think a dialog here is appropriate, as apt-add-repo is a terminal utility (and this particular bug is about using a terminal application), so popping up a dialog would feel sortof strange. However adding a log message here would be nice. What do you think?

[Robert Roth (evfool)]

I just checked, and:
* there's already a message logged with print()
* however this code is used (or should be used) by both the terminal and gtk frontend, so maybe throwing an exception and (try .. except ..) handling it with an appropriate log message/warning dialog depending on the frontend used might be even better.
Does that sound better?

[Didier Roche-Tolomelli (didrocks)]

Great ;)
Sounds indeed the right plan to me.

Bonus point if the other print() should follow the same spirit and you fix it :)

[lp:~evfool/software-properties/lp1061101]

823. By on 2012-11-23

Use an exception to signal a signing key problem

[Robert Roth (evfool)]

I have updated the branch to raise a SigningKeyException, and handled it in apt-add-repository and software-properties-gtk command line option, however when using the interface and calling add_source_from_line method over dbus the exception gets swallowed somehow.
If anyone with a bit more DBUS experience could take a look, it would be nice. Right now there's no feedback when the signing key cannot be added from the GTK interface, but this did happened before the fix too, so there's no change there.

Unmerged revisions

823. By on 2012-11-23

Use an exception to signal a signing key problem

822. By Robert Roth on 2012-11-21

Handle None signing key fingerprints from LP (LP: #1061101)

=== modified file 'add-apt-repository'
--- add-apt-repository	2012-11-09 12:21:58 +0000
+++ add-apt-repository	2012-11-23 11:54:22 +0000
@@ -9,7 +9,7 @@
 import locale
 
 from softwareproperties.SoftwareProperties import SoftwareProperties
-from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line
+from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line, SigningKeyException
 from softwareproperties import lp_application_name
 import aptsources
 from aptsources.sourceslist import SourceEntry
@@ -198,7 +198,13 @@
             print(_("Error: '%s' doesn't exist in a sourcelist file") % debsrc_line)
 
     else:
-        if not sp.add_source_from_line(line):
+        returned = None
+        try:
+            returned = sp.add_source_from_line(line)
+        except SigningKeyException as e:
+            print(_("There was a problem retrieving the PPA signing key from the server (%s) , please try again later.")%str(e))
+            sys.exit(2)
+        if not returned:
             print(_("Error: '%s' invalid") % line)
             sys.exit(1)
         sp.sourceslist.save()
 
=== modified file 'software-properties-gtk'
--- software-properties-gtk	2012-06-21 21:00:08 +0000
+++ software-properties-gtk	2012-11-23 11:54:22 +0000
@@ -91,7 +91,11 @@
     file = args[0]
   if options.enable_ppa:
     app = SoftwarePropertiesGtk(datadir=options.data_dir, options=options, file=file)
-    app.add_source_from_line("ppa:%s" % options.enable_ppa)
+    try:
+      app.add_source_from_line("ppa:%s" % options.enable_ppa)
+    except SigningKeyException as e:
+      print(_("There was a problem retrieving the PPA signing key from the server (%s) , please try again later.")%str(e))
+      sys.exit(2)
     app.sourceslist.save()
   elif options.enable_component:
     sourceslist = SourcesList()
 
=== modified file 'softwareproperties/ppa.py'
--- softwareproperties/ppa.py	2012-10-02 18:32:36 +0000
+++ softwareproperties/ppa.py	2012-11-23 11:54:22 +0000
@@ -63,6 +63,15 @@
     def __str__(self):
         return repr(self.value)
 
+class SigningKeyException(Exception):
+
+    def __init__(self, value, original_error=None):
+        self.value = value
+        self.original_error = original_error
+
+    def __str__(self):
+        return repr(self.value)
+
 
 def encode(s):
     return re.sub("[^a-zA-Z0-9_-]", "_", s)
@@ -138,14 +147,25 @@
                           else DEFAULT_KEYSERVER)
         
     def run(self):
-        self.add_ppa_signing_key(self.ppa_path)
+        self.exc = None
+        try:
+            self.add_ppa_signing_key(self.ppa_path)
+        except SigningKeyException:
+            import sys
+            self.exc=sys.exc_info()
+
+    def join(self, timeout):
+        Thread.join(self, timeout)
+        if self.exc:
+            raise self.exc[1]
         
     def _recv_key(self, keyring, secret_keyring, signing_key_fingerprint, keyring_dir):
         # double check that the signing key is a v4 fingerprint (160bit)
-        if not verify_keyid_is_v4(signing_key_fingerprint):
-            print("Error: signing key fingerprint '%s' too short" % 
+        if signing_key_fingerprint:
+            raise SigningKeyException("No signing key fingerprint received, the PPA might not have it yet.");
+        elif not verify_keyid_is_v4(signing_key_fingerprint):
+            raise SigningKeyException("Error: signing key fingerprint '%s' too short" % 
                   signing_key_fingerprint)
-            return False
         # then get it
         res = subprocess.call(self.GPG_DEFAULT_OPTIONS + [
             "--homedir", keyring_dir,
 
=== modified file 'tests/test_lp.py'
--- tests/test_lp.py	2012-10-16 17:19:36 +0000
+++ tests/test_lp.py	2012-11-23 11:54:22 +0000
@@ -46,6 +46,19 @@
 
     @patch("softwareproperties.ppa.get_ppa_info_from_lp")
     @patch("softwareproperties.ppa.subprocess")
+    def test_fingerprint_none(self, mock_subprocess, mock_get_ppa_info):
+        """Test that None PPA signing keys are properly handled (signing keys are not created at the same time as the PPA)""" 
+        mock_ppa_info = MOCK_PPA_INFO.copy()
+        mock_ppa_info["signing_key_fingerprint"] = None
+        mock_get_ppa_info.return_value = mock_ppa_info
+        # do it
+        res = self.t.add_ppa_signing_key("/mvo/ppa/ubuntu")
+        self.assertFalse(res)
+        self.assertFalse(mock_subprocess.Popen.called)
+        self.assertFalse(mock_subprocess.call.called)
+
+    @patch("softwareproperties.ppa.get_ppa_info_from_lp")
+    @patch("softwareproperties.ppa.subprocess")
     def test_fingerprint_len_check(self, mock_subprocess, mock_get_ppa_info):
         """Test that short keyids (<160bit) are rejected""" 
         mock_ppa_info = MOCK_PPA_INFO.copy()