[libvirt] [jenkins-ci PATCH v2 9/9] docker: add a makefile for building docker images locally

[Andrea Bolognani]

On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote:
[...]
> +++ b/dockerfiles/Makefile

So this is basically very similar to

  https://libvirt.org/git/?p=libvirt-dockerfiles.git;a=blob;f=refresh

and I feel like it belongs to that repository rather than this one.

That causes a bit of inconvenience, but on the other hand I expect
that regular libvirt developers will *not* perform local builds, and
will rely on the official images from quay.io instead - and your
proposed libvirt integration patches also seem to assume as much.

So the only people affected by this would be the CI maintainers, and
for them sticking with the current practice of generating Dockerfiles
in a separate repository shouldn't be a problem.

[...]
> +HOSTS = $(shell $(LCITOOL) -a hosts)
> +
> +CROSS_HOST = libvirt-debian-9
> +
> +ARCHES = $(shell $(LCITOOL) -a arches -h $(CROSS_HOST))

HOSTS, CROSS_HOST and ARCHES are not used anywhere, at least as far
as I can tell. Probably leftovers from a previous version :)

[...]
> +buildenv-%.docker: $(LCITOOL) $(CONFIGS)
> +	case $@ in  \
> +	    *cross*) \
> +	        HOST=`echo $@ | sed -e 's/-cross-.*//' -e 's/buildenv-//' -e 's/.docker//'` && \
> +	        ARCH=`echo $@ | sed -e 's/^.*-cross-//' -e 's/.docker//'` && \
> +	        $(LCITOOL) -a dockerfile -p libvirt -h $$HOST -x $$ARCH > $@ ;; \
> +	    *) \
> +	        HOST=`echo $@ | sed -e 's/buildenv-//' -e 's/.docker//'` && \
> +	        $(LCITOOL) -a dockerfile -p libvirt -h $$HOST > $@ ;; \
> +	esac

If you look at the script mentioned above, you'll see that this
won't quite work: when generating the Dockerfile for Fedora Rawhide,
we need to include the libvirt+mingw* projects too.

Which brings me back to the idea I suggested at the end of the
previous message, or rather an extended version of it: what if,
instead of weighing down the Fedora Rawhide image with the MinGW
compiler and libraries, and having a bunch of cross compilation
images, we only had

  buildenv-mingw
  buildenv-cross

with the latter including cross-compilers for all architectures?

I think splitting the MinGW builds from regular Fedora Rawhide
builds might even improve responsiveness on ci.centos.org, since
we'd be able to run the two in parallel.

[...]
> +++ b/guests/lcitool
> @@ -313,8 +313,9 @@ class Application:
>                    build    build projects on hosts
>  
>                  informational actions:
> -                  hosts     list all known hosts
> -                  projects  list all known projects
> +                  hosts        list all known hosts
> +                  projects     list all known projects
> +                  dockerfiles  list all known dockerfiles

Though it's not quite a clear call, I would still probably list this
under "uncommon actions", because it's mostly for our own (scripting)
convenience and we don't expect users to run it direcly.

It also should have been added in its own commit.

[...]
> +    def _action_dockerfiles(self, _hosts, _projects, _revision, _cross_arch):
> +        for host in self._inventory.expand_pattern("all"):
> +            facts = self._inventory.get_facts(host)
> +            package_format = facts["package_format"]
> +            if package_format not in ["deb", "rpm"]:
> +                continue
> +            print ("buildenv-%s.docker" % host)

No whitespace between function name and parentheses.

And... Hold on a second. Are you really trying to sneak a *third*
way to do string formatting into the script?!? :P

> +            for arch in facts.get("cross_build", {}).get("arches", {}).keys():
> +                print ("buildenv-%s-cross-%s.docker" % (host, arch))

Two problems with this, both of which are shared with the non-cross
variant above: first, it will result in output like

  buildenv-libvirt-debian-9-cross-s390x.docker

but so far we have omitted the "libvirt" part, at least when it
comes to the official images hosted at

  https://quay.io/libvirt

Of course for those images we can get away with it because "libvirt"
is part of the URL, when performing local builds we can't really
count on that...

Then again, see the argument above about maintainers being the only
one expected to run local builds, which makes it IMHO perfectly fine
to stick with the "libvirt"-less names for images.

The second problem is the use of the .docker extension. It seems
like the best practice is to have something like

  buildenv-fedora-29/Dockerfile

but that would complicate managing the files further with not much
benefit that I can see; using

  buildenv-fedora-29.Dockerfile

instead, as seen in the libvirt-dockerfiles.git repository, seems
like a sensible enough middle ground, so I'd prefer sticking with
that naming scheme.

-- 
Andrea Bolognani / Red Hat / Virtualization