Merge lp:~andrea.corbellini/software-properties/fix-621977 into lp:software-properties

[Andrea Corbellini]

This branch fixes bug #621977 in the following way:

1. deb-src lines are added, but commented out, so that Software Properties still lets you enable them later;

2. a new -s, --enable-source command line option is added so that you can add uncommented deb-src line with a single command.

[Robert Roth (evfool)]

The patch looks fine with one minor detail: lots of unchanged lines are added to the diff (probably because tab/spaces usage - whitespace differences). It would be nice if you could set up your editor to use spaces instead of tabs (I think software-properties uses spaces instead of tabs) to avoid harder-to-review diffs, because the reviewer has to filter the real changes.

Other than that: nice solution to comment the source line by default, but leave an option to enable the source with the same command.

review: Approve

[Andrea Corbellini (andrea.corbellini)]

> The patch looks fine with one minor detail: lots of unchanged lines are added
> to the diff (probably because tab/spaces usage - whitespace differences). It
> would be nice if you could set up your editor to use spaces instead of tabs (I
> think software-properties uses spaces instead of tabs) to avoid harder-to-
> review diffs, because the reviewer has to filter the real changes.

Actually I'm using spaces for indentation. The problem with the diff is caused by the fact that my editor automatically removes trailing whitespace characters. Anyhow, sorry about that :-)

> Other than that: nice solution to comment the source line by default, but
> leave an option to enable the source with the same command.

Thanks!

[Barry Warsaw (barry)]

The diff looks fine, and while the whitespace cleanups are distracting, I personally appreciate all that trailing whitespace not hurting my eyes :)

I'll go ahead and sponsor this into Raring. Thanks for your contribution to Ubuntu!

review: Approve

[Barry Warsaw (barry)]

Whoops, this causes some test failures. I think they're shallow -- let's see.

[Barry Warsaw (barry)]

1 failure, 1 error:

======================================================================
ERROR: test_add_remove_source_by_line (__main__.TestDBus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_dbus.py", line 248, in test_add_remove_source_by_line
    self.iface.RemoveSource(s.replace("deb", "deb-src"))
  File "/usr/lib/python2.7/dist-packages/dbus/proxies.py", line 145, in __call__
    **keywords)
  File "/usr/lib/python2.7/dist-packages/dbus/connection.py", line 651, in call_blocking
    message, timeout)
DBusException: org.freedesktop.DBus.Python.AttributeError: Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/dbus/service.py", line 707, in _message_cb
    retval = candidate_method(self, *args, **keywords)
  File "../softwareproperties/dbus/SoftwarePropertiesDBus.py", line 278, in RemoveSource
    self.remove_source(_to_unicode(source))
  File "../softwareproperties/SoftwareProperties.py", line 741, in remove_source
    if source.file != apt_pkg.config.find_file("Dir::Etc::sourcelist"):
AttributeError: 'NoneType' object has no attribute 'file'

======================================================================
FAIL: test_enable_enable_disable_source_code_sources (__main__.TestDBus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_dbus.py", line 127, in test_enable_enable_disable_source_code_sources
    self.assertFalse("deb-src" in sourceslist)
AssertionError: True is not false

So I think before this is merged you have to decide how to handle this. Marking this Needs Fixing for now.

review: Needs Fixing

[Andrea Corbellini (andrea.corbellini)]

I've submitted a newer, cleaner branch for review:

  https://code.launchpad.net/~andrea.corbellini/software-properties/fix-621977-1/+merge/139919

review: Needs Resubmitting

Unmerged revisions

822. By Andrea Corbellini on 2012-11-18

Do not enable deb-src lines by default.

=== modified file 'add-apt-repository'
--- add-apt-repository	2012-11-09 12:21:58 +0000
+++ add-apt-repository	2012-11-18 19:27:20 +0000
@@ -33,7 +33,7 @@
             if len(user_inst.ppas) > 0:
                 # Translators: %(entity)s is either "team" or "user"
                 print(_("The %(entity)s named '%(user)s' has no PPA named '%(ppa)s'") % {
-                        'entity' : entity_name, 
+                        'entity' : entity_name,
                          'user' : user,
                          'ppa' : ppa_name})
                 print(_("Please choose from the following available PPAs:"))
@@ -44,7 +44,7 @@
             else:
                 # Translators: %(entity)s is either "team" or "user"
                 print(_("The %(entity)s named '%(user)s' does not have any PPA") % {
-                        'entity' : entity_name, 
+                        'entity' : entity_name,
                          'user' : user})
         except KeyError:
             pass
@@ -64,13 +64,13 @@
     gettext.textdomain("software-properties")
     usage = """Usage: %prog <sourceline>
 
-%prog is a script for adding apt sources.list entries. 
-It can be used to add any repository and also provides a shorthand 
+%prog is a script for adding apt sources.list entries.
+It can be used to add any repository and also provides a shorthand
 syntax for adding a Launchpad PPA (Personal Package Archive)
 repository.
 
 <sourceline> - The apt repository source line to add. This is one of:
-  a complete apt line in quotes, 
+  a complete apt line in quotes,
   a repo url and areas in quotes (areas defaults to 'main')
   a PPA shortcut.
   a distro component
@@ -79,7 +79,7 @@
     apt-add-repository 'deb http://myserver/path/to/repo stable myrepo'
     apt-add-repository 'http://myserver/path/to/repo myrepo'
     apt-add-repository 'https://packages.medibuntu.org free non-free'
-    apt-add-repository http://extras.ubuntu.com/ubuntu 
+    apt-add-repository http://extras.ubuntu.com/ubuntu
     apt-add-repository ppa:user/repository
     apt-add-repository multiverse
 
@@ -87,7 +87,7 @@
 sources.list
 """
     parser = OptionParser(usage)
-    # FIXME: provide a --sources-list-file= option that 
+    # 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,
@@ -98,6 +98,9 @@
     parser.add_option("-k", "--keyserver",
         dest="keyserver", default=DEFAULT_KEYSERVER,
         help=_("URL of keyserver. Default: %default"))
+    parser.add_option("-s", "--enable-source", action="store_true",
+        dest="enable_source", default=False,
+        help=_("Allow downloading of the source packages from the repository"))
     parser.add_option("-y", "--yes", action="store_true",
         dest="assume_yes", default=False,
         help=_("Assume yes to all queries"))
@@ -129,7 +132,7 @@
             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 
+            # 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)
@@ -142,7 +145,7 @@
         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:
@@ -198,7 +201,7 @@
             print(_("Error: '%s' doesn't exist in a sourcelist file") % debsrc_line)
 
     else:
-        if not sp.add_source_from_line(line):
+        if not sp.add_source_from_line(line, options.enable_source):
             print(_("Error: '%s' invalid") % line)
             sys.exit(1)
         sp.sourceslist.save()
 
=== modified file 'softwareproperties/SoftwareProperties.py'
--- softwareproperties/SoftwareProperties.py	2012-10-01 09:24:55 +0000
+++ softwareproperties/SoftwareProperties.py	2012-11-18 19:27:20 +0000
@@ -78,10 +78,10 @@
     RELEASE_UPGRADES_LTS    : 'lts',
     RELEASE_UPGRADES_NEVER  : 'never',
   }
-  
+
   def __init__(self, datadir=None, options=None, rootdir="/"):
-    """ Provides the core functionality to configure the used software 
-        repositories, the corresponding authentication keys and 
+    """ Provides the core functionality to configure the used software
+        repositories, the corresponding authentication keys and
         update automation """
     self.popconfile = rootdir+"/etc/popularity-contest.conf"
 
@@ -97,7 +97,7 @@
 
     self.sourceslist = SourcesList()
     self.distro = aptsources.distro.get_distro()
-    
+
     self.seen_server = []
     self.modified_sourceslist = False
 
@@ -136,8 +136,8 @@
     self.write_config()
 
   def get_update_automation_level(self):
-    """ Parse the apt cron configuration. Try to fit a predefined use case 
-        and return it. Special case: if the user made a custom 
+    """ Parse the apt cron configuration. Try to fit a predefined use case
+        and return it. Special case: if the user made a custom
         configurtation, that we cannot represent it will return None """
     if apt_pkg.config.find_i(softwareproperties.CONF_MAP["autoupdate"]) > 0:
         # Autodownload
@@ -159,9 +159,9 @@
         return None
 
   def set_update_automation_level(self, state):
-    """ Set the apt periodic configurtation to the selected 
-        update automation level. To synchronize the cache update and the 
-        actual upgrading function, the upgrade function, e.g. unattended, 
+    """ Set the apt periodic configurtation to the selected
+        update automation level. To synchronize the cache update and the
+        actual upgrading function, the upgrade function, e.g. unattended,
         will run every day, if enabled. """
     if state == softwareproperties.UPDATE_INST_SEC:
         apt_pkg.config.set(softwareproperties.CONF_MAP["unattended"], str(1))
@@ -234,8 +234,8 @@
     return True
 
   def get_popcon_participation(self):
-    """ Will return True if the user wants to participate in the popularity 
-        contest. Otherwise it will return False. Special case: if no 
+    """ Will return True if the user wants to participate in the popularity
+        contest. Otherwise it will return False. Special case: if no
         popcon is installed it will return False """
     if os.path.exists(self.popconfile):
         lines = open(self.popconfile).read().split("\n")
@@ -275,10 +275,10 @@
     open(self.popconfile, "w").writelines(lines)
 
   def get_source_code_state(self):
-    """Return True if all distro componets are also available as 
+    """Return True if all distro componets are also available as
        source code. Otherwise return Flase. Special case: If the
        configuration cannot be represented return None"""
-  
+
     if len(self.distro.source_code_sources) < 1:
         # we don't have any source code sources, so
         # uncheck the button
@@ -327,7 +327,7 @@
     print("\n")
 
   def massive_debug_output(self):
-    """Print the complete sources.list""" 
+    """Print the complete sources.list"""
     print("START SOURCES.LIST:")
     for source in self.sourceslist:
         print(source.str())
@@ -342,12 +342,12 @@
 
   def enable_component(self, comp):
     """Enable a component of the distro"""
-    self.distro.enable_component(comp) 
+    self.distro.enable_component(comp)
     self.set_modified_sourceslist()
 
   def disable_component(self, comp):
     """Disable a component of the distro"""
-    self.distro.disable_component(comp) 
+    self.distro.disable_component(comp)
     self.set_modified_sourceslist()
 
   def _find_template_from_string(self, name):
@@ -391,7 +391,7 @@
     for source in self.distro.source_code_sources:
         self.sourceslist.remove(source)
     self.set_modified_sourceslist()
-  
+
   def enable_source_code_sources(self):
     """Enable source code source for all distro sources"""
     sources = []
@@ -426,7 +426,7 @@
     for source in self.sourceslist.list:
         source_bkp = SourceEntry(line=source.line,file=source.file)
         self.sourceslist_backup.append(source_bkp)
-  
+
   def _find_source_from_string(self, line):
     # ensure that we have a current list, it might have been changed underneath
     # us
@@ -456,7 +456,7 @@
     return False
 
   def revert(self):
-    """Revert all settings to the state when software-properties 
+    """Revert all settings to the state when software-properties
        was launched"""
     #FIXME: GPG keys are still missing
     self.restore_apt_conf()
@@ -557,7 +557,7 @@
   def get_cdrom_sources(self):
     """Return the list of CDROM based distro sources"""
     return self.distro.cdrom_sources
-      
+
   def get_comp_download_state(self, comp):
     """Return a tuple: the first value describes if a component is enabled
        in the Internet repositories. The second value describes if the
@@ -567,7 +567,7 @@
 
   def get_comp_child_state(self, template):
     """Return a tuple: the first value describes if a component is enabled
-       in one of the child source that matcth the given template. 
+       in one of the child source that matcth the given template.
        The second value describes if the first value is inconsistent."""
     comps = []
     for child in self.distro.child_sources:
@@ -579,17 +579,17 @@
         return (True, False)
     elif len(comps) > 0 and\
         len(self.distro.enabled_comps ^ set(comps)) != 0:
-        # A matching child source does exist but doesn't include all 
+        # A matching child source does exist but doesn't include all
         # enabled distro components
         return(False, True)
     else:
         # There is no corresponding child source at all
         return (False, False)
-  
+
   def reload_sourceslist(self):
     self.sourceslist.refresh()
     self.sourceslist_visible=[]
-    self.distro.get_sources(self.sourceslist)    
+    self.distro.get_sources(self.sourceslist)
 
   def write_config(self):
     """Write the current apt configuration to file"""
@@ -634,10 +634,10 @@
         # and append the updated keys
         for i in cnf.list():
           f.write("APT::Periodic::%s \"%s\";\n" % (i, cnf.find_i(i)))
-        f.close()    
+        f.close()
 
   def save_sourceslist(self):
-    """Backup the existing sources.list files and write the current 
+    """Backup the existing sources.list files and write the current
        configuration"""
     self.sourceslist.backup(".save")
     self.sourceslist.save()
@@ -647,7 +647,7 @@
     helper that checks if a given line is in the source list
     return the channel name or None if not found
     """
-    srcentry = SourceEntry(srcline)    
+    srcentry = SourceEntry(srcline)
     if os.path.exists(self.CHANNEL_PATH):
       for f in glob.glob("%s/*.list" % self.CHANNEL_PATH):
         for line in open(f):
@@ -696,14 +696,15 @@
     line = "deb %s %s %s" % ( repo, self.distro.codename, areas )
     return line
 
-  def add_source_from_line(self, line):
+  def add_source_from_line(self, line, enable_debsrc=False):
     """
     Add a source with the given apt line and auto-add
     signing key if we have it in the whitelist
     """
     (deb_line, file) = expand_ppa_line(line.strip(), self.distro.codename)
     deb_line = self.expand_http_line(deb_line)
-    debsrc_line = 'deb-src' + deb_line[3:]
+    debsrc_entry_type = 'deb-src' if enable_debsrc else '#deb-src'
+    debsrc_line = debsrc_entry_type + deb_line[3:]
     new_deb_entry = SourceEntry(deb_line, file)
     new_debsrc_entry = SourceEntry(debsrc_line, file)
     if new_deb_entry.invalid or new_debsrc_entry.invalid:
@@ -716,7 +717,7 @@
                          comment=new_deb_entry.comment,
                          file=new_deb_entry.file,
                          architectures=new_deb_entry.architectures)
-    self.sourceslist.add(new_debsrc_entry.type,
+    self.sourceslist.add(debsrc_entry_type,
                          new_debsrc_entry.uri,
                          new_debsrc_entry.dist,
                          new_debsrc_entry.comps,