Re: [gentoo-catalyst] Re: [PATCH 2/2] Move from PORTDIR_OVERLAY to repos.conf

[Matt Turner <mattst88@gentoo.org>]

On Sun, Oct 18, 2020 at 11:15 AM Felix Bier
<Felix.Bier@rohde-schwarz.com> wrote:
>
> This commit fixes the following issues:
>
>   * The PORTDIR_OVERLAY variable has been deprecated by Gentoo.
>
>     With this commit, the variable is no longer written to the
>     generated make.conf. Instead, a config file
>     /etc/portage/repos.conf/<repo-name>.conf
>     is generated for each overlay. The repo name is read from the
>     overlay using the portage API. Internally, portage parses
>     metadata/layout.conf and profiles/repo_name to obtain the name.
>
>     References:
>     https://wiki.gentoo.org/wiki//etc/portage/make.conf
>     https://wiki.gentoo.org/wiki//etc/portage/repos.conf
>
>   * All overlays were copied into the same target directory. If the
>     same file name occurred in multiple overlays, the last overlay
>     would overwrite all previous files with this name. In particular,
>     only the metadata/layout.conf of the last overlay was retained,
>     so it was not possible to reference the other overlays e.g. via
>     the masters entry in the layout.conf or the portage-2 syntax
>     for specifying a parent profile from another overlay. Also,
>     this created problems when the overlays contained ebuilds
>     for the same package, but with differing versions, because
>     after copying, the target directory contained both versions of the
>     ebuild but only the manifest file of the last overlay.
>
>     With this commit, each overlay is copied into a separate
>     sub-directory, e.g. /var/gentoo/repos/local/<repo-name>/.

I see this /var/gentoo/repos path used as an example in a few places
but not actually in any code. I think our example should match the
default structure these days, e.g., /var/db/repos/<repo-name>.

>     This directory is referenced via the location entry in the
>     generated /etc/portage/repos.conf/<repo-name>.conf.
> ---
>  catalyst/base/stagebase.py | 72 ++++++++++++++++++++++++++++----------
>  catalyst/defaults.py       |  1 +
>  catalyst/support.py        | 18 ++++++++++
>  3 files changed, 72 insertions(+), 19 deletions(-)
>
> diff --git a/catalyst/base/stagebase.py b/catalyst/base/stagebase.py
> index ac0f4f24..3d4f2a76 100644
> --- a/catalyst/base/stagebase.py
> +++ b/catalyst/base/stagebase.py
> @@ -1,4 +1,5 @@
>
> +import configparser
>  import copy
>  import os
>  import platform
> @@ -17,7 +18,8 @@ from DeComp.compress import CompressMap
>  from catalyst import log
>  from catalyst.defaults import (confdefaults, MOUNT_DEFAULTS, PORT_LOGDIR_CLEAN)
>  from catalyst.support import (CatalystError, file_locate, normpath,
> -                              cmd, read_makeconf, ismount, file_check)
> +                              cmd, read_makeconf, get_repo_name, ismount,
> +                              file_check)
>  from catalyst.base.targetbase import TargetBase
>  from catalyst.base.clearbase import ClearBase
>  from catalyst.base.genbase import GenBase
> @@ -821,17 +823,48 @@ class StageBase(TargetBase, ClearBase, GenBase):
>                      env=self.env)
>                  self.resume.enable("setup_confdir")
>
> +    def get_repo_conf_path(self, repo_name):
> +        """ Construct repo conf path: /etc/portage/repos.conf/{name}.conf """
> +        return normpath(os.path.join(self.settings['repos_conf'], repo_name + ".conf"))

I would rather we use pathlib and not add new uses of normpath.

> +
> +    def get_overlay_location(self, repo_name):
> +        """ Construct overlay repo path: /var/gentoo/repos/local/{name} """
> +        return normpath(os.path.join(self.settings['local_overlay'], repo_name))
> +
> +    def write_repo_conf(self, repo_name, config):
> +        """ Write ConfigParser to {chroot}/etc/portage/repo.conf/{name}.conf """
> +        repo_conf = self.get_repo_conf_path(repo_name)
> +        chroot_repo_conf = self.settings['chroot_path'] + repo_conf
> +        log.info('Creating repo config %s.', chroot_repo_conf)
> +        ensure_dirs(os.path.dirname(chroot_repo_conf))

Same thing: I'd rather use pathlib and use Path.mkdir and not add new
uses of ensure_dirs.

> +
> +        try:
> +            with open(chroot_repo_conf, 'w') as myf:

Let's rename myf. The 'my' prefix pattern in catalyst is not a good
one. Even just 'f' or 'file' would be better, IMO.

> +                config.write(myf)
> +        except OSError as e:
> +            raise CatalystError('Could not write {}: {}'.format(
> +                chroot_repo_conf, e)) from e
> +
>      def portage_overlay(self):

Feel free to rename things, like this function if their name no longer
matches the current terminology.

> -        """ We copy the contents of our overlays to /usr/local/portage """
> +        """ We copy the contents of our overlays to /var/gentoo/repos/local/{name} """

Let's not use a real path here. It only stands to confuse the reader.

>          if "portage_overlay" in self.settings:
>              for x in self.settings["portage_overlay"]:
>                  if os.path.exists(x):
> -                    log.info('Copying overlay dir %s', x)
> -                    ensure_dirs(
> -                        self.settings['chroot_path'] + self.settings['local_overlay'])
> -                    cmd("cp -a " + x + "/* " + self.settings["chroot_path"] +
> -                        self.settings["local_overlay"],
> -                        env=self.env)
> +                    name = get_repo_name(x)
> +
> +                    location = self.get_overlay_location(name)
> +                    config = configparser.ConfigParser()
> +                    config[name] = {'location': location}
> +                    self.write_repo_conf(name, config)
> +
> +                    chroot_location = normpath(
> +                        self.settings['chroot_path'] + location)
> +                    log.info('Copying overlay dir %s to %s',
> +                             x, chroot_location)
> +                    ensure_dirs(chroot_location)
> +                    cmd('cp -a ' + x + '/* ' + chroot_location, env=self.env)
> +                else:
> +                    log.warning('Skipping missing overlay %s.', x)
>
>      def root_overlay(self):
>          """ Copy over the root_overlay """
> @@ -1080,12 +1113,6 @@ class StageBase(TargetBase, ClearBase, GenBase):
>                      varname = x.split('_')[1].upper()
>                      myf.write(f'{varname}="{self.settings[x]}"\n')
>
> -            if setup:
> -                # Setup the portage overlay
> -                if "portage_overlay" in self.settings:
> -                    myf.write('PORTDIR_OVERLAY="%s"\n' %
> -                              self.settings["local_overlay"])
> -
>              # Set default locale for system responses. #478382
>              myf.write(
>                  '\n'
> @@ -1157,11 +1184,18 @@ class StageBase(TargetBase, ClearBase, GenBase):
>              log.warning("You've been hacking. Clearing target patches: %s", target)
>              clear_path(target)
>
> -        # Remove our overlay
> -        overlay = normpath(
> -            self.settings["chroot_path"] + self.settings["local_overlay"])
> -        if os.path.exists(overlay):
> -            clear_path(overlay)
> +        # Remove our overlays
> +        if "portage_overlay" in self.settings:
> +            for repo_path in self.settings["portage_overlay"]:
> +                repo_name = get_repo_name(repo_path)
> +
> +                repo_conf = self.get_repo_conf_path(repo_name)
> +                chroot_repo_conf = self.settings["chroot_path"] + repo_conf
> +                clear_path(chroot_repo_conf)
> +
> +                location = self.get_overlay_location(repo_name)
> +                chroot_location = self.settings['chroot_path'] + location
> +                clear_path(chroot_location)
>
>          if "sticky-config" not in self.settings["options"]:
>              # re-write the make.conf to be sure it is clean
> diff --git a/catalyst/defaults.py b/catalyst/defaults.py
> index c153fcc4..9660a7f3 100644
> --- a/catalyst/defaults.py
> +++ b/catalyst/defaults.py
> @@ -38,6 +38,7 @@ confdefaults = {
>      "local_overlay": "/var/db/repos/local",
>      "port_conf": "/etc/portage",
>      "make_conf": "%(port_conf)s/make.conf",
> +    "repos_conf": "%(port_conf)s/repos.conf",
>      "options": set(),
>      "pkgdir": "/var/cache/binpkgs",
>      "port_tmpdir": "/var/tmp/portage",
> diff --git a/catalyst/support.py b/catalyst/support.py
> index a6a6854a..b8069c7d 100644
> --- a/catalyst/support.py
> +++ b/catalyst/support.py
> @@ -7,6 +7,8 @@ import shutil
>  import time
>  from subprocess import Popen
>
> +from portage.repository.config import RepoConfig
> +
>  from catalyst import log
>
>  BASH_BINARY = "/bin/bash"
> @@ -179,6 +181,22 @@ def read_makeconf(mymakeconffile):
>          return makeconf
>
>
> +def get_repo_name(repo_path):
> +    """ Get the name of the repo at the given repo_path.
> +
> +         References:
> +         https://wiki.gentoo.org/wiki/Repository_format/profiles/repo_name
> +         https://wiki.gentoo.org/wiki/Repository_format/metadata/layout.conf#repo-name
> +    """
> +
> +    repo_config = RepoConfig(None, {"location": repo_path})
> +
> +    if repo_config.missing_repo_name:
> +        raise CatalystError("Missing name in repository {}".format(repo_path))

I'd prefer to use f-strings unless there's a good reason not to.