3:24 a.m.
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
3:24 a.m.
New subject: [libvirt PATCH v2 1/1] ci: Add helper script
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(a)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(a){self.target}")
+
+ def action_shell(self):
+ self.make_run(f"ci-shell(a){self.target}")
+
+ def run(self):
+ method = "action_{}".format(self.action.replace("-",
"_"))
+ getattr(self, method).__call__()
+
+
+if __name__ == "__main__":
+ Application().run()
--
2.26.2
9:17 a.m.
New subject: [libvirt PATCH v2 1/1] ci: Add helper script
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(a){self.target}")
+
+ def action_shell(self):
+ self.make_run(f"ci-shell(a){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
11:54 a.m.
New subject: [libvirt PATCH v2 1/1] ci: Add helper script
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
8:17 a.m.
New subject: [libvirt PATCH v2 1/1] ci: Add helper script
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
1348
days inactive
1374
days old
4 comments
2 participants
participants (2)
-
Andrea Bolognani -
Erik Skultety