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

[Juerg Haefliger]

Review: Needs Information

Comments inline.

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()

Is it really necessary to cache the previous state? It should be sufficient to just go through the list of installed modules after the upgrade, no?

> +
> +        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]

These are package versions so built-in sort() might do the wrong thing. Theoretically. Not sure if we can hit that problem...

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

Is this flattening a 2d list? Then maybe:
deps = sum(target_version.dependencies, [])

> +
> +        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
> +

Wow, this is complicated just to figure out the new kernel version? Isn't there an easier way? Maybe look at '/lib/modules/*/build'. But then again one could have non-Ubuntu kernel/headers installed, so probably not a good idea?

> +        # 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)
> +

Again, can't we simplify this?
1) Figure out the new kernel version
2) Walk through 'dmks status' and rebuild all modules for kernel version from step 1

The current implementation ignores newly installed modules. Again not sure if that can happen during an upgrade.

> +        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.