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
