[libvirt PATCH v2 1/1] ci: Add helper script
Erik Skultety
eskultet at redhat.com
Tue Feb 23 15:17:10 UTC 2021
On Wed, Feb 17, 2021 at 10:24:51AM +0100, Andrea Bolognani wrote:
> This is intended to be the perform a number of CI-related
> operations that currently are implemented in various different
> scripts written in various different programming languages; in
> this first iteration it does two things:
>
> * implement the functionality of the existing "refresh"
> scripts, which it supersedes;
>
> * provide a nicer front-end for a subset of the
> functionality exposed by the ci/Makefile scaffolding, such
> as running basic builds.
>
> Over time, the plan is to rewrite all CI-related functionality
> in Python and move it into this script.
You dived into it more aggressively than what I proposed, but we still need to
approach it gradually, like don't extract the Makefile functionality partially.
Let's lay the foundation first, then replace refresh and then at some point in
the future, drop the Makefile and integrate it into the helper, I don't mind
keeping the Makefile for a little longer.
> index 0000000000..dec24ac741
> --- /dev/null
> +++ b/ci/helper
> @@ -0,0 +1,187 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (C) 2021 Red Hat, Inc.
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library. If not, see
> +# <http://www.gnu.org/licenses/>.
> +
> +import argparse
> +import os
> +import pty
> +import shutil
> +import subprocess
> +import sys
> +
> +
> +class Parser:
> + def __init__(self):
> + self.parser = argparse.ArgumentParser()
> + subparsers = self.parser.add_subparsers(
> + dest="action",
> + metavar="ACTION",
> + )
> + subparsers.required = True
> +
> + buildparser = subparsers.add_parser(
> + "build",
> + help="run a build in a container",
> + )
> + self.add_target_arg(buildparser)
> + self.add_engine_arg(buildparser)
> + self.add_login_arg(buildparser)
I've disliked ^this practice for a while, but I somehow never got to improving
it in lcitool. However, the argparse way to do this for shared arguments is via
parent parsers [1], but since I disapprove of introducing Makefile bits in this
patch anyway, you won't need to bother with that just yet.
> +
> + shellparser = subparsers.add_parser(
> + "shell",
> + help="start a shell in a container",
> + )
> + self.add_target_arg(shellparser)
> + self.add_engine_arg(shellparser)
> + self.add_login_arg(shellparser)
> +
> + refreshparser = subparsers.add_parser(
> + "refresh",
> + help="refresh data generated with lcitool",
> + )
> + self.add_lcitool_arg(refreshparser)
> +
> + def add_target_arg(self, parser):
> + parser.add_argument(
> + "target",
> + help="build on target OS",
> + )
You added target, but the user doesn't know what those are unless they run the
Makefile ;), that's why I said, no partial functionality.
...
> +
> +
> +class Application:
> + def __init__(self):
> + self.basedir = os.path.dirname(os.path.realpath(__file__))
Let's standardize on Pathlib, we already did that in lcitool, that's the
Pythonic way of handling paths in the current era.
> +
> + args = Parser().parse()
> + self.action = args.action
> +
> + if args.action == "refresh":
> + self.lcitool = args.lcitool
> + if not shutil.which(self.lcitool):
> + sys.exit("error: 'lcitool' not installed")
Here, you report with sys.exit...
> +
> + elif args.action in ["build", "shell"]:
> + self.target = args.target
> + self.engine = args.engine
> + self.login = args.login
> +
> + def make_run(self, target):
> + args = [
> + "-C", self.basedir, target,
> + f"CI_ENGINE={self.engine}",
> + f"CI_USER_LOGIN={self.login}",
> + ]
> +
> + if pty.spawn(["make"] + args) != 0:
> + raise Exception("make failed")
...here you raise an exception. We need to be consistent, I'd suggest sys.exit.
Later I'm planning on introducing logging to lcitool which is on my TODO, so we
can bother with that here as well at some point in the future.
> +
> + def lcitool_run(self, args):
> + output = subprocess.check_output([self.lcitool] + args)
> + return output.decode("utf-8")
I'm glad you did this, as it kinda hints that it actually would make sense to
make a library out of lcitool some day to be idiomatic so to speak :).
> +
> + def lcitool_get_hosts(self):
> + output = self.lcitool_run(["hosts"])
> + return output.splitlines()
> +
> + def generate_dockerfile(self, host, cross=None):
> + args = ["dockerfile", host, "libvirt"]
> + outfile = f"{self.basedir}/containers/ci-{host}.Dockerfile"
> +
> + if cross:
> + args.extend(["--cross", cross])
> + outfile = f"{self.basedir}/containers/ci-{host}-cross-{cross}.Dockerfile"
You can save some columns if you introduce a variable to hold the ^base path.
> +
> + output = self.lcitool_run(args)
> + with open(outfile, "w") as f:
> + f.write(output)
> +
> + def generate_vars(self, host):
> + output = self.lcitool_run(["variables", host, "libvirt"])
> + with open(f"{self.basedir}/cirrus/{host}.vars", "w") as f:
> + f.write(output)
> +
> + def refresh_containers(self):
> + for host in self.lcitool_get_hosts():
> + if "freebsd" in host or "macos" in host:
> + continue
> +
> + if host == "fedora-rawhide":
> + for cross in ["mingw32", "mingw64"]:
> + print(f"containers/{host} ({cross})")
> + self.generate_dockerfile(host, cross)
> +
> + if "debian-" in host:
> + for cross in ["aarch64", "armv6l", "armv7l", "i686", "mips", "mips64el", "mipsel", "ppc64le", "s390x"]:
Break the ^line please and align the list.
> + if host == "debian-sid" and cross == "mips":
> + continue
> + print(f"containers/{host} ({cross})")
> + self.generate_dockerfile(host, cross)
> +
> + print(f"containers/{host}")
> + self.generate_dockerfile(host)
> +
> + def refresh_cirrus(self):
> + for host in self.lcitool_get_hosts():
> + if "freebsd" not in host and "macos" not in host:
^IMO better written as:
if host.split("-")[0] not in ["freebsd", "macos"]:
continue
For some reason when I look at the original condition it feels confusing
because by the way it's written you don't anticipate the actual host name
something else than "freebsd"|"macos" whereas in reality this is
"freebsd-<version>". Same for the hunk at the beginning of the refresh
method. Maybe even for the 'if "debian-" in host' check for the sake of
consistency, but it's not a deal breaker.
> + continue
> +
> + print(f"cirrus/{host}")
> + self.generate_vars(host)
> +
> + def action_refresh(self):
> + self.refresh_containers()
> + self.refresh_cirrus()
> +
> + def action_build(self):
> + self.make_run(f"ci-build@{self.target}")
> +
> + def action_shell(self):
> + self.make_run(f"ci-shell@{self.target}")
> +
> + def run(self):
> + method = "action_{}".format(self.action.replace("-", "_"))
> + getattr(self, method).__call__()
This is quite hacky, there's a better way to do it...you've saved the args to
self.args already, so make use of it. You'll need to call set_defaults on the
respective parsers and pass the action callback via 'func='. Then you only need
to do:
self.args.func()
Erik
PS: Overall, I like the aggressive approach you've taken :)
[1] https://docs.python.org/3/library/argparse.html#parents
More information about the libvir-list
mailing list