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

[Robert Roth]

This branch fixes the following bugs:
- Bug #496879 - returns non-0 result code from add-apt-repository whenever anything goes wrong, PPA can not be found, or signing key can not be found
- Bug #779287 - API url changed to the URL of the pubic API
- Bug #489488 - partially fixes this by adding the PPA display name as deb-line comment, thus the displayname will be shown in software-properties instead of the long and confusing PPA url
- Bug #930624 - add a title (Add) to the add-dialog (instead of the default software-properties-gtk) and sets the image of the Add Source button to respect the buttons-have-icons setting
- Bug #599801 - sort software sources from the other software tab alphabetically by their description, this way we get a more deterministic ordering than the default, thus users will be able to find the source they are looking for faster.

I have also made the following changes:
- removed a duplicated utf8 method(the same method has been declared twice in apt-add-repo),
- added another testcase in testlp to test invalid repo is handled correctly (no PPA info is returned)
- updated the error handling (try .. except HTTPError, URLError statements have been removed) to work with new curl based PPA info fetching (as curl does not throw an exception, but the result code can be queried separately) by only accepting HTTP 200 OK for now, other "partially successful" HTTP response codes should be added as required
- added new testaptaddrepo tests for apt-add-repository sanity checks, to see if adding invalid repo returns non-0 result code and working commands return 0 result codes

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

749. By Robert Roth on 2012-02-11

Fix add source dialog consistency issues (LP: #930624)

750. By Robert Roth on 2012-02-12

Sort software sources alphabetically by their description (LP: #599801)

[Michael Vogt (mvo)]

Thanks! This looks great. I tweaked some minor issues and added a FIXME for the new tests/test_aptaddrepo.py so that it can be run as non-root. Do you think you would have time to fix this small issue? If not I will give it a go later :)

review: Needs Information

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

751. By Robert Roth on 2012-05-11

Merged from trunk

[Robert Roth (evfool)]

Hi mvo, I have missed the status change on this proposal. I have merged from the current trunk now, and I can't see your FIXMEs, what should I fix here?

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

752. By Robert Roth on 2012-05-11

Merged from mvo's branch

753. By Robert Roth on 2012-05-11

Added apt-root dir CLI parameter for apt-add-repository

[Robert Roth (evfool)]

I have found your branch, merged from it, made a few adjustments, and added an apt-rootdir CLI parameter for apt-add-repository and tried to make it work in the test, however somehow I couldn't get the tests correctly working. Could you please take a look at the updated branch?

Unmerged revisions

753. By Robert Roth on 2012-05-11

Added apt-root dir CLI parameter for apt-add-repository

752. By Robert Roth on 2012-05-11

Merged from mvo's branch

751. By Robert Roth on 2012-05-11

Merged from trunk

750. By Robert Roth on 2012-02-12

Sort software sources alphabetically by their description (LP: #599801)

749. By Robert Roth on 2012-02-11

Fix add source dialog consistency issues (LP: #930624)

748. By Robert Roth on 2012-02-11

Also return non-null result code if PPA key validation fails (LP: #496879)

747. By Robert Roth on 2012-02-11

Add basic unit test for apt-add-repository

746. By Robert Roth on 2012-02-11

Add the display name of a PPA as deb entry comment (LP: #489488)

745. By Robert Roth on 2012-02-11

Use public Launchpad API (LP:#779287)

744. By Robert Roth on 2012-02-11

Fix error handling in new launchpad info retrieval (LP: #496879)

=== modified file 'add-apt-repository'
--- add-apt-repository	2012-03-07 06:43:56 +0000
+++ add-apt-repository	2012-05-11 09:20:22 +0000
@@ -4,6 +4,7 @@
 import sys
 import gettext
 import locale
+import pycurl
 
 from softwareproperties.SoftwareProperties import SoftwareProperties
 from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line
@@ -24,7 +25,7 @@
         return s.encode("utf-8", "ignore")
     return unicode(s, "utf8", "ignore").encode("utf8")
 
-def _maybe_suggest_ppa_name_based_on_user(user):
+def _maybe_suggest_ppa_name_based_on_user(user, ppa_name):
     try:
         from launchpadlib.launchpad import Launchpad
         lp = Launchpad.login_anonymously(lp_application_name, "production")
@@ -32,31 +33,61 @@
             user_inst = lp.people[user]
             entity_name = "team" if user_inst.is_team else "user"
             if len(user_inst.ppas) > 0:
-                print _("The %s named '%s' has no PPA named '%s'" 
-                        %(entity_name, user, ppa_name))
+                print _("The %s named '%s' has no PPA named '%s'" ) % (entity_name, user, ppa_name)
                 print _("Please choose from the following available PPAs:")
                 for ppa in user_inst.ppas:
                     print _(" * '%s':  %s" %(ppa.name, ppa.displayname))
             else:
-                print _("The %s named '%s' does not have any PPA"
-                        %(entity_name, user))
+                print _("The %s named '%s' does not have any PPA repositories.") % (entity_name, user)
         except KeyError:
             pass
     except ImportError:
         print _("Please check that the PPA name or format is correct.")
 
-
-
-def utf8(s):
-    """
-    Takes a string or unicode object and returns a utf-8 encoded
-    string, errors are ignored
-    """
-    if s is None:
-        return None
-    if isinstance(s, unicode):
-        return s.encode("utf-8", "ignore")
-    return unicode(s, "utf8", "ignore").encode("utf8")
+def ask_user_about_ppa(line, is_removal):
+
+    from softwareproperties.ppa import get_ppa_info_from_lp, LAUNCHPAD_PPA_API
+    user, sep, ppa_name = line.split(":")[1].partition("/")
+    ppa_name = ppa_name or "ppa"
+    try: 
+        ppa_info = get_ppa_info_from_lp(user, ppa_name)
+    except pycurl.error:
+        print _("Cannot access PPA (%s) to get PPA information, "
+               "please check your internet connection.") % \
+            (LAUNCHPAD_PPA_API % (user, ppa_name))
+        return (1, None)
+    # ppa not found
+    if not ppa_info:
+        print _("The specified PPA (%s) could not be found. ") % line
+        if user.startswith("~"):
+            print _("Did you mean 'ppa:%s/%s' ?" %(user[1:], ppa_name))
+            return (1, None) # Exit because the user cannot be correct
+        # If the PPA does not exist, then try to find if the user/team 
+        # exists. If it exists, list down the PPAs
+        _maybe_suggest_ppa_name_based_on_user(user, ppa_name)
+        return (1, None)
+    # private PPAs are not supported
+    if "private" in ppa_info and ppa_info["private"]:
+        print _("Adding private PPAs is not supported currently")
+        return (1, None)
+        
+    if is_removal:
+        print _("You are about to remove the following PPA from your system:")
+    else:
+        print _("You are about to add the following PPA to your system:")
+    print " %s" % utf8(ppa_info["description"] or "")
+    print _(" More info: %s") % ppa_info["web_link"]
+    if (sys.stdin.isatty() and
+        not "FORCE_ADD_APT_REPOSITORY" in os.environ):
+        if is_removal:
+            print _("Press [ENTER] to continue or ctrl-c to cancel removing it")
+        else:
+            print _("Press [ENTER] to continue or ctrl-c to cancel adding it")
+        try: 
+           sys.stdin.readline()
+        except KeyboardInterrupt:
+           return (2, None)
+    return (0, ppa_info["displayname"])
 
 if __name__ == "__main__":
     try:
@@ -100,9 +131,16 @@
     parser.add_option("-y", "--yes", action="store_true",
         dest="assume_yes", default=False,
         help="Assume yes to all queries")
+    parser.add_option("-a", "--aptrootdir",
+        action="store", dest="aptrootdir", type="string", default=None, 
+        help="Use the given directory as apt root (for testing purposes)")
     (options, args) = parser.parse_args()
 
-    if os.geteuid() != 0:
+    if options.aptrootdir:
+        if not os.access(options.aptrootdir, os.W_OK):
+            print _("Error: you don't have write access to the sources.list file %s, try running as root")%options.aptrootdir
+            sys.exit(1)
+    elif os.geteuid() != 0:
         print _("Error: must run as root")
         sys.exit(1)
 
@@ -118,46 +156,16 @@
 
     # display PPA info (if needed)
     if line.startswith("ppa:") and not options.assume_yes:
-        from softwareproperties.ppa import get_ppa_info_from_lp, LAUNCHPAD_PPA_API
-        user, sep, ppa_name = line.split(":")[1].partition("/")
-        ppa_name = ppa_name or "ppa"
-        try:
-            ppa_info = get_ppa_info_from_lp(user, ppa_name)
-        except HTTPError:
-            print _("Cannot add PPA: '%s'.") % line
-            if user.startswith("~"):
-                print _("Did you mean 'ppa:%s/%s' ?" %(user[1:], ppa_name))
-                sys.exit(1) # Exit because the user cannot be correct
-            # If the PPA does not exist, then try to find if the user/team 
-            # exists. If it exists, list down the PPAs
-            _maybe_suggest_ppa_name_based_on_user(user)
-            sys.exit(1)
-        except (ValueError, URLError):
-            print _("Cannot access PPA (%s) to get PPA information, "
-                    "please check your internet connection.") % \
-                (LAUNCHPAD_PPA_API % (user, ppa_name))
-            sys.exit(1)
-        # private PPAs are not supported
-        if "private" in ppa_info and ppa_info["private"]:
-            print _("Adding private PPAs is not supported currently")
-            sys.exit(1)
-        
-        if options.remove:
-            print _("You are about to remove the following PPA from your system:")
-        else:
-            print _("You are about to add the following PPA to your system:")
-        print " %s" % utf8(ppa_info["description"] or "")
-        print _(" More info: %s") % ppa_info["web_link"]
-        if (sys.stdin.isatty() and
-            not "FORCE_ADD_APT_REPOSITORY" in os.environ):
-            if options.remove:
-                print _("Press [ENTER] to continue or ctrl-c to cancel removing it")
-            else:
-                print _("Press [ENTER] to continue or ctrl-c to cancel adding it")
-            sys.stdin.readline()
+        (error_code, display_name) = ask_user_about_ppa(line, options.remove)
+        if error_code != 0:
+            sys.exit(error_code)
+        line += "#" + display_name
 
     # add it
-    sp = SoftwareProperties(options=options)
+    if options.aptrootdir:
+        sp = SoftwareProperties(options=options, rootdir=options.aptrootdir)
+    else:
+        sp = SoftwareProperties(options=options)
     if options.remove:
         (line, file) = expand_ppa_line(line.strip(), sp.distro.codename)
         deb_line = sp.expand_http_line(line)
@@ -167,14 +175,14 @@
         try:
             sp.remove_source(deb_entry)
         except ValueError:
-            print _("Error: '%s' doesn't exist in a sourcelist file" % deb_line)
+            print _("Error: '%s' doesn't exist in a sourcelist file") % deb_line
         try:
             sp.remove_source(debsrc_entry)
         except ValueError:
-            print _("Error: '%s' doesn't exist in a sourcelist file" % debsrc_line)
+            print _("Error: '%s' doesn't exist in a sourcelist file") % debsrc_line
 
     else:
         if not sp.add_source_from_line(line):
-            print _("Error: '%s' invalid" % line)
+            print _("Error: could not add '%s'") % line
             sys.exit(1)
         sp.sourceslist.save()

=== modified file 'data/gtkbuilder/dialog-add.ui'
--- data/gtkbuilder/dialog-add.ui	2011-06-22 15:03:09 +0000
+++ data/gtkbuilder/dialog-add.ui	2012-05-11 09:20:22 +0000
@@ -2,11 +2,17 @@
 <interface>
   <!-- interface-requires gtk+ 2.12 -->
   <!-- interface-naming-policy toplevel-contextual -->
+  <object class="GtkImage" id="add_image">
+    <property name="visible">True</property>
+    <property name="can_focus">False</property>
+    <property name="stock">gtk-add</property>
+  </object>
   <object class="GtkDialog" id="dialog_add_custom">
     <property name="border_width">6</property>
     <property name="resizable">False</property>
     <property name="modal">True</property>
     <property name="type_hint">dialog</property>
+    <property name="title" translatable="yes">Add</property>
     <property name="skip_taskbar_hint">True</property>
     <child internal-child="vbox">
       <object class="GtkVBox" id="dialog-vbox2">
@@ -126,6 +132,8 @@
             </child>
             <child>
               <object class="GtkButton" id="button_add_source">
+                <property name="label" translatable="yes">_Add Source</property>
+                <property name="use_action_appearance">False</property>
                 <property name="visible">True</property>
                 <property name="sensitive">False</property>
                 <property name="can_focus">True</property>
@@ -133,42 +141,9 @@
                 <property name="can_default">True</property>
                 <property name="has_default">True</property>
                 <property name="receives_default">False</property>
-                <child>
-                  <object class="GtkAlignment" id="alignment1">
-                    <property name="visible">True</property>
-                    <property name="xscale">0</property>
-                    <property name="yscale">0</property>
-                    <child>
-                      <object class="GtkHBox" id="hbox10">
-                        <property name="visible">True</property>
-                        <property name="spacing">2</property>
-                        <child>
-                          <object class="GtkImage" id="image2">
-                            <property name="visible">True</property>
-                            <property name="stock">gtk-add</property>
-                          </object>
-                          <packing>
-                            <property name="expand">False</property>
-                            <property name="fill">False</property>
-                            <property name="position">0</property>
-                          </packing>
-                        </child>
-                        <child>
-                          <object class="GtkLabel" id="label35">
-                            <property name="visible">True</property>
-                            <property name="label" translatable="yes">_Add Source</property>
-                            <property name="use_underline">True</property>
-                          </object>
-                          <packing>
-                            <property name="expand">False</property>
-                            <property name="fill">False</property>
-                            <property name="position">1</property>
-                          </packing>
-                        </child>
-                      </object>
-                    </child>
-                  </object>
-                </child>
+                <property name="use_action_appearance">False</property>
+                <property name="image">add_image</property>
+                <property name="use_underline">True</property>
               </object>
               <packing>
                 <property name="expand">False</property>

=== modified file 'softwareproperties/SoftwareProperties.py'
--- softwareproperties/SoftwareProperties.py	2012-03-07 08:42:03 +0000
+++ softwareproperties/SoftwareProperties.py	2012-05-11 09:20:22 +0000
@@ -491,7 +491,7 @@
             for c in source.comps:
                 contents += " %s" % c
         if source.type in ("deb-src", "rpm-src"):
-            contents += " %s" % _("(Source Code)")
+            contents += " (%s)" % _("Source Code")
         return contents
     else:
         # try to make use of a corresponding template
@@ -697,6 +697,8 @@
     deb_line = self.expand_http_line(deb_line)
     debsrc_line = 'deb-src' + deb_line[3:]
     new_deb_entry = SourceEntry(deb_line, file)
+    if new_deb_entry.comment:
+       debsrc_line += " (%s)" % _("Source Code")
     new_debsrc_entry = SourceEntry(debsrc_line, file)
     if new_deb_entry.invalid or new_debsrc_entry.invalid:
       return False
@@ -719,8 +721,8 @@
     if worker:
         # wait for GPG key to be downloaded
         worker.join(30)
-    return True
-
+    return worker.success
+    
   def remove_source(self, source):
     """Remove the given source"""
     # first find the source object if we got a string

=== modified file 'softwareproperties/gtk/SoftwarePropertiesGtk.py'
--- softwareproperties/gtk/SoftwarePropertiesGtk.py	2012-03-07 09:13:48 +0000
+++ softwareproperties/gtk/SoftwarePropertiesGtk.py	2012-05-11 09:20:22 +0000
@@ -572,6 +572,7 @@
                                       GObject.TYPE_PYOBJECT,
                                       GObject.TYPE_BOOLEAN,
                                       GObject.TYPE_BOOLEAN)
+    self.source_store.set_sort_column_id(STORE_DESCRIPTION, Gtk.SortType.ASCENDING)
     self.treeview_sources.set_model(self.source_store)
     self.treeview_sources.set_row_separator_func(self.is_separator,
                                                  STORE_SEPARATOR)
@@ -582,6 +583,7 @@
     col_desc = Gtk.TreeViewColumn(_("Software Sources"), cell_desc,
                                   markup=COLUMN_DESC)
     col_desc.set_max_width(1000)
+    col_desc.set_sort_column_id(STORE_DESCRIPTION)
 
     cell_toggle = Gtk.CellRendererToggle()
     cell_toggle.set_property("xpad", 2)

=== modified file 'softwareproperties/ppa.py'
--- softwareproperties/ppa.py	2012-01-31 15:51:15 +0000
+++ softwareproperties/ppa.py	2012-05-11 09:20:22 +0000
@@ -25,10 +25,11 @@
 import subprocess
 from threading import Thread
 import pycurl
+from httplib import OK as HTTP_200_OK
 
 DEFAULT_KEYSERVER = "hkp://keyserver.ubuntu.com:80/"
 # maintained until 2015
-LAUNCHPAD_PPA_API = 'https://launchpad.net/api/1.0/~%s/+archive/%s'
+LAUNCHPAD_PPA_API = 'https://api.launchpad.net/1.0/~%s/+archive/%s'
 # None means use pycurl default 
 LAUNCHPAD_PPA_CERT = None
 
@@ -45,13 +46,18 @@
     #        via some sort of API, see LP #385129)
     abrev = abrev.split(":")[1]
     ppa_owner = abrev.split("/")[0]
+    comment = None
+    if abrev.find("#") != -1:
+        comment = abrev.split("#")[1]
     try:
-        ppa_name = abrev.split("/")[1]
+        ppa_name = abrev.split("/")[1].split("#")[0]
     except IndexError, e:
         ppa_name = "ppa"
     sourceslistd = apt_pkg.config.find_dir("Dir::Etc::sourceparts")
     line = "deb http://ppa.launchpad.net/%s/%s/ubuntu %s main" % (
         ppa_owner, ppa_name, distro_codename)
+    if comment:
+        line += "#" + comment
     filename = "%s/%s-%s-%s.list" % (
         sourceslistd, encode(ppa_owner), encode(ppa_name), distro_codename)
     return (line, filename)
@@ -78,9 +84,12 @@
     curl.setopt(pycurl.URL, str(lp_url))
     curl.setopt(pycurl.HTTPHEADER, ["Accept: application/json"])
     curl.perform()
+    return_code = curl.getinfo(pycurl.HTTP_CODE);
     curl.close()
-    lp_page = callback.contents
-    return json.loads(lp_page)
+    if return_code == HTTP_200_OK:
+        lp_page = callback.contents
+        return json.loads(lp_page)
+    return None
 
 class AddPPASigningKeyThread(Thread):
     " thread class for adding the signing key in the background "
@@ -88,11 +97,12 @@
     def __init__(self, ppa_path, keyserver=None):
         Thread.__init__(self)
         self.ppa_path = ppa_path
+        self.success = None
         self.keyserver = (keyserver if keyserver is not None
                           else DEFAULT_KEYSERVER)
         
     def run(self):
-        self.add_ppa_signing_key(self.ppa_path)
+        self.success = self.add_ppa_signing_key(self.ppa_path)
         
     def add_ppa_signing_key(self, ppa_path):
         """Query and add the corresponding PPA signing key.
@@ -112,9 +122,11 @@
             print "Error: can't find signing_key_fingerprint at %s" % lp_url
             return False
         res = subprocess.call(
-            ["apt-key", "adv", 
+            ["apt-key", "adv",
              "--keyserver", self.keyserver,
              "--recv", signing_key_fingerprint])
+        if res != 0:
+            print "Error: can't retrieve key file for the PPA's signing_key_fingerprint"
         return (res == 0)
 
 

=== added file 'tests/test_aptaddrepo.py'
--- tests/test_aptaddrepo.py	1970-01-01 00:00:00 +0000
+++ tests/test_aptaddrepo.py	2012-05-11 09:20:22 +0000
@@ -0,0 +1,36 @@
+#!/usr/bin/python
+
+import pycurl
+import unittest
+import sys
+from subprocess import call
+sys.path.insert(0, "..")
+
+class TestAptAddRepo(unittest.TestCase):
+
+    # FIXME: we need to add a --datadir argument for add-apt-repository
+    #        so that we can run this test as non-root
+
+    def test_adding_invalid_ppa(self):
+        resultcode = call(["../add-apt-repository",
+                           "-a" , "./aptroot",
+                           "ppa:mvo/inexistent-testing-ppa"])
+        self.assertNotEquals(resultcode, 0)
+
+    def test_ppa_add(self):
+        resultcode = call(["../add-apt-repository",
+                           "-y",
+                           "-a" , "./aptroot",
+                           "ppa:mvo/ppa"])
+        self.assertEquals(resultcode, 0)
+
+    def test_ppa_remove(self):
+        resultcode = call(["../add-apt-repository",
+                           "-y", "-r",
+                           "-a" , "./aptroot",
+                           "ppa:mvo/ppa"])
+        self.assertEquals(resultcode, 0)
+            
+
+if __name__ == "__main__":
+    unittest.main()

=== modified file 'tests/test_lp.py'
--- tests/test_lp.py	2012-01-31 15:51:15 +0000
+++ tests/test_lp.py	2012-05-11 09:20:22 +0000
@@ -19,7 +19,10 @@
         self.assertRaises(
             pycurl.error, softwareproperties.ppa.get_ppa_info_from_lp, "mvo", "ppa")
 
-        
+    def test_invalid_ppa(self):
+        # retrieve info for an inexistent PPA which will return anything else than HTTP 200 OK
+        info = softwareproperties.ppa.get_ppa_info_from_lp("evfool", "inexistent-testing-ppa")
+        self.assertEquals(info, None)
 
 
 if __name__ == "__main__":