Merge lp:~brian-murray/ubuntu-release-upgrader/patching-get-distro into lp:ubuntu-release-upgrader

[Brian Murray]

I want to update the code that only adds new mirrors to ubuntu-release-upgrader to also remove mirrors. However, if that change is made and you have an obsolete mirror you'll be left with a sources.list file that only contains main for the destination release. This'll result in a bad upgrade experience. See the linked bug for details.

I've modified the release upgrader to copy the disabled entries in sources.list and replace the server uri so that the upgrade will proceed and not leave a bunch of packages unupgraded. I've added a test, test_sources_list_inactive_mirror, for this.

[Barry Warsaw (barry)]

LGTM, with just a few style nits. However, there's a merge conflict that needs to be fixed first.

review: Needs Fixing

[lp:~brian-murray/ubuntu-release-upgrader/patching-get-distro]

2942. By Brian Murray on 2015-11-18

nitpick changes based off reviewer feedback

2943. By Brian Murray on 2015-11-18

Continue to leave BaseMetaPkgs commented out in the test version of DistUpgrade.cfg as it breaks other tests, set BaseMetaPkgs in the tests that require it.

2944. By Brian Murray on 2015-11-18

clarify notes regarding old code

=== modified file 'DistUpgrade/DistUpgradeController.py'
--- DistUpgrade/DistUpgradeController.py	2015-10-27 01:51:21 +0000
+++ DistUpgrade/DistUpgradeController.py	2015-11-18 18:42:17 +0000
@@ -506,14 +506,17 @@
         logging.debug("verifySourcesListEntry: %s" % entry)
         # no way to verify without network
         if not self.useNetwork:
-            logging.debug("skiping downloadable check (no network)")
+            logging.debug("skipping downloadable check (no network)")
             return True
         # check if the entry points to something we can download
         uri = "%s/dists/%s/Release" % (entry.uri, entry.dist)
         return url_downloadable(uri, logging.debug)
 
     def rewriteSourcesList(self, mirror_check=True):
-        logging.debug("rewriteSourcesList()")
+        if mirror_check:
+            logging.debug("rewriteSourcesList() with mirror_check")
+        else:
+            logging.debug("rewriteSourcesList()")
 
         sync_components = self.config.getlist("Sources","Components")
 
@@ -525,12 +528,15 @@
             mirror_check=False
 
         # check if we need to enable main
+        main_was_missing = False
         if mirror_check == True and self.useNetwork:
             # now check if the base-meta pkgs are available in
             # the archive or only available as "now"
             # -> if not that means that "main" is missing and we
-            #    need to  enable it
-            for pkgname in self.config.getlist("Distro","BaseMetaPkgs"):
+            #    need to enable it
+            logging.debug(self.config.getlist("Distro", "BaseMetaPkgs"))
+            for pkgname in self.config.getlist("Distro", "BaseMetaPkgs"):
+                logging.debug("Checking pkg: %s" % pkgname)
                 if ((not pkgname in self.cache or
                      not self.cache[pkgname].candidate or
                      len(self.cache[pkgname].candidate.origins) == 0)
@@ -544,17 +550,24 @@
                         distro = get_distro()
                         distro.get_sources(self.sources)
                         distro.enable_component("main")
-                    except NoDistroTemplateException:
+                        main_was_missing = True
+                    except NoDistroTemplateException as e:
+                        logging.exception('NoDistroTemplateException raised: %s' % e)
                         # fallback if everything else does not work,
-                        # we replace the sources.list with a single
-                        # line to ubuntu-main
-                        logging.warning('get_distro().enable_component("main") failed, overwriting sources.list instead as last resort')
-                        s =  "# auto generated by ubuntu-release-upgrader"
-                        s += "deb http://archive.ubuntu.com/ubuntu %s main restricted" % self.toDist
-                        s += "deb http://archive.ubuntu.com/ubuntu %s-updates main restricted" % self.toDist
-                        s += "deb http://security.ubuntu.com/ubuntu %s-security main restricted" % self.toDist
-                        with open("/etc/apt/sources.list","w") as f:
-                            f.write(s)
+                        # we replace the sources.list with lines to
+                        # main and restricted
+                        logging.debug('get_distro().enable_component("main") failed, overwriting sources.list instead as last resort')
+                        comment = " auto generated by ubuntu-release-upgrader"
+                        comps = ["main", "restricted"]
+                        uri = "http://archive.ubuntu.com/ubuntu"
+                        self.sources.add("deb", uri, self.toDist, comps,
+                                         comment)
+                        self.sources.add("deb", uri, self.toDist+"-updates",
+                                         comps, comment)
+                        self.sources.add("deb",
+                                         "http://security.ubuntu.com/ubuntu",
+                                         self.toDist+"-security", comps,
+                                         comment)
                     break
 
         # this must map, i.e. second in "from" must be the second in "to"
@@ -609,7 +622,7 @@
                                        "dists",
                                        entry.dist,
                                        "Release")):
-                    logging.warning("disabling cdrom source '%s' because it has no  Release file" % entry)
+                    logging.warning("disabling cdrom source '%s' because it has no Release file" % entry)
                     entry.disabled = True
                 continue
 
@@ -626,6 +639,7 @@
 
             # special case for landscape.canonical.com because they
             # don't use a standard archive layout (gutsy->hardy)
+            # XXX - Is this still relevant?
             if (not entry.disabled and
                 entry.uri.startswith("http://landscape.canonical.com/packages/%s" % self.fromDist)):
                 logging.debug("commenting landscape.canonical.com out")
@@ -693,8 +707,8 @@
                     self.sources_disabled = True
                     logging.debug("entry '%s' was disabled (unknown dist)" % get_string_with_no_auth_from_source_entry(entry))
 
-                # if we make it to this point, we have a official or third-party mirror
-
+                # if we make it to this point, we have an official or third-party mirror
+                # XXX - is this still relevant?
                 # check if the arch is powerpc or sparc and if so, transition
                 # to ports.ubuntu.com (powerpc got demoted in gutsy, sparc
                 # in hardy)
@@ -710,7 +724,7 @@
                           "%s-security" % self.toDist]:
                     # create entry if needed, ignore disabled
                     # entries and deb-src
-                    self.found_components.setdefault(d,set())
+                    self.found_components.setdefault(d, set())
                     if (not entry.disabled and entry.dist == d and
                         entry.type == "deb"):
                         for comp in entry.comps:
@@ -731,12 +745,34 @@
                 entry.disabled = True
                 self.sources_disabled = True
                 logging.debug("entry '%s' was disabled (unknown mirror)" % get_string_with_no_auth_from_source_entry(entry))
+                # if its not a valid mirror and we manually added main, be
+                # nice and add pockets and components corresponding to what we
+                # disabled.
+                if main_was_missing:
+                    if entry.dist in fromDists:
+                        entry.dist = toDists[fromDists.index(entry.dist)]
+                    # gather what components are enabled and are inconsistent
+                    for d in ["%s" % self.toDist,
+                              "%s-updates" % self.toDist,
+                              "%s-security" % self.toDist]:
+                        # create entry if needed, ignore deb-src entries
+                        self.found_components.setdefault(d, set())
+                        if entry.dist == d and entry.type == "deb":
+                            for comp in entry.comps:
+                                # only sync components we know about
+                                if not comp in sync_components:
+                                    continue
+                                self.found_components[d].add(comp)
+                    logging.debug("Adding entry: %s %s %s" % (entry.type, entry.dist, entry.comps))
+                    uri = "http://archive.ubuntu.com/ubuntu"
+                    comment = " auto generated by ubuntu-release-upgrader"
+                    self.sources.add(entry.type, uri, entry.dist, entry.comps, comment)
 
         # now go over the list again and check for missing components
         # in $dist-updates and $dist-security and add them
         for entry in self.sources.list[:]:
             # skip all comps that are not relevant (including e.g. "hardy")
-            if (entry.invalid or entry.disabled or entry.type == "deb-src" or 
+            if (entry.invalid or entry.disabled or entry.type == "deb-src" or
                 entry.uri.startswith("cdrom:") or entry.dist == self.toDist):
                 continue
             # now check for "$dist-updates" and "$dist-security" and add any inconsistencies

=== added file 'tests/data-sources-list-test/sources.list.obsolete_mirror'
--- tests/data-sources-list-test/sources.list.obsolete_mirror	1970-01-01 00:00:00 +0000
+++ tests/data-sources-list-test/sources.list.obsolete_mirror	2015-11-18 18:42:17 +0000
@@ -0,0 +1,10 @@
+
+# main repo
+deb http://mirror.mcs.anl.gov/ubuntu feisty main restricted universe multiverse
+deb http://mirror.mcs.anl.gov/ubuntu feisty-updates main restricted universe multiverse
+#deb http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+deb http://mirror.mcs.anl.gov/ubuntu feisty-security main restricted universe multiverse
+deb-src http://mirror.mcs.anl.gov/ubuntu feisty main restricted universe multiverse
+deb-src http://mirror.mcs.anl.gov/ubuntu feisty-updates main restricted universe multiverse
+##deb-src http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+deb-src http://mirror.mcs.anl.gov/ubuntu feisty-security main restricted universe multiverse

=== modified file 'tests/test_sources_list.py'
--- tests/test_sources_list.py	2015-01-20 22:30:06 +0000
+++ tests/test_sources_list.py	2015-11-18 18:42:17 +0000
@@ -18,6 +18,10 @@
 )
 from DistUpgrade import DistUpgradeConfigParser
 from DistUpgrade.utils import url_downloadable
+from DistUpgrade.distro import (
+    UbuntuDistribution,
+    NoDistroTemplateException
+)
 import logging
 import mock
 
@@ -87,7 +91,8 @@
 deb http://security.ubuntu.com/ubuntu/ gutsy-security main restricted
 """)
 
-    def test_sources_list_rewrite(self):
+    @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
+    def test_sources_list_rewrite(self, mock_get_distro):
         """
         test regular sources.list rewrite
         """
@@ -96,8 +101,13 @@
         apt_pkg.config.set("Dir::Etc::sourcelist", "sources.list")
         v = DistUpgradeViewNonInteractive()
         d = DistUpgradeController(v, datadir=self.testdir)
+        d.config.set("Distro", "BaseMetaPkgs", "ubuntu-minimal")
+        mock_get_distro.return_value = UbuntuDistribution("Ubuntu", "feisty",
+                                                          "Ubuntu Feisty Fawn",
+                                                          "7.04")
         d.openCache(lock=False)
         res = d.updateSourcesList()
+        self.assertTrue(mock_get_distro.called)
         self.assertTrue(res)
 
         # now test the result
@@ -117,6 +127,90 @@
              apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".distUpgrade"
              ]))
 
+    @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
+    def test_sources_list_inactive_mirror(self, mock_get_distro):
+        """
+        test sources.list rewrite of an obsolete mirror
+        """
+        shutil.copy(os.path.join(self.testdir, "sources.list.obsolete_mirror"),
+                    os.path.join(self.testdir, "sources.list"))
+        apt_pkg.config.set("Dir::Etc::sourcelist", "sources.list")
+        v = DistUpgradeViewNonInteractive()
+        d = DistUpgradeController(v, datadir=self.testdir)
+        d.config.set("Distro", "BaseMetaPkgs", "ubuntu-minimal")
+        mock_get_distro.return_value = UbuntuDistribution("Ubuntu", "feisty",
+                                                          "Ubuntu Feisty Fawn",
+                                                          "7.04")
+        d.openCache(lock=False)
+        res = d.updateSourcesList()
+        self.assertTrue(mock_get_distro.called)
+        self.assertTrue(res)
+
+        # now test the result
+        #print(open(os.path.join(self.testdir,"sources.list")).read())
+        self._verifySources("""
+# main repo
+# deb http://mirror.mcs.anl.gov/ubuntu gutsy main restricted universe multiverse # disabled on upgrade to gutsy
+# deb http://mirror.mcs.anl.gov/ubuntu gutsy-updates main restricted universe multiverse # disabled on upgrade to gutsy
+# deb http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+# deb http://mirror.mcs.anl.gov/ubuntu gutsy-security main restricted universe multiverse # disabled on upgrade to gutsy
+# deb-src http://mirror.mcs.anl.gov/ubuntu gutsy main restricted universe multiverse # disabled on upgrade to gutsy
+# deb-src http://mirror.mcs.anl.gov/ubuntu gutsy-updates main restricted universe multiverse # disabled on upgrade to gutsy
+##deb-src http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+deb http://archive.ubuntu.com/ubuntu/ gutsy main
+deb http://archive.ubuntu.com/ubuntu gutsy main restricted universe multiverse # auto generated by ubuntu-release-upgrader
+deb http://archive.ubuntu.com/ubuntu gutsy-updates main restricted universe multiverse # auto generated by ubuntu-release-upgrader
+deb http://archive.ubuntu.com/ubuntu gutsy-security main restricted universe multiverse # auto generated by ubuntu-release-upgrader
+# deb-src http://mirror.mcs.anl.gov/ubuntu gutsy-security main restricted universe multiverse # disabled on upgrade to gutsy
+""")
+        # check that the backup file was created correctly
+        self.assertEqual(0, subprocess.call(
+            ["cmp",
+             apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".obsolete_mirror",
+             apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".distUpgrade"
+             ]))
+
+    @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
+    def test_sources_list_no_template(self, mock_get_distro):
+        """
+        test sources.list rewrite when there is no distro template
+        """
+        shutil.copy(os.path.join(self.testdir, "sources.list.obsolete_mirror"),
+                    os.path.join(self.testdir, "sources.list"))
+        apt_pkg.config.set("Dir::Etc::sourcelist", "sources.list")
+        v = DistUpgradeViewNonInteractive()
+        d = DistUpgradeController(v, datadir=self.testdir)
+        d.config.set("Distro", "BaseMetaPkgs", "ubuntu-minimal")
+        mock_get_distro.side_effect = NoDistroTemplateException("No distro template.")
+        d.openCache(lock=False)
+        res = d.updateSourcesList()
+        self.assertTrue(mock_get_distro.called)
+        self.assertTrue(res)
+
+        # now test the result
+        #print(open(os.path.join(self.testdir,"sources.list")).read())
+        self._verifySources("""
+# main repo
+# deb http://mirror.mcs.anl.gov/ubuntu gutsy main restricted universe multiverse # disabled on upgrade to gutsy
+# deb http://mirror.mcs.anl.gov/ubuntu feisty-updates main restricted universe multiverse # disabled on upgrade to gutsy
+# deb http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+# deb http://mirror.mcs.anl.gov/ubuntu feisty-security main restricted universe multiverse # disabled on upgrade to gutsy
+# deb-src http://mirror.mcs.anl.gov/ubuntu gutsy main restricted universe multiverse # disabled on upgrade to gutsy
+# deb-src http://mirror.mcs.anl.gov/ubuntu feisty-updates main restricted universe multiverse # disabled on upgrade to gutsy
+##deb-src http://mirror.mcs.anl.gov/ubuntu feisty-proposed main restricted universe multiverse
+deb http://archive.ubuntu.com/ubuntu gutsy main restricted # auto generated by ubuntu-release-upgrader
+deb http://archive.ubuntu.com/ubuntu gutsy-updates main restricted # auto generated by ubuntu-release-upgrader
+deb http://security.ubuntu.com/ubuntu gutsy-security main restricted # auto generated by ubuntu-release-upgrader
+# deb-src http://mirror.mcs.anl.gov/ubuntu feisty-security main restricted universe multiverse # disabled on upgrade to gutsy
+""")
+
+        # check that the backup file was created correctly
+        self.assertEqual(0, subprocess.call(
+            ["cmp",
+             apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".obsolete_mirror",
+             apt_pkg.config.find_file("Dir::Etc::sourcelist") + ".distUpgrade"
+             ]))
+
     def test_commercial_transition(self):
         """
         test transition of pre-gutsy archive.canonical.com archives
@@ -137,7 +231,8 @@
 deb http://archive.canonical.com/ubuntu gutsy partner
 """)
 
-    def test_extras_removal(self):
+    @mock.patch("DistUpgrade.DistUpgradeController.get_distro")
+    def test_extras_removal(self, mock_get_distro):
         """
         test removal of extras.ubuntu.com archives
         """
@@ -149,6 +244,9 @@
                            os.path.join(self.testdir, "sources.list.d"))
         v = DistUpgradeViewNonInteractive()
         d = DistUpgradeController(v, datadir=self.testdir)
+        mock_get_distro.return_value = UbuntuDistribution("Ubuntu", "feisty",
+                                                          "Ubuntu Feisty Fawn",
+                                                          "7.04")
         d.openCache(lock=False)
         res = d.updateSourcesList()
         self.assertTrue(res)
@@ -252,7 +350,7 @@
         "http://us.archive.ubuntu.com/ubuntu", logging.debug),
         "Could not reach mirror")
     def testEOL2SupportedWithMirrorUpgrade(self):
-        " test upgrade from a EOL release to a supported release with mirror"
+        " test upgrade from a EOL release to a supported release with mirror "
         # Use us.archive.ubuntu.com, because it is available in Canonical's
         # data center, unlike most mirrors.  This lets this test pass when
         # when run in their Jenkins test environment.
@@ -387,7 +485,7 @@
 
     def test_unicode_comments(self):
         """
-        test transition of apt-cacher/apt-torrent uris
+        test a sources.list file with unicode comments
         """
         shutil.copy(os.path.join(self.testdir, "sources.list.unicode"),
                     os.path.join(self.testdir, "sources.list"))