Re: [PATCH 2/3] tests/vm: Introduce get_qemu_packages_from_lcitool_vars() helper

Re: [PATCH 2/3] tests/vm: Introduce get_qemu_packages_from_lcitool_vars() helper

[Erik Skultety]

On Wed, May 31, 2023 at 10:09:05PM +0200, Philippe Mathieu-Daudé wrote:
> The 'lcitool variables $OS qemu' command produces a file containing
> consistent environment variables helpful to build QEMU on $OS.
> In particular the $PKGS variable contains the packages required to
> build QEMU.
> 
> Since some of these files are committed in the repository (see
> 0e103a65ba "gitlab: support for FreeBSD 12, 13 and macOS 11 via
> cirrus-run"), we can parse these files to get the package list
> required to build a VM.
> 
> Add the get_qemu_packages_from_lcitool_vars() helper which return
> such package list from a lcitool env var file.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/vm/basevm.py | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> index 8ec021ddcf..2632c3d41a 100644
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -522,6 +522,12 @@ def get_qemu_version(qemu_path):
>      version_num = re.split(' |\(', version_line)[3].split('.')[0]
>      return int(version_num)
>  
> +def get_qemu_packages_from_lcitool_vars(vars_path):
> +    """Parse a lcitool variables file and return the PKGS list."""
> +    with open(vars_path, 'r') as fd:
> +        line = list(filter(lambda y: y.startswith('PKGS'), fd.readlines()))[0]
> +        return line.split("'")[1].split()

Nothing wrong with this one, it's also less lines of code, but just an FYI in
case you wanted a slightly more readable (yet a tiny bit less performant piece
of code) you could make use of the JSON format with 'variables --format json'.

Regards,
Erik

Re: [PATCH 0/3] tests/vm/freebsd: Get up-to-date package list from lcitool

[Erik Skultety]

On Wed, May 31, 2023 at 10:09:03PM +0200, Philippe Mathieu-Daudé wrote:
> Inspired by this patch from Thomas:
> https://lore.kernel.org/qemu-devel/20230531090415.40421-1-thuth@redhat.com/
> 
> Instead of updating the package list manually, use lcitool vars file.
> 
> Philippe Mathieu-Daudé (3):
>   tests/vm: Pass project source path to build_image()
>   tests/vm: Introduce get_qemu_packages_from_lcitool_vars() helper
>   tests/vm/freebsd: Get up-to-date package list from lcitool vars file
> 
>  tests/vm/Makefile.include |  1 +
>  tests/vm/basevm.py        | 10 +++++++--
>  tests/vm/centos           |  2 +-
>  tests/vm/centos.aarch64   |  2 +-
>  tests/vm/freebsd          | 46 +++++----------------------------------
>  tests/vm/haiku.x86_64     |  2 +-
>  tests/vm/netbsd           |  2 +-
>  tests/vm/openbsd          |  2 +-
>  tests/vm/ubuntuvm.py      |  2 +-
>  9 files changed, 20 insertions(+), 49 deletions(-)
> 
> -- 
> 2.38.1
> 

From my POV the changes are pretty straight-forward, but I'll let others who
use this code on pretty much daily basis to comment. What I'll say though is
that I only just now looked at the Python VM spawner helpers you have and it
left me wondering whether there'd be interest in dropping much of the code in
favour of lcitool invocations - not all, not even trying to say it would be
100% smooth, but it would
1) give lcitool even bigger exposure
2) find bugs
3) generalize the functionality even more
4) open doors for more features needed by the community

Specifically looking at the vm/centos source file, we recently added support
for downloading guests from vendor-provided cloud images (asking libosinfo for
the URL to download from )which are cached essentially the same way as your
code does and then provisions it using a storage overlay. Package installation
and other configuration is done via Ansible, so root SSH is taken care of,
serial console is force enabled, etc.

If the community were interested in migrating the aforementioned functionality
to lcitool wrt/ provisioning local VMs (no FreeBSD at the moment though :(  [1])
I'd be happy to help with that.

Regards,
Erik

[1] recently I saw a statement somewhere that FreeBSD is finally considering
providing official cloud-init template images and that it should happen some
time this year, so knowing this I'd rather wait some more than to add yet
another code to special-case the cloud-init provisions with lcitool for FreeBSD

Re: [PATCH 3/3] tests/vm/freebsd: Get up-to-date package list from lcitool vars file

[Daniel P. Berrangé]

On Wed, May 31, 2023 at 10:09:06PM +0200, Philippe Mathieu-Daudé wrote:
> See previous commit for rationale on using lcitool vars file to
> get an up-to-date package list. Since there is a such file generated
> for FreeBSD 13 available in the repository, use it. That way we
> don't need to manually keep this array in sync.
> 
> Inspired-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  tests/vm/freebsd | 44 ++++----------------------------------------
>  1 file changed, 4 insertions(+), 40 deletions(-)
> 
> diff --git a/tests/vm/freebsd b/tests/vm/freebsd
> index 6a0d7a4569..4f12878645 100755
> --- a/tests/vm/freebsd
> +++ b/tests/vm/freebsd
> @@ -31,45 +31,7 @@ class FreeBSDVM(basevm.BaseVM):
>      link = "https://download.freebsd.org/releases/CI-IMAGES/13.2-RELEASE/amd64/Latest/FreeBSD-13.2-RELEASE-amd64-BASIC-CI.raw.xz"
>      csum = "a4fb3b6c7b75dd4d58fb0d75e4caf72844bffe0ca00e66459c028b198ffb3c0e"
>      size = "20G"
> -    pkgs = [
> -        # build tools
> -        "git",
> -        "pkgconf",
> -        "bzip2",
> -        "python39",
> -        "ninja",
> -
> -        # gnu tools
> -        "bash",
> -        "gmake",
> -        "gsed",
> -        "gettext",
> -
> -        # libs: crypto
> -        "gnutls",
> -
> -        # libs: images
> -        "jpeg-turbo",
> -        "png",
> -
> -        # libs: ui
> -        "sdl2",
> -        "gtk3",
> -        "libxkbcommon",
> -
> -        # libs: opengl
> -        "libepoxy",
> -        "mesa-libs",
> -
> -        # libs: migration
> -        "zstd",
> -
> -        # libs: networking
> -        "libslirp",
> -
> -        # libs: sndio
> -        "sndio",
> -    ]
> +    lcitool_vars = ".gitlab-ci.d/cirrus/freebsd-13.vars"

So we have various other distros in the tests/vm/ directory, for which
we have lcitool support, but for which there's no existing vars file.

I'm wondering if we're better off just putting the data files we need
directly in the tests/vm/ directory, and keeping it indepenant of the
cirrus CI data files. It is all auto-generated, so the duplication
would not be a maint burden.

eg just have a 'freebsd.json' file, alongside the 'freebsd' script.

We should also expand lcitool to cover haiku, netbsd and openbsd
distros one day.


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 :|

Re: [PATCH 2/3] tests/vm: Introduce get_qemu_packages_from_lcitool_vars() helper

[Daniel P. Berrangé]

On Thu, Jun 01, 2023 at 09:36:27AM +0200, Erik Skultety wrote:
> On Wed, May 31, 2023 at 10:09:05PM +0200, Philippe Mathieu-Daudé wrote:
> > The 'lcitool variables $OS qemu' command produces a file containing
> > consistent environment variables helpful to build QEMU on $OS.
> > In particular the $PKGS variable contains the packages required to
> > build QEMU.
> > 
> > Since some of these files are committed in the repository (see
> > 0e103a65ba "gitlab: support for FreeBSD 12, 13 and macOS 11 via
> > cirrus-run"), we can parse these files to get the package list
> > required to build a VM.
> > 
> > Add the get_qemu_packages_from_lcitool_vars() helper which return
> > such package list from a lcitool env var file.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >  tests/vm/basevm.py | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/vm/basevm.py b/tests/vm/basevm.py
> > index 8ec021ddcf..2632c3d41a 100644
> > --- a/tests/vm/basevm.py
> > +++ b/tests/vm/basevm.py
> > @@ -522,6 +522,12 @@ def get_qemu_version(qemu_path):
> >      version_num = re.split(' |\(', version_line)[3].split('.')[0]
> >      return int(version_num)
> >  
> > +def get_qemu_packages_from_lcitool_vars(vars_path):
> > +    """Parse a lcitool variables file and return the PKGS list."""
> > +    with open(vars_path, 'r') as fd:
> > +        line = list(filter(lambda y: y.startswith('PKGS'), fd.readlines()))[0]
> > +        return line.split("'")[1].split()
> 
> Nothing wrong with this one, it's also less lines of code, but just an FYI in
> case you wanted a slightly more readable (yet a tiny bit less performant piece
> of code) you could make use of the JSON format with 'variables --format json'.

Specifically we could do

   with open(vars_path, 'r') as fh:
     return json.load(fh)['pkgs']


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 :|