Merge ~juliank/ubuntu-release-upgrader:improve-obsoletes into ubuntu-release-upgrader:ubuntu/main

[Julian Andres Klode]

[Julian Andres Klode (juliank)]

I think we likely want to ship this _after_ the current fixes in ubuntu/main have landed in -updates, as this is a bigger change for performance, but ymmv.

[Nick Rosbrook (enr0n)]

This looks good to me. Thanks for the detailed commit message.

review: Approve

[Nick Rosbrook (enr0n)]

I rebased your commits and pushed to ubuntu/main.

diff --git a/DistUpgrade/DistUpgradeCache.py b/DistUpgrade/DistUpgradeCache.py
index 0692ef9..77ffb64 100644
--- a/DistUpgrade/DistUpgradeCache.py
+++ b/DistUpgrade/DistUpgradeCache.py
@@ -940,7 +940,7 @@ class MyCache(apt.Cache):
         return False
 
     @withResolverLog
-    def tryMarkObsoleteForRemoval(self, pkgname, remove_candidates, forced_obsoletes, foreign_pkgs):
+    def tryMarkObsoleteForRemoval(self, pkgname, remove_candidates, forced_obsoletes, foreign_pkgs, auto_fix):
         #logging.debug("tryMarkObsoleteForRemoval(): %s" % pkgname)
         # coherence check, first see if it looks like a running kernel pkg
         if pkgname.endswith(self.uname):
@@ -972,28 +972,27 @@ class MyCache(apt.Cache):
         # if this package has not been forced obsolete, only
         # delete it if it doesn't remove other dependents
         # that are not obsolete as well
-        actiongroup = apt_pkg.ActionGroup(self._depcache)
-        # just make pyflakes shut up, later we should use
-        # with self.actiongroup():
-        actiongroup
-        self.create_snapshot()
+        if auto_fix:
+            self.create_snapshot()
         try:
-            self[pkgname].mark_delete(purge=purge)
+            self[pkgname].mark_delete(purge=purge, auto_fix=auto_fix)
             self.view.processEvents()
-            if pkgname in forced_obsoletes:
-                return True
-            #logging.debug("marking '%s' for removal" % pkgname)
-            for pkg in self.get_changes():
-                if (pkg.name not in remove_candidates or
-                      pkg.name in foreign_pkgs or
-                      self._inRemovalDenylist(pkg.name) or
-                      pkg.name == self.linux_metapackage):
-                    logging.debug("package '%s' produces an unwanted removal '%s', skipping" % (pkgname, pkg.name))
-                    self.restore_snapshot()
-                    return False
+            if auto_fix:
+                if pkgname in forced_obsoletes:
+                    return True
+                #logging.debug("marking '%s' for removal" % pkgname)
+                for pkg in self.get_changes():
+                    if (pkg.name not in remove_candidates or
+                          pkg.name in foreign_pkgs or
+                          self._inRemovalDenylist(pkg.name) or
+                          pkg.name == self.linux_metapackage):
+                        logging.debug("package '%s' produces an unwanted removal '%s', skipping" % (pkgname, pkg.name))
+                        self.restore_snapshot()
+                        return False
         except (SystemError, KeyError) as e:
             logging.warning("_tryMarkObsoleteForRemoval failed for '%s' (%s: %s)" % (pkgname, repr(e), e))
-            self.restore_snapshot()
+            if auto_fix:
+                self.restore_snapshot()
             return False
         return True
 
diff --git a/DistUpgrade/DistUpgradeController.py b/DistUpgrade/DistUpgradeController.py
index eebe281..f7853ae 100644
--- a/DistUpgrade/DistUpgradeController.py
+++ b/DistUpgrade/DistUpgradeController.py
@@ -1995,12 +1995,41 @@ class DistUpgradeController(object):
         logging.debug("remove_candidates: '%s'" % remove_candidates)
         logging.debug("Start checking for obsolete pkgs")
         progress = self._view.getOpCacheProgress()
-        for (i, pkgname) in enumerate(remove_candidates):
-            progress.update((i/float(len(remove_candidates)))*100.0)
-            if pkgname not in self.foreign_pkgs:
-                self._view.processEvents()
-                if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs):
-                    logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+        scheduled_remove = set()
+
+        with self.cache.actiongroup():
+            # Forced obsoletes we remove, removing any of their dependencies, hence do a first loop with auto_fix=True
+            for (i, pkgname) in enumerate(remove_candidates):
+                progress.update((i/float(len(remove_candidates)))*100.0 / 2)
+                if pkgname in self.foreign_pkgs and pkgname in self.force_obsoletes:
+                    self._view.processEvents()
+                    if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs, auto_fix=True):
+                        logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+                    else:
+                        scheduled_remove.add(pkgname)
+
+            # Now let's try to remove other packages
+            for (i, pkgname) in enumerate(remove_candidates):
+                progress.update((i/float(len(remove_candidates)))*100.0 / 2 + 50)
+                if pkgname not in self.foreign_pkgs:
+                    self._view.processEvents()
+                    if not self.cache.tryMarkObsoleteForRemoval(pkgname, remove_candidates, self.forced_obsoletes, self.foreign_pkgs, auto_fix=False):
+                        logging.debug("'%s' scheduled for remove but not safe to remove, skipping", pkgname)
+                    else:
+                        scheduled_remove.add(pkgname)
+
+            # We have scheduled their removals, but not any reverse-dependencies. If anything is broken now,
+            # resolve them by keeping back the obsolete packages.
+            self.cache._startAptResolverLog()
+            pr = apt.ProblemResolver(self.cache)
+            pr.resolve_by_keep()
+            self.cache._stopAptResolverLog()
+
+            # resolve_by_keep() will revert any unsafe removals, so we need to list them here again.
+            for pkgname in scheduled_remove:
+                if not self.cache[pkgname].marked_delete:
+                    logging.debug("obsolete package '%s' could not be removed", pkgname)
+
         logging.debug("Finish checking for obsolete pkgs")
         progress.done()