Merge lp:~hodgestar/software-properties/configurable-key-server into lp:software-properties

[Hodgestar]

A small patch that adds a --keyserver option to apt-add-repository. My main use case is so that I can fall back to other alternatives if the Ubuntu key server is down or inaccessible for some reason. I consider the patch just a draft -- I don't know enough about how the code base is used to tell whether I've added the new option to the appropriate places.

[lp:~hodgestar/software-properties/configurable-key-server]

659. By Jonathan Riddell on 2011-04-14

data/designer/main.ui make strings match GTK UI file LP: #760825

660. By Stéphane Graber on 2011-04-24

In Sources tab, ignore deb-src templates

661. By Stéphane Graber on 2011-04-24

releasing version 0.80.9

[Barry Warsaw (barry)]

Hi Simon, thanks very much for the contribution to Ubuntu! I agree this could be a useful addition to the code. It doesn't happen often (these days, IME) but I have seen failures from the ubuntu keyserver, so in those cases it would be nice to have a fallback.

I have some comments on your patch. You're not too far off, but with a few changes, I think it would be a solid addition.

In add-apt-repository, I don't think you need the 'dest' argument to add_option(). The long version of the switch will be taken as the default dest. Also note the misspelling of 'Default' in the help string.

It looks like you have multiple places where 'hkp://keyserver.ubuntu.com:80/' is hardcoded. I'd refactor this so that it's defined exactly once, in a module global constant (upper-cased) in ppa.py. Then in apt-add-repository, I'd change the import at line 9 to be:

    from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line

(i.e. DEFAULT_KEYSERVER being the module global). Use that variable in the add_option() call.

Interestingly enough, the SoftwareProperties class already takes an `options` keyword in its __init__(), so I don't think you need to extend that method's signature to take a keyserver argument. In add-apt-repository, just create the instance like this:

    sp = SoftwareProperties(options=options)

I guess the only concern here is whether the `options` that SoftwareProperties takes is intended to be the command line options or something else. Since it's undocumented and untested, let's assume 'yes' although the use of self.options.massive_debug has me scratching my head. Anyway, I'd just pass in options to the constructor.

Now when you instantiate AddPPASigningKeyThread, pass in something like this:

    worker = AddPPASigningKeyThread(parsed_uri.path, options and options.keyserver)

Now in ppa.py, where you add a keyserver argument to AddPPASigningKeyThread.__init__(), use a default value of None instead of the hardcoded path, and do something like this in the __init__():

    self.keyserver = (keyserver if keyserver is not None else DEFAULT_KEYSERVER)

I hope that all makes sense!

I wish this were testable, because then I'd be sure to request that you add a test for your new code :). If you feel like giving it a shot, go for it, but as the rest of the code isn't well tested, I wouldn't block your branch on that.

Marking as needs-fixing for now, but I'm happy to review an update if you push it.

review: Needs Fixing (code)

[lp:~hodgestar/software-properties/configurable-key-server]

663. By Simon Cross <email address hidden> on 2011-05-02

Factor out default keyserver to a module global. Fix typo in --keyserver docstring. Allow passing in None for the keyserver to fallback to the default value.

664. By Simon Cross <email address hidden> on 2011-05-02

Move --keyserver into the generic 'options' holder used by SoftwareProperties. This also adds --massive-debug to add-apt-repository and --keyserver to software-properties-gtk and -kde to ensure that all scripts that use SoftwareProperties support the options it needs.

=== modified file 'add-apt-repository'
--- add-apt-repository	2010-12-07 10:20:45 +0000
+++ add-apt-repository	2011-05-02 22:01:28 +0000
@@ -6,7 +6,7 @@
 import locale
 
 from softwareproperties.SoftwareProperties import SoftwareProperties
-from softwareproperties.ppa import expand_ppa_line
+from softwareproperties.ppa import DEFAULT_KEYSERVER, expand_ppa_line
 from aptsources.sourceslist import SourceEntry
 from optparse import OptionParser
 from gettext import gettext as _
@@ -41,9 +41,15 @@
     parser = OptionParser(usage)
     # FIXME: provide a --sources-list-file= option that 
     #        puts the line into a specific file in sources.list.d
+    parser.add_option ("-m", "--massive-debug", action="store_true",
+        dest="massive_debug", default=False,
+        help="Print a lot of debug information to the command line")
     parser.add_option("-r", "--remove", action="store_true",
         dest="remove", default=False,
         help="remove repository from sources.list.d directory")
+    parser.add_option("-k", "--keyserver",
+        dest="keyserver", default=DEFAULT_KEYSERVER,
+        help="URL of keyserver. Default: %default")
     (options, args) = parser.parse_args()
 
     if os.geteuid() != 0:
@@ -57,7 +63,7 @@
     # force new ppa file to be 644 (LP: #399709)
     os.umask(0022)
 
-    sp = SoftwareProperties()	
+    sp = SoftwareProperties(options=options)
     line = args[0]
     if options.remove:
         (line, file) = expand_ppa_line(line.strip(), sp.distro.codename)
 
=== modified file 'software-properties-gtk'
--- software-properties-gtk	2011-04-05 13:07:35 +0000
+++ software-properties-gtk	2011-05-02 22:01:28 +0000
@@ -39,6 +39,7 @@
 #sys.path.append("@prefix@/share/update-manager/python")
 
 from softwareproperties.gtk.SoftwarePropertiesGtk import SoftwarePropertiesGtk
+from softwareproperties.ppa import DEFAULT_KEYSERVER
 
 if __name__ == "__main__":
   _ = gettext.gettext
@@ -70,6 +71,9 @@
   parser.add_option("--enable-ppa", "",
                     action="store", type="string", default=None,
                     help="Enable PPA with the given name")
+  parser.add_option("-k", "--keyserver",
+                    dest="keyserver", default=DEFAULT_KEYSERVER,
+                    help="URL of keyserver. Default: %default")
   parser.add_option("--data-dir", "",
                     action="store", type="string", default="/usr/share/software-properties/",
                     help="Use data files (UI) from the given directory")
 
=== modified file 'software-properties-kde'
--- software-properties-kde	2009-12-09 13:21:36 +0000
+++ software-properties-kde	2011-05-02 22:01:28 +0000
@@ -32,6 +32,7 @@
 #sys.path.append("@prefix@/share/update-manager/python")
 
 from softwareproperties.kde.SoftwarePropertiesKDE import SoftwarePropertiesKDE
+from softwareproperties.ppa import DEFAULT_KEYSERVER
 
 from PyKDE4.kdecore import ki18n, KAboutData, KCmdLineArgs , KCmdLineOptions
 from PyKDE4.kdeui import KApplication, KMessageBox
@@ -41,6 +42,7 @@
   massive_debug = False
   no_update = False
   enable_component = ""
+  keyserver = DEFAULT_KEYSERVER
 
 #--------------- main ------------------
 if __name__ == '__main__':
@@ -69,6 +71,7 @@
   opts.add ("dont-update", ki18n("No update on repository change (useful if called from an external program)"))
   opts.add ("enable-component <name>", ki18n("Enable the specified component of the distro's repositories"), "component_arg")
   opts.add ("enable-ppa <name>", ki18n("Enable PPA with the given name"), "ppa_arg")
+  opts.add ("keyserver <url>", ki18n("URL of keyserver"), "keyserver_url")
   opts.add ("attach <WinID>", ki18n("Win ID to act as a dialogue for"), "attach_arg")
   
   KCmdLineArgs.addCmdLineOptions(opts)
@@ -104,6 +107,8 @@
     options.no_update = True
   if args.isSet("attach") == True:
     attachWinID = args.getOption("attach")
+  if args.isSet("keyserver"):
+    options.keyserver = str(args.getOption("keyserver"))
 
   if args.isSet("enable-ppa"):
     app = SoftwarePropertiesKDE(datadir=data_dir, options=options, file=file)
 
=== modified file 'softwareproperties/SoftwareProperties.py'
--- softwareproperties/SoftwareProperties.py	2010-12-07 10:20:45 +0000
+++ softwareproperties/SoftwareProperties.py	2011-05-02 22:01:28 +0000
@@ -616,7 +616,7 @@
     # FIXME: abstract this all alway into the ppa.py file
     parsed_uri = urlparse(SourceEntry(srcline).uri)
     if parsed_uri.netloc == 'ppa.launchpad.net':
-      worker = AddPPASigningKeyThread(parsed_uri.path)
+      worker = AddPPASigningKeyThread(parsed_uri.path, self.options and self.options.keyserver)
       worker.start()
 
   def update_interface(self):
 
=== modified file 'softwareproperties/ppa.py'
--- softwareproperties/ppa.py	2010-12-13 08:42:40 +0000
+++ softwareproperties/ppa.py	2011-05-02 22:01:28 +0000
@@ -27,6 +27,8 @@
 from urllib2 import urlopen, Request, URLError
 from urlparse import urlparse
 
+DEFAULT_KEYSERVER = "hkp://keyserver.ubuntu.com:80/"
+
 def encode(s):
     return re.sub("[^a-zA-Z0-9_-]","_", s)
 
@@ -55,9 +57,11 @@
 class AddPPASigningKeyThread(Thread):
     " thread class for adding the signing key in the background "
 
-    def __init__(self, ppa_path):
+    def __init__(self, ppa_path, keyserver=None):
         Thread.__init__(self)
         self.ppa_path = ppa_path
+        self.keyserver = (keyserver if keyserver is not None
+                          else DEFAULT_KEYSERVER)
         
     def run(self):
         self.add_ppa_signing_key(self.ppa_path)
@@ -86,7 +90,7 @@
             #        the load
             res = subprocess.call(
                 ["apt-key", "adv", 
-                 "--keyserver", "hkp://keyserver.ubuntu.com:80/",
+                 "--keyserver", self.keyserver,
                  "--recv", signing_key_fingerprint[0]])
             return (res == 0)
         except URLError, e: