[Merge] ~enr0n/ubuntu-release-upgrader:lp2020406 into ubuntu-release-upgrader:ubuntu/main

[Nick Rosbrook]

Thanks for your review! I have responded to all comments.

Diff comments:

> diff --git a/DistUpgrade/DistUpgradeQuirks.py b/DistUpgrade/DistUpgradeQuirks.py
> index 0bd1ef9..dfcd86c 100644
> --- a/DistUpgrade/DistUpgradeQuirks.py
> +++ b/DistUpgrade/DistUpgradeQuirks.py
> @@ -521,6 +523,101 @@ class DistUpgradeQuirks(object):
>              self._view.getInstallProgress(self.controller.cache)
>          )
>  
> +    def _disable_dkms_autoinstall(self):
> +        if not os.path.exists('/usr/sbin/dkms'):
> +            return
> +
> +        with open('/etc/dkms/no-autoinstall', 'w'):
> +            pass
> +
> +        self._installed_dkms_modules = set()

In practice I found that at the end of the upgrade (where dkms autoinstall is disabled), dkms status is empty. So I think this is necessary unless we just assume that everything in /usr/src should be installed. But if someone had a dkms package installed but later ran `dkms uninstall` on that module, then `dkms autoinstall` would normally ignore that on upgrade, so I thought we should do the same.

> +
> +        r = subprocess.run(['dkms', 'status'],
> +                           stdout=subprocess.PIPE,
> +                           stderr=subprocess.PIPE)
> +        if r.returncode != 0:
> +            logging.debug('dkms status failed: {}'.format(r.stderr.decode('utf-8')))
> +            return
> +
> +        for line in [l.decode('utf-8') for l in r.stdout.splitlines()]:
> +            # Each line of dkms status output is formatted like this:
> +            #  $module/$version, $kernel_version, $arch: $install_state
> +            parts = [l.strip() for l in line.split(',')]
> +            try:
> +                module = parts[0].split('/')[0]
> +                install_state = parts[2].split(':')[1].strip()
> +
> +                if install_state == 'installed':
> +                    self._installed_dkms_modules.add(module)
> +            except IndexError:
> +                logging.debug('Unexpected output from dkms status: {}', line)
> +
> +    def _reenable_dkms_autoinstall(self):
> +        if not os.path.exists('/usr/sbin/dkms'):
> +            return
> +
> +        try:
> +            os.remove('/etc/dkms/no-autoinstall')
> +        except OSError as e:
> +            logging.debug('Failed to remove /etc/dkms/no-autoinstall: {}'
> +                          .format(e))
> +
> +        if not self._installed_dkms_modules:
> +            return
> +
> +        # Figure out which kernel we should build against by checking the
> +        # dependency of the installed linux-headers metapackage.
> +        headers_version = None
> +
> +        headers_pkg = self._get_linux_metapackage(self.controller.cache, True)
> +        headers_pkg = self.controller.cache[headers_pkg]
> +
> +        installed_versions = [v for v in headers_pkg.versions if v.is_installed]
> +        target_version = sorted(installed_versions)[-1]

I am pretty sure that the underlying object from python-apt (apt.Version) implements all of the comparison functions (__eq__, __ge__, etc.) so that sorted() will work correctly. I will double check this though.

> +
> +        deps = []
> +        for d in target_version.dependencies:
> +            deps += d

Ah cool, thanks!

> +
> +        for dep in deps:
> +            if dep.name.startswith('linux-headers'):
> +                headers_version = dep.name.removeprefix('linux-headers-')
> +                break
> +
> +        if headers_version is None:
> +            logging.debug('Failed to find installed linux-headers package')
> +            return
> +

Yeah, I think for consistency with the rest of the code it is necessary to at least base our decision on the meta package returned by `_get_linux_metapackage()`. If you know of a better way to map `linux-headers-generic` to the latest installed version of `linux-headers-$version-generic`, then I would use that. But from just looking at the meta package itself, it seemed the most reliable way was to look at the dependency.

> +        # Now try to re-build/install the previously installed dkms modules
> +        # against these newly-installed kernel headers, and if a module fails
> +        # to build, leave it uninstalled.
> +        failed_modules = []
> +        for mod in self._installed_dkms_modules:
> +            logging.debug('Attempting to re-install {} dkms module with {}'
> +                          .format(mod, headers_version))
> +
> +            # By this point in the upgrade, the dkms modules may have been
> +            # upgraded, so check what versions are installed now.
> +            sourcedirs = glob.glob('/usr/src/{}-*'.format(mod))
> +            versions = [os.path.basename(p).removeprefix(mod+'-') for p in sourcedirs]
> +
> +            for v in versions:
> +                mod = '{}/{}'.format(mod, v)
> +
> +                r = subprocess.run(['dkms', 'install', mod, '-k', headers_version],
> +                                   stderr=subprocess.PIPE)
> +                if r.returncode != 0:
> +                    logging.debug('dkms install failed for {}:\n{}'
> +                                  .format(mod, r.stderr.decode('utf-8')))
> +                    failed_modules.append(mod)
> +

See my earlier comment about dkms status being empty after the upgrade. That is a good point about newly installed dkms modules, however.

> +        if failed_modules:
> +            self._view.information(_('dkms module install failed'),
> +                                   _('The following dkms modules failed to '
> +                                     'build with the installed kernel headers '
> +                                     'and will remain disabled:\n{}')
> +                                     .format('  ' + '\n'.join(failed_modules)))
> +
>      def _add_apport_ignore_tracker_extract(self):
>          path = '/etc/apport/blacklist.d/upgrade-quirk-tracker-extract-3'
>  


-- 
https://code.launchpad.net/~enr0n/ubuntu-release-upgrader/+git/ubuntu-release-upgrader/+merge/445212
Your team Ubuntu Core Development Team is subscribed to branch ubuntu-release-upgrader:ubuntu/main.