Re: [RFC PATCH 2/4] lcitool: use libvirt-ci as library

[Daniel P . Berrangé]

On Tue, Jan 17, 2023 at 10:16:36AM +0100, Paolo Bonzini wrote:
> Using the lcitool package as a library will make it possible to
> customize more of the process, for example by introducing custom
> mappings.

I wouldn't consider the lcitool python code to be a public
library API, which is for example, why its not a published
pakage on pypi.  So from that POV I'm really not a fan of
this change to use the internal APIs directly, as it means
that any contributors who want to update the submodule are
liable to get hit by incompatible API changes.

> diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
> index ee55ee40ee5d..31a34dce7a33 100755
> --- a/tests/lcitool/refresh
> +++ b/tests/lcitool/refresh
> @@ -12,61 +12,80 @@
>  # or (at your option) any later version. See the COPYING file in
>  # the top-level directory.
>  
> -import sys
> -import subprocess
> -
> +from contextlib import contextmanager
>  from pathlib import Path
>  
> +import sys
> +
>  if len(sys.argv) != 1:
>      print("syntax: %s" % sys.argv[0], file=sys.stderr)
>      sys.exit(1)
>  
> -self_dir = Path(__file__).parent
> -src_dir = self_dir.parent.parent
> +script = Path(__file__)
> +script_dir = script.parent
> +src_dir = script_dir.parent.parent
>  dockerfiles_dir = Path(src_dir, "tests", "docker", "dockerfiles")
>  
> -lcitool_path = Path(self_dir, "libvirt-ci", "bin", "lcitool")
> +sys.path.append(str(Path(script_dir, "libvirt-ci")))
>  
> -lcitool_cmd = [lcitool_path, "--data-dir", self_dir]
> +from lcitool import LcitoolError
> +from lcitool.packages import Packages
> +from lcitool.projects import Projects
> +from lcitool.targets import BuildTarget, Targets
> +from lcitool.formatters import DockerfileFormatter, ShellVariablesFormatter
> +from lcitool.util import DataDir
> +
> +PREFIX = ''
> +
> +DATA_DIR = DataDir(script_dir)
> +PROJECTS = Projects(DATA_DIR)
> +PACKAGES = Packages()
> +TARGETS = Targets()
>  
>  
> -def atomic_write(filename, content):
> +@contextmanager
> +def atomic_write(filename):
>      tmp = filename.with_suffix(filename.suffix + ".tmp")
>      try:
>          with tmp.open("w") as fp:
> -            print(content, file=fp, end="")
> +            yield fp
>              tmp.rename(filename)
> -    except Exception as ex:
> +    except Exception:
>          tmp.unlink()
>          raise
>  
>  
> -def generate(filename, cmd, trailer):
> -    print("Generate %s" % filename)
> -    lcitool = subprocess.run(cmd, capture_output=True)
> -
> -    if lcitool.returncode != 0:
> -        raise Exception("Failed to generate %s: %s" % (filename, 
> lcitool.stderr))
> -
> -    content = lcitool.stdout.decode("utf8")
> -    if trailer is not None:
> -        content += trailer
> -    atomic_write(filename, content)
> +@contextmanager
> +def generate(filename):
> +    print("Generating %s" % filename)
> +    nonlocal PREFIX
> +    try:
> +        PREFIX = "Failed to generate %s: " % filename
> +        with atomic_write(filename) as fp:
> +            print('# THIS FILE WAS AUTO-GENERATED BY',
> +                  script.relative_to(src_dir), file=fp)
> +            print(file=fp)
> +            yield fp
> +    finally:
> +        PREFIX = ''
>  
>  
>  def generate_dockerfile(host, target, cross=None, trailer=None):
>      filename = Path(src_dir, "tests", "docker", "dockerfiles", host + 
> ".docker")
> -    cmd = lcitool_cmd + ["dockerfile"]
> -    if cross is not None:
> -        cmd.extend(["--cross", cross])
> -    cmd.extend([target, "qemu"])
> -    generate(filename, cmd, trailer)
> +    with generate(filename) as fp:
> +        dockerfile = DockerfileFormatter(PROJECTS)
> +        target = BuildTarget(TARGETS, PACKAGES, target, cross)
> +        print(dockerfile.format(target, ["qemu"]), file=fp)
> +        if trailer is not None:
> +            print(trailer, file=fp, end="")
>  
>  
> -def generate_cirrus(target, trailer=None):
> +def generate_cirrus(target):
>      filename = Path(src_dir, ".gitlab-ci.d", "cirrus", target + ".vars")
> -    cmd = lcitool_cmd + ["variables", target, "qemu"]
> -    generate(filename, cmd, trailer)
> +    with generate(filename) as fp:
> +        variables = ShellVariablesFormatter(PROJECTS)
> +        target = BuildTarget(TARGETS, PACKAGES, target)
> +        print(variables.format(target, ["qemu"]), file=fp)
>  
>  
>  ubuntu2004_tsanhack = [
> @@ -98,11 +117,11 @@ def cross_build(prefix, targets):
>      targets = "ENV DEF_TARGET_LIST %s\n" % (targets)
>      return "".join([conf, targets])
>  
> +
>  #
>  # Update all the various build configurations.
>  # Please keep each group sorted alphabetically for easy reading.
>  #
> -
>  try:
>      #
>      # Standard native builds
> @@ -179,6 +198,6 @@ try:
>      generate_cirrus("macos-12")
>  
>      sys.exit(0)
> -except Exception as ex:
> -    print(str(ex), file=sys.stderr)
> +except LcitoolError as ex:
> +    print(PREFIX + ex.module_prefix + " error: " + str(ex), file=sys.stderr)
>      sys.exit(1)
> -- 
> 2.38.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|