Merge lp:~bjornt/landscape-client/apt-upgrade-all-packages into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Ask Apt to upgrade the whole system, rather than marking each package
for upgrade when the server requests a general system upgrade. This
makes the system upgrade holds for dependencies.

I'm making a dist-upgrade, because the existing tests require it. It's
easy to change, if we want a normal apt-get upgrade.

[Free Ekanayaka (free.ekanayaka)]

I believe we don't want dist-upgrade, as it can have quite disruptive effects like removing packages, or replacing postgres-8 with postgres-9 (in principle).

Also I didn't look at the code very in depth, but will this have really the same behavior as before? The important thing is to go back to the server if there are package changes that haven't been explicitly requested by the server (with install/remove messages) and that are not allowed by the auto-apply policy in effect.

review: Needs Information

[lp:~bjornt/landscape-client/apt-upgrade-all-packages]

435. By Björn Tillenius on 2012-01-16

Merge trunk, resolve conflicts.

[Björn Tillenius (bjornt)]

We can change it to use upgrade instead of dist-upgrade. However, I do think that the current Smart behavior is like dist-upgrade. I will do some testing to confirm, it's easy to change in another branch.

And yes, it will tell the changes to the server. cache.upgrade() doesn't upgrade the packages. You have to call cache.commit() for the changes to take place. But since the changes aren't in _package_installs/_package_removals, a DependencyError is raised before the commit.

[Free Ekanayaka (free.ekanayaka)]

Thanks for explaining Bjorn, there's even a test raising DependencyError indeed.

So I guess the current Smart behavior is like dist-ugprade because we use PolicyUpgrade? I don't know the details of that, it might indeed be similar to dist-upgrade. However, dist-upgrade is really meant for cross-release upgrades ("distribution upgrades") not for regular updates, and we have separate machinery for release upgrades, so I'd really advocate for a plain apt-get upgrade.

Looks great, +1!

review: Approve

[Alberto Donato (ack)]

looks good! +1

review: Approve

=== modified file 'landscape/package/changer.py'
--- landscape/package/changer.py	2011-12-13 10:29:04 +0000
+++ landscape/package/changer.py	2012-01-16 15:12:26 +0000
@@ -169,9 +169,7 @@
             self._facade.reset_marks()
 
         if upgrade:
-            for package in self._facade.get_packages():
-                if self._facade.is_package_installed(package):
-                    self._facade.mark_upgrade(package)
+            self._facade.mark_global_upgrade()
 
         for ids, mark_func in [(install, self._facade.mark_install),
                                  (remove, self._facade.mark_remove)]:
 
=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2012-01-13 09:37:40 +0000
+++ landscape/package/facade.py	2012-01-16 15:12:26 +0000
@@ -90,7 +90,7 @@
         self._pkg2hash = {}
         self._hash2pkg = {}
         self._package_installs = []
-        self._package_upgrades = []
+        self._global_upgrade = False
         self._package_removals = []
         self.refetch_package_index = False
 
@@ -363,7 +363,7 @@
         held_package_names = set()
         package_changes = self._package_installs[:]
         package_changes.extend(self._package_removals)
-        if not package_changes and not self._package_upgrades:
+        if not package_changes and not self._global_upgrade:
             return None
         fixer = apt_pkg.ProblemResolver(self._cache._depcache)
         for version in self._package_installs:
@@ -377,14 +377,8 @@
             # been True.
             fixer.clear(version.package._pkg)
             fixer.protect(version.package._pkg)
-        for version in self._package_upgrades:
-            if self._is_package_held(version.package):
-                continue
-            version.package.mark_install(
-                auto_fix=False,
-                from_user=not version.package.is_auto_installed)
-            fixer.clear(version.package._pkg)
-            fixer.protect(version.package._pkg)
+        if self._global_upgrade:
+            self._cache.upgrade(dist_upgrade=True)
         for version in self._package_removals:
             if self._is_package_held(version.package):
                 held_package_names.add(version.package.name)
@@ -448,17 +442,16 @@
     def reset_marks(self):
         """Clear the pending package operations."""
         del self._package_installs[:]
-        del self._package_upgrades[:]
         del self._package_removals[:]
+        self._global_upgrade = False
 
     def mark_install(self, version):
         """Mark the package for installation."""
         self._package_installs.append(version)
 
-    def mark_upgrade(self, version):
-        """Mark the package for upgrade."""
-        if version.package.candidate != version:
-            self._package_upgrades.append(version)
+    def mark_global_upgrade(self):
+        """Upgrade all installed packages."""
+        self._global_upgrade = True
 
     def mark_remove(self, version):
         """Mark the package for removal."""
@@ -626,6 +619,12 @@
     def mark_upgrade(self, pkg):
         self._marks[pkg] = UPGRADE
 
+    def mark_global_upgrade(self):
+        """Upgrade all installed packages."""
+        for package in self.get_packages():
+            if self.is_package_installed(package):
+                self.mark_upgrade(package)
+
     def reset_marks(self):
         self._marks.clear()
 
 
=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2012-01-16 14:12:40 +0000
+++ landscape/package/tests/test_facade.py	2012-01-16 15:12:26 +0000
@@ -905,14 +905,13 @@
         self.facade.reload_channels()
         foo = self.facade.get_packages_by_name("foo")[0]
         self.facade.mark_install(foo)
-        bar_10 = sorted(self.facade.get_packages_by_name("bar"))[0]
-        self.facade.mark_upgrade(bar_10)
+        self.facade.mark_global_upgrade()
         [baz] = self.facade.get_packages_by_name("baz")
         self.facade.mark_remove(baz)
         self.facade.reset_marks()
         self.assertEqual(self.facade._package_installs, [])
-        self.assertEqual(self.facade._package_upgrades, [])
         self.assertEqual(self.facade._package_removals, [])
+        self.assertFalse(self.facade._global_upgrade)
         self.assertEqual(self.facade.perform_changes(), None)
 
     def test_wb_mark_install_adds_to_list(self):
@@ -930,10 +929,10 @@
         install = self.facade._package_installs[0]
         self.assertEqual("minimal", install.package.name)
 
-    def test_wb_mark_upgrade_adds_to_list(self):
+    def test_wb_mark_global_upgrade_sets_variable(self):
         """
-        C{mark_upgrade} adds the package to the list of packages to be
-        upgraded.
+        C{mark_global_upgrade} sets a variable, so that the actual
+        upgrade happens in C{perform_changes}.
         """
         deb_dir = self.makeDir()
         self._add_system_package("foo", version="1.0")
@@ -941,8 +940,9 @@
         self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
         self.facade.reload_channels()
         foo_10 = sorted(self.facade.get_packages_by_name("foo"))[0]
-        self.facade.mark_upgrade(foo_10)
-        self.assertEqual([foo_10], self.facade._package_upgrades)
+        self.facade.mark_global_upgrade()
+        self.assertTrue(self.facade._global_upgrade)
+        self.assertEqual(foo_10, foo_10.package.installed)
 
     def test_wb_mark_remove_adds_to_list(self):
         """
@@ -1062,7 +1062,27 @@
              ("single-arch", "2.0")],
             sorted(changes))
 
-    def test_mark_upgrade_candidate_version(self):
+    def test_mark_global_upgrade(self):
+        """
+        C{mark_global_upgrade} upgrades all packages that can be
+        upgraded. It makes C{perform_changes} raise a C{DependencyError}
+        with the required changes, so that the user can review the
+        changes and approve them.
+        """
+        deb_dir = self.makeDir()
+        self._add_system_package("foo", version="1.0")
+        self._add_system_package("bar")
+        self._add_package_to_deb_dir(deb_dir, "foo", version="2.0")
+        self._add_package_to_deb_dir(deb_dir, "baz")
+        self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
+        self.facade.reload_channels()
+        foo2 = sorted(self.facade.get_packages_by_name("foo"))[1]
+        self.facade.mark_global_upgrade()
+        exception = self.assertRaises(
+            DependencyError, self.facade.perform_changes)
+        self.assertEqual([foo2], exception.packages)
+
+    def test_mark_global_upgrade_candidate_version(self):
         """
         If more than one version is available, the package will be
         upgraded to the candidate version. Since the user didn't request
@@ -1077,16 +1097,16 @@
         self.facade.reload_channels()
         foo1, foo2, foo3 = sorted(self.facade.get_packages_by_name("foo"))
         self.assertEqual(foo3, foo1.package.candidate)
-        self.facade.mark_upgrade(foo1)
+        self.facade.mark_global_upgrade()
         exception = self.assertRaises(
             DependencyError, self.facade.perform_changes)
         self.assertEqual([foo3], exception.packages)
 
-    def test_mark_upgrade_no_upgrade(self):
+    def test_mark_global_upgrade_no_upgrade(self):
         """
         If the candidate version of a package is already installed,
-        mark_upgrade() won't request an upgrade to be made. I.e.
-        perform_changes() won't do anything.
+        C{mark_global_upgrade()} won't request an upgrade to be made. I.e.
+        C{perform_changes()} won't do anything.
         """
         deb_dir = self.makeDir()
         self._add_system_package("foo", version="3.0")
@@ -1096,10 +1116,10 @@
         self.facade.reload_channels()
         foo3 = sorted(self.facade.get_packages_by_name("foo"))[-1]
         self.assertEqual(foo3, foo3.package.candidate)
-        self.facade.mark_upgrade(foo3)
+        self.facade.mark_global_upgrade()
         self.assertEqual(None, self.facade.perform_changes())
 
-    def test_mark_upgrade_preserves_auto(self):
+    def test_mark_global_upgrade_preserves_auto(self):
         """
         Upgrading a package will retain its auto-install status.
         """
@@ -1114,8 +1134,7 @@
         noauto1, noauto2 = sorted(self.facade.get_packages_by_name("noauto"))
         auto1.package.mark_auto(True)
         noauto1.package.mark_auto(False)
-        self.facade.mark_upgrade(auto1)
-        self.facade.mark_upgrade(noauto1)
+        self.facade.mark_global_upgrade()
         self.assertRaises(DependencyError, self.facade.perform_changes)
         self.assertTrue(auto2.package.is_auto_installed)
         self.assertFalse(noauto2.package.is_auto_installed)
@@ -1211,7 +1230,7 @@
         error = self.assertRaises(DependencyError, self.facade.perform_changes)
         self.assertEqual([bar], error.packages)
 
-    def test_mark_upgrade_dependency_error(self):
+    def test_mark_global_upgrade_dependency_error(self):
         """
         If a package is marked for upgrade, a DependencyError will be
         raised, indicating which version of the package will be
@@ -1226,7 +1245,7 @@
         self.facade.reload_channels()
         foo_10, foo_15 = sorted(self.facade.get_packages_by_name("foo"))
         [bar] = self.facade.get_packages_by_name("bar")
-        self.facade.mark_upgrade(foo_10)
+        self.facade.mark_global_upgrade()
         error = self.assertRaises(DependencyError, self.facade.perform_changes)
         self.assertEqual(
             sorted([bar, foo_15], key=self.version_sortkey),
@@ -1266,7 +1285,7 @@
             "Can't perform the changes, since the following packages" +
             " are held: bar, foo", error.args[0])
 
-    def test_mark_upgrade_held_packages(self):
+    def test_mark_global_upgrade_held_packages(self):
         """
         If a package that is on hold is marked for upgrade,
         C{perform_changes} won't request to install a newer version of
@@ -1280,10 +1299,36 @@
         self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
         self.facade.reload_channels()
         [foo_10, foo_15] = sorted(self.facade.get_packages_by_name("foo"))
-        self.facade.mark_upgrade(foo_10)
+        self.facade.mark_global_upgrade()
         self.assertEqual(None, self.facade.perform_changes())
         self.assertEqual(foo_10, foo_15.package.installed)
 
+    def test_mark_global_upgrade_held_dependencies(self):
+        """
+        If a package that can be upgraded, but that package depends on a
+        newer version of a held package, the first package won't be
+        marked as requiring upgrade.
+        """
+        self._add_system_package(
+            "foo", version="1.0",
+            control_fields={"Status": "hold ok installed"})
+        self._add_system_package(
+            "bar", version="1.0",
+            control_fields={"Depends": "foo"})
+        deb_dir = self.makeDir()
+        self._add_package_to_deb_dir(deb_dir, "foo", version="2.0")
+        self._add_package_to_deb_dir(
+            deb_dir, "bar", version="2.0",
+            control_fields={"Depends": "foo (>> 1.0)"})
+        self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./")
+        self.facade.reload_channels()
+        [foo_1, foo_2] = sorted(self.facade.get_packages_by_name("foo"))
+        [bar_1, bar_2] = sorted(self.facade.get_packages_by_name("bar"))
+        self.facade.mark_global_upgrade()
+        self.assertEqual(None, self.facade.perform_changes())
+        self.assertEqual(foo_1, foo_2.package.installed)
+        self.assertEqual(bar_1, bar_2.package.installed)
+
     def test_get_locked_packages_simple(self):
         """
         C{get_locked_packages} returns all packages that are marked as