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
