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

[Nick Rosbrook]

Diff comments:

> diff --git a/DistUpgrade/DistUpgradeController.py b/DistUpgrade/DistUpgradeController.py
> index d0c7075..151bdbd 100644
> --- a/DistUpgrade/DistUpgradeController.py
> +++ b/DistUpgrade/DistUpgradeController.py
> @@ -567,7 +567,41 @@ class DistUpgradeController(object):
>  
>          index = {}
>          for entry in self.sources:
> -            if not isinstance(entry, SourceEntry) or entry.invalid:
> +            if not isinstance(entry, SourceEntry):
> +                continue
> +
> +            if entry.invalid:
> +                if entry.file == sourcelist_file:
> +                    continue
> +
> +                logging.debug(
> +                    f'Could not migrate {str(entry)} from {entry.file}'
> +                    ', disabling instead.'
> +                )
> +
> +                # It's possible that the source is considered invalid because python-apt
> +                # does not support [signed-by=] or something. For anything that we
> +                # cannot migrate, move it instead to <filename>.disabled. These will not
> +                # be cleaned up at the end of the upgrade.
> +                #
> +                # NB: Unfortunately, we cannot just do entry.disabled = True and let
> +                # sources.save() write disabled entries, because SourceEntry.str will
> +                # return the raw line when entry.invalid == True. So, the line will not
> +                # actually be commented out.
> +                disabled_path = f'{entry.file}.disabled'
> +
> +                if not os.path.exists(disabled_path):
> +                    with open(disabled_path, 'w') as f:
> +                        f.write(
> +                            '# This file could not be automatically migrated '
> +                            'to a .sources file during the upgrade.\n'
> +                            '# Please see sources.list(5) (or '
> +                            'https://manpages.ubuntu.com/manpages/man5/sources.list.5.html'
> +                            ')\n# for details on how to migrate manually.\n\n'
> +                        )
> +                with open(disabled_path, 'a') as f:

> should this be indented under the `if not os.path.exists`?

No, this part is intentional, because the outer loop is iterating over source *entries*, not source *files*. E.g.:

# /etc/apt/source.list.d/proposed.list
deb http://archive.ubuntu.com/ubuntu resolute-proposed main
deb http://archive.ubuntu.com/ubuntu resolute-proposed universe

are two distinct source entries in the same file.

> or should we have a check of the contents of the file to see if it's already been migrated (correctly or incorrectly)?

Well, nothing is *actually* being migrated here. This file is just to serve as information to the user. It's only slightly better than the current case where the entries are silently dropped. So I don't think it's worth parsing existing files or anything like that.

I can push an adjustment though.

> +                    f.write(f'# {str(entry)}\n')
> +
>                  continue
>  
>              # Remove disabled deb-src entries, because stylistically it makes


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