On Wed, Oct 6, 2021 at 8:20 PM Sam James <s...@gentoo.org> 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 <s...@gentoo.org>
> ---
> 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
>
>
