[libvirt PATCH v2 0/1] ci: Add helper script
Changes from [v1]: * implement (partial) support for running builds and spawning shells inside the container; * make the code more maintainable by using a couple of classes. [v1] https://listman.redhat.com/archives/libvir-list/2021-February/msg00900.h= tml Andrea Bolognani (1): ci: Add helper script ci/cirrus/refresh | 22 ----- ci/containers/refresh | 41 --------- ci/helper | 187 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 63 deletions(-) delete mode 100755 ci/cirrus/refresh delete mode 100755 ci/containers/refresh create mode 100755 ci/helper --=20 2.26.2
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. Advantages: * it provides a more standard, more convenient command line interface; * it refreshes all lcitool-generated files in one go; * it can be called from the top-level source directory; * it automatically finds lcitool if it's somewhere in the user's $PATH; * it produces some output to keep the user updated on the progress of the current operation; * it's written in a real programming language, which will hopefully help maintainability. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- ci/cirrus/refresh | 22 ----- ci/containers/refresh | 41 --------- ci/helper | 187 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 63 deletions(-) delete mode 100755 ci/cirrus/refresh delete mode 100755 ci/containers/refresh create mode 100755 ci/helper diff --git a/ci/cirrus/refresh b/ci/cirrus/refresh deleted file mode 100755 index 63ca794134..0000000000 --- a/ci/cirrus/refresh +++ /dev/null @@ -1,22 +0,0 @@ -#!/bin/sh - -if test -z "$1" -then - echo "syntax: $0 PATH-TO-LCITOOL" - exit 1 -fi - -LCITOOL=$1 - -if ! test -x "$LCITOOL" -then - echo "$LCITOOL is not executable" - exit 1 -fi - -HOSTS=$($LCITOOL hosts | grep -E 'freebsd|macos') - -for host in $HOSTS -do - $LCITOOL variables "$host" libvirt >"$host.vars" -done diff --git a/ci/containers/refresh b/ci/containers/refresh deleted file mode 100755 index f38d3634b5..0000000000 --- a/ci/containers/refresh +++ /dev/null @@ -1,41 +0,0 @@ -#!/bin/sh - -if test -z "$1" -then - echo "syntax: $0 PATH-TO-LCITOOL" - exit 1 -fi - -LCITOOL=$1 - -if ! test -x "$LCITOOL" -then - echo "$LCITOOL is not executable" - exit 1 -fi - -HOSTS=$($LCITOOL hosts | grep -Ev 'freebsd|macos') - -for host in $HOSTS -do - case "$host" in - fedora-rawhide) - for cross in mingw32 mingw64 - do - $LCITOOL dockerfile $host libvirt --cross $cross > ci-$host-cross-$cross.Dockerfile - done - ;; - debian-*) - for cross in aarch64 armv6l armv7l i686 mips mips64el mipsel ppc64le s390x - do - if test "$host-cross-$cross" = "debian-sid-cross-mips" - then - continue - fi - $LCITOOL dockerfile $host libvirt --cross $cross > ci-$host-cross-$cross.Dockerfile - done - ;; - esac - - $LCITOOL dockerfile $host libvirt > ci-$host.Dockerfile -done diff --git a/ci/helper b/ci/helper new file mode 100755 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) + + 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", + ) + + def add_engine_arg(self, parser): + parser.add_argument( + "--engine", + choices=["auto", "podman", "docker"], + default="auto", + help="container engine to use", + ) + + def add_login_arg(self, parser): + parser.add_argument( + "--login", + default=os.getlogin(), + help="login to use inside the container", + ) + + def add_lcitool_arg(self, parser): + parser.add_argument( + "--lcitool", + metavar="PATH", + default="lcitool", + help="path to lcitool binary", + ) + + def parse(self): + return self.parser.parse_args() + + +class Application: + def __init__(self): + self.basedir = os.path.dirname(os.path.realpath(__file__)) + + 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") + + 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") + + def lcitool_run(self, args): + output = subprocess.check_output([self.lcitool] + args) + return output.decode("utf-8") + + 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" + + 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"]: + 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: + 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__() + + +if __name__ == "__main__": + Application().run() -- 2.26.2
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
On Tue, 2021-02-23 at 16:17 +0100, Erik Skultety wrote:
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.
Sorry for taking a while to get back to you. I just posted a [v3] which should address pretty much all of your feedback, but I'm not sure we're on the same page when it comes to the overall strategy so I'm going to spend a few words on that here. Even as a simple wrapper, the new helper script is a massive usability win because options work in a standard way and are easily discoverable, so I'm very keen on adopting it as the official entry point for local CI-related functionality right away. Whether the underlying code is implemented or not in Python is not as important... That's obviously the end goal, but in the short term I'd rather have people call the Makefile through the new script than directly. And once the new script has been introduced, we can *definitely* rewrite functionality to Python in chunks. For example, the code underpinning the 'list-images' target is already mostly outsourced to a shell script, and your series from last month reimplemented that with a Python one: instead of making that script standalone and having the Makefile call out to it, we should just implement the functionality in the new helper script and drop the corresponding Makefile code entirely. The rest of the Makefile will have to be rewritten in one go because it's all tangled together, but again we can do that whenever we have some free cycles, without having to hurry too much since, regardless of the underlying implementation, users are already enjoying the improved user interface. [v3] https://listman.redhat.com/archives/libvir-list/2021-March/msg00631.html -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 12, 2021 at 06:54:15PM +0100, Andrea Bolognani wrote:
On Tue, 2021-02-23 at 16:17 +0100, Erik Skultety wrote:
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.
Sorry for taking a while to get back to you.
I just posted a [v3] which should address pretty much all of your feedback, but I'm not sure we're on the same page when it comes to the overall strategy so I'm going to spend a few words on that here.
Even as a simple wrapper, the new helper script is a massive usability win because options work in a standard way and are easily discoverable, so I'm very keen on adopting it as the official entry point for local CI-related functionality right away.
Whether the underlying code is implemented or not in Python is not as important... That's obviously the end goal, but in the short term I'd rather have people call the Makefile through the new script than directly.
And once the new script has been introduced, we can *definitely* rewrite functionality to Python in chunks. For example, the code underpinning the 'list-images' target is already mostly outsourced to a shell script, and your series from last month reimplemented that with a Python one: instead of making that script standalone and having the Makefile call out to it, we should just implement the functionality in the new helper script and drop the corresponding Makefile code entirely.
The rest of the Makefile will have to be rewritten in one go because it's all tangled together, but again we can do that whenever we have some free cycles, without having to hurry too much since, regardless of the underlying implementation, users are already enjoying the improved user interface.
That's fine, my point was that I didn't like a partial wrapper of the Makefile functionality, I just suggested one approach, while you went with a different one if v3 - which I certainly don't mind. Erik
participants (2)
-
Andrea Bolognani -
Erik Skultety