Re: [gentoo-portage-dev] [PATCH] lib/_emerge/actions.py: warn on missing /run

[Alec Warner]

On Wed, Oct 6, 2021 at 8:20 PM Sam James <sam@g.o> wrote:
>
> Newer versions of build-docbook-catalog use
> /run/lock. This exposed that we weren't
> asking users to mount /run in the handbook.
>
> Check if it exists and warn if it doesn't.
>
> This should primarily (exclusively?) be a
> problem in chroots given an init system
> should be creating this.
>
> Bug: https://bugs.gentoo.org/816303
> Signed-off-by: Sam James <sam@g.o>
> ---
>  lib/_emerge/actions.py | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/lib/_emerge/actions.py b/lib/_emerge/actions.py
> index 05a115250..1b40bebd3 100644
> --- a/lib/_emerge/actions.py
> +++ b/lib/_emerge/actions.py
> @@ -2986,17 +2986,26 @@ def validate_ebuild_environment(trees):
>      check_locale()
>
>
> -def check_procfs():
> -    procfs_path = "/proc"
> -    if platform.system() not in ("Linux",) or os.path.ismount(procfs_path):
> -        return os.EX_OK
> -    msg = "It seems that %s is not mounted. You have been warned." % procfs_path
> -    writemsg_level(
> -        "".join("!!! %s\n" % l for l in textwrap.wrap(msg, 70)),
> -        level=logging.ERROR,
> -        noiselevel=-1,
> -    )
> -    return 1

Please add a docstring (""" comment) to the function describing why
it's doing what its doing.

> +def check_mounted_fs():
> +    paths = {"/proc": False, "/run": False}

So on Linux, proc is necessary. Isn't /run necessary regardless of OS?
Will docbook build on prefix, or on BSD without /run (I'm guessing
not?)

We might go with something like:

def check_mounted_fs():
  """Cheeky comment explaining why we probably need these mounts."""
required_paths = ['/run']
if platform.system() == 'Linux':
  required_paths.append('/proc')

missing_mounts = False
for path in required_paths:
  if not os.path.ismount(path):
    msg = ....
    writemsg = ...
    missing_mounts = True
  # else we do nothing, the mount is where we want it, etc.

return missing_mounts # true if mounts are missing, false otherwise.

> +
> +    for path in paths.keys():
> +        if platform.system() not in ("Linux",) or os.path.ismount(path):
> +            paths[path] = True
> +            continue
> +
> +        msg = "It seems that %s is not mounted. You have been warned." % path
> +        writemsg_level(
> +            "".join("!!! %s\n" % l for l in textwrap.wrap(msg, 70)),
> +            level=logging.ERROR,
> +            noiselevel=-1,
> +        )

Duncan covered this already, but I will +1 his concern about better
messaging being an option ;)

> +
> +    # Were any of the mounts we were looking for missing?
> +    if False in paths.values():
> +        return 1

So a couple of nitpicks here. This is a common sort of "collect"
pattern; and I'd use python's any() / all() helpers for a more
pythonic experience.

E.g.

items = [some, list, of, items]
results = []
for j in items:
  results.append(some_computation(j))

# Did any of the computations succeed?
return any(results) # at least 1 True
# Did all of the computations succeed?
return all(results) # all True
# Did none of the computations succeed?
return not any(results) # all False

So in this example you have elected to store the results in a dict,
and we want to return true if all the results are true:

return all(paths.values())

But see also earlier notes above.

> +
> +    return os.EX_OK

This isn't shell, so return True or False (not 0 or 1.) (but see above comment.)

>
>
>  def config_protect_check(trees):
> @@ -3474,7 +3483,8 @@ def run_action(emerge_config):
>          repo_name_check(emerge_config.trees)
>          repo_name_duplicate_check(emerge_config.trees)
>          config_protect_check(emerge_config.trees)
> -    check_procfs()
> +
> +    check_mounted_fs()
>
>      for mytrees in emerge_config.trees.values():
>          mydb = mytrees["porttree"].dbapi
> --
> 2.33.0
>
>