[libvirt] [jenkins-ci PATCH v2 0/9] Add support for cross compiling libvirt via Debian
Changed in v2: - Fix multiple package name mistakes - Modify lcitool to generate cross-arch docker files - Add --no-install-recommended flag to apt-get - Add DEBIAN_FRONTEND=noninteractive env to apt-get - Improve error reporting in lcitool - Add make rule for generating dockerfiles locally Daniel P. Berrangé (9): guests: use libpcap0.8-dev package on Debian guests: add xfsprogs development package for libvirt guests: fix glusterfs package name on Debian lcitool: include root cause when failing to load facts lcitool: force non-interactive apt-get frontend lcitool: avoid installing recommended packages lcitool: avoid using an env var to store package list lcitool: support generating cross compiler dockerfiles docker: add a makefile for building docker images locally .gitignore | 1 + dockerfiles/Makefile | 33 +++++ guests/host_vars/libvirt-debian-9/docker.yml | 57 ++++++++ .../host_vars/libvirt-debian-sid/docker.yml | 62 +++++++++ guests/lcitool | 123 ++++++++++++++---- guests/vars/mappings.yml | 9 +- guests/vars/projects/libvirt.yml | 1 + 7 files changed, 257 insertions(+), 29 deletions(-) create mode 100644 dockerfiles/Makefile -- 2.20.1
The libpcap-dev package is a temporary backcompat package everything switches to the new libpcap0.8-dev pacakge name.x Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/vars/mappings.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index 84543c4..f211169 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -281,7 +281,7 @@ mappings: rpm: parted-devel libpcap: - deb: libpcap-dev + deb: libpcap0.8-dev pkg: libpcap rpm: libpcap-devel -- 2.20.1
On Tue, 2019-02-05 at 17:52 +0000, Daniel P. Berrangé wrote:
The libpcap-dev package is a temporary backcompat package everything switches to the new libpcap0.8-dev pacakge name.x
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/vars/mappings.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
On Tue, 2019-02-05 at 17:52 +0000, Daniel P. Berrangé wrote:
The libpcap-dev package is a temporary backcompat package everything switches to the new libpcap0.8-dev pacakge name.x
Oh, before you push the patch please fix the commit message: The libpcap-dev package is a temporary backcompat package until everything switches to the new libpcap0.8-dev package name. -- Andrea Bolognani / Red Hat / Virtualization
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/vars/mappings.yml | 5 +++++ guests/vars/projects/libvirt.yml | 1 + 2 files changed, 6 insertions(+) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index f211169..a07d090 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -815,6 +815,11 @@ mappings: deb: libxen-dev Fedora: xen-devel + xfsprogs: + deb: xfslibs-dev + rpm: xfsprogs-devel + default: + xmllint: default: libxml2 deb: libxml2-utils diff --git a/guests/vars/projects/libvirt.yml b/guests/vars/projects/libvirt.yml index 605d201..031b9ff 100644 --- a/guests/vars/projects/libvirt.yml +++ b/guests/vars/projects/libvirt.yml @@ -54,6 +54,7 @@ packages: - wireshark - xen - xmllint + - xfsprogs - xsltproc - yajl - zfs -- 2.20.1
On Tue, 2019-02-05 at 17:52 +0000, Daniel P. Berrangé wrote: [...]
+ xfsprogs: + deb: xfslibs-dev + rpm: xfsprogs-devel + default:
You can leave the last line out, as the default is assumed to be empty unless explicitly specified. [...]
- xen - xmllint + - xfsprogs
The sorting is wrong here: xfsprogs goes between xen and xmllint. With the above fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
We want the development headers not the client binary Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/vars/mappings.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index a07d090..824808d 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -141,7 +141,7 @@ mappings: rpm: glibc-static glusterfs: - deb: glusterfs-client + deb: libglusterfs-dev rpm: glusterfs-api-devel gnome-common: -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
glusterfs: - deb: glusterfs-client + deb: libglusterfs-dev rpm: glusterfs-api-devel
This won't work, because before Debian Sid the libglusterfs-dev package didn't exist and header files were included in the glusterfs-common package. So the correct solution is to do either glusterfs: deb: glusterfs-common rpm: glusterfs-api-devel DebianSid: libglusterfs-dev or the more future-proof (and verbose) glusterfs: deb: libglusterfs-dev rpm: glusterfs-api-devel Debian8: glusterfs-common Debian9: glusterfs-common Ubuntu16: glusterfs-common Ubuntu18: glusterfs-common I'd probably go for the latter. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 06, 2019 at 03:58:37PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
glusterfs: - deb: glusterfs-client + deb: libglusterfs-dev rpm: glusterfs-api-devel
This won't work, because before Debian Sid the libglusterfs-dev package didn't exist and header files were included in the glusterfs-common package.
So the correct solution is to do either
glusterfs: deb: glusterfs-common rpm: glusterfs-api-devel DebianSid: libglusterfs-dev
or the more future-proof (and verbose)
glusterfs: deb: libglusterfs-dev rpm: glusterfs-api-devel Debian8: glusterfs-common Debian9: glusterfs-common Ubuntu16: glusterfs-common Ubuntu18: glusterfs-common
I'd probably go for the latter.
Yeah preferring the future is better 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 :|
On Wed, 2019-02-06 at 15:57 +0000, Daniel P. Berrangé wrote:
On Wed, Feb 06, 2019 at 03:58:37PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
glusterfs: - deb: glusterfs-client + deb: libglusterfs-dev rpm: glusterfs-api-devel
This won't work, because before Debian Sid the libglusterfs-dev package didn't exist and header files were included in the glusterfs-common package.
So the correct solution is to do either
glusterfs: deb: glusterfs-common rpm: glusterfs-api-devel DebianSid: libglusterfs-dev
or the more future-proof (and verbose)
glusterfs: deb: libglusterfs-dev rpm: glusterfs-api-devel Debian8: glusterfs-common Debian9: glusterfs-common Ubuntu16: glusterfs-common Ubuntu18: glusterfs-common
I'd probably go for the latter.
Yeah preferring the future is better
Alright then! Reviewed-by: Andrea Bolognani <abologna@redhat.com> with that addressed. -- Andrea Bolognani / Red Hat / Virtualization
The root cause exception contains the useful information about what failed during loading. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 759eff6..4fb0008 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -202,8 +202,9 @@ class Inventory: try: self._facts[host] = self._read_all_facts(host) self._facts[host]["inventory_hostname"] = host - except Exception: - raise Error("Can't load facts for '{}'".format(host)) + except Exception as ex: + raise Error("Can't load facts for '%(host)s': %(ex)s" % + { "host": host, "ex": ex}) @staticmethod def _add_facts_from_file(facts, yaml_path): -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
@@ -202,8 +202,9 @@ class Inventory: try: self._facts[host] = self._read_all_facts(host) self._facts[host]["inventory_hostname"] = host - except Exception: - raise Error("Can't load facts for '{}'".format(host)) + except Exception as ex: + raise Error("Can't load facts for '%(host)s': %(ex)s" % + { "host": host, "ex": ex})
Did you actually run into a situation where this was useful? It's one of those diagnostics that I didn't really expect to trigger in practice... Either way, I don't really have a problem with adding it, but what I don't like is using a completely different way to format strings than the rest of the code. I don't use Python nearly enough to have an opinion on the merits of either syntax compared to the other, so if the one you're using here is considered a best practice then we can definitely switch to it; however, it will have to be done in a separate commit that converts the entire script at once rather than in a piecemail fashion. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 06, 2019 at 04:53:46PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
@@ -202,8 +202,9 @@ class Inventory: try: self._facts[host] = self._read_all_facts(host) self._facts[host]["inventory_hostname"] = host - except Exception: - raise Error("Can't load facts for '{}'".format(host)) + except Exception as ex: + raise Error("Can't load facts for '%(host)s': %(ex)s" % + { "host": host, "ex": ex})
Did you actually run into a situation where this was useful? It's one of those diagnostics that I didn't really expect to trigger in practice...
Yes, if you create malformed yaml file this triggers. It means the error message now tells you the place where the yaml syntax error is
Either way, I don't really have a problem with adding it, but what I don't like is using a completely different way to format strings than the rest of the code.
I don't use Python nearly enough to have an opinion on the merits of either syntax compared to the other, so if the one you're using here is considered a best practice then we can definitely switch to it; however, it will have to be done in a separate commit that converts the entire script at once rather than in a piecemail fashion.
I've never come across the "{}" syntax before so didn't even know how to use named params for it until now. 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 :|
On Wed, Feb 06, 2019 at 03:56:37PM +0000, Daniel P. Berrangé wrote:
On Wed, Feb 06, 2019 at 04:53:46PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
@@ -202,8 +202,9 @@ class Inventory: try: self._facts[host] = self._read_all_facts(host) self._facts[host]["inventory_hostname"] = host - except Exception: - raise Error("Can't load facts for '{}'".format(host)) + except Exception as ex: + raise Error("Can't load facts for '%(host)s': %(ex)s" % + { "host": host, "ex": ex})
Did you actually run into a situation where this was useful? It's one of those diagnostics that I didn't really expect to trigger in practice...
Yes, if you create malformed yaml file this triggers. It means the error message now tells you the place where the yaml syntax error is
Either way, I don't really have a problem with adding it, but what I don't like is using a completely different way to format strings than the rest of the code.
I don't use Python nearly enough to have an opinion on the merits of either syntax compared to the other, so if the one you're using here is considered a best practice then we can definitely switch to it; however, it will have to be done in a separate commit that converts the entire script at once rather than in a piecemail fashion.
I've never come across the "{}" syntax before so didn't even know how to use named params for it until now.
I kind of agree with Andrea, the preferred formatting in python is to use {} and .format() method, the '%' formatting is old and should not be used. In this case you have these options: "Can't load facts for '{}': {}".format(host, ex) "Can't load facts for '{0}': {1}".format(host, ex) "Can't load facts for '{host}': {ex}".format(host=host, ex=ex) Pavel
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 4fb0008..1d5cbd0 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -542,11 +542,14 @@ class Application: if package_format == "deb": sys.stdout.write(textwrap.dedent(""" - RUN apt-get update && \\ - apt-get dist-upgrade -y && \\ - apt-get install -y ${PACKAGES} && \\ - apt-get autoremove -y && \\ - apt-get autoclean -y + RUN DEBIAN_FRONTEND=noninteractive && \\ + ( \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install -y ${PACKAGES} && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y \\ + ) """)) elif package_format == "rpm": if os_name == "Fedora" and os_version == "Rawhide": -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
if package_format == "deb": sys.stdout.write(textwrap.dedent(""" - RUN apt-get update && \\ - apt-get dist-upgrade -y && \\ - apt-get install -y ${PACKAGES} && \\ - apt-get autoremove -y && \\ - apt-get autoclean -y + RUN DEBIAN_FRONTEND=noninteractive && \\ + ( \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install -y ${PACKAGES} && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y \\ + )
Wouldn't RUN export DEBIAN_FRONTEND=noninteractive && \\ apt-get update && \\ ... work as well, without requiring the extra indentation (and shell)? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Feb 06, 2019 at 05:21:36PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
if package_format == "deb": sys.stdout.write(textwrap.dedent(""" - RUN apt-get update && \\ - apt-get dist-upgrade -y && \\ - apt-get install -y ${PACKAGES} && \\ - apt-get autoremove -y && \\ - apt-get autoclean -y + RUN DEBIAN_FRONTEND=noninteractive && \\ + ( \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install -y ${PACKAGES} && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y \\ + )
Wouldn't
RUN export DEBIAN_FRONTEND=noninteractive && \\ apt-get update && \\ ...
work as well, without requiring the extra indentation (and shell)?
Oh, true, I could have done that 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 :|
We know exactly which packages we need and don't want apt picking extra "recommended" ones for us. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guests/lcitool b/guests/lcitool index 1d5cbd0..eb111b8 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -546,7 +546,7 @@ class Application: ( \\ apt-get update && \\ apt-get dist-upgrade -y && \\ - apt-get install -y ${PACKAGES} && \\ + apt-get install --no-install-recommends -y ${PACKAGES} && \\ apt-get autoremove -y && \\ apt-get autoclean -y \\ ) -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote:
We know exactly which packages we need and don't want apt picking extra "recommended" ones for us.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
Every statement in a dockerfile results in a new layer in the image. There is no need for an env var to store the package list when it can be included inline. This avoids the env variable being later exposed to the container at runtime. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index eb111b8..cd757eb 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -530,43 +530,46 @@ class Application: if os_full in mappings[package]: temp[package] = mappings[package][os_full] - flattened = [] + pkgs = [] for item in temp: - if temp[item] is not None and temp[item] not in flattened: - flattened += [temp[item]] + pkgname = temp[item] + if pkgname is None: + continue + if pkgname not in pkgs: + pkgs.append(pkgname) print("FROM {}".format(facts["docker_base"])) - sys.stdout.write("ENV PACKAGES ") - sys.stdout.write(" \\\n ".join(sorted(flattened))) - + varmap = {} + varmap["pkgs"] = "".join([" \\\n " + pkgname + for pkgname in sorted(pkgs)]) if package_format == "deb": sys.stdout.write(textwrap.dedent(""" RUN DEBIAN_FRONTEND=noninteractive && \\ ( \\ apt-get update && \\ apt-get dist-upgrade -y && \\ - apt-get install --no-install-recommends -y ${PACKAGES} && \\ + apt-get install --no-install-recommends -y %(pkgs)s && \\ apt-get autoremove -y && \\ apt-get autoclean -y \\ ) - """)) + """) % varmap ) elif package_format == "rpm": if os_name == "Fedora" and os_version == "Rawhide": sys.stdout.write(textwrap.dedent(""" RUN yum update -y --nogpgcheck fedora-gpg-keys && \\ yum update -y && \\ - yum install -y ${PACKAGES} && \\ + yum install -y %(pkgs)s && \\ yum autoremove -y && \\ yum clean all -y - """)) + """) % varmap ) else: sys.stdout.write(textwrap.dedent(""" RUN yum update -y && \\ - yum install -y ${PACKAGES} && \\ + yum install -y %(pkgs)s && \\ yum autoremove -y && \\ yum clean all -y - """)) + """) % varmap ) def run(self): cmdline = self._parser.parse_args() -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
- flattened = [] + pkgs = [] for item in temp: - if temp[item] is not None and temp[item] not in flattened: - flattened += [temp[item]] + pkgname = temp[item] + if pkgname is None: + continue + if pkgname not in pkgs: + pkgs.append(pkgname)
Nothing against refactoring the code this way, but such a change belongs in its own patch. [...]
- sys.stdout.write("ENV PACKAGES ") - sys.stdout.write(" \\\n ".join(sorted(flattened))) - + varmap = {} + varmap["pkgs"] = "".join([" \\\n " + pkgname + for pkgname in sorted(pkgs)])
Unlike the original, this pointlessly loops through sorted(pkgs) one additional time and adds a phantom element to the beginning of the list. So, once you put everything together...
if package_format == "deb": sys.stdout.write(textwrap.dedent(""" RUN DEBIAN_FRONTEND=noninteractive && \\ ( \\ apt-get update && \\ apt-get dist-upgrade -y && \\ - apt-get install --no-install-recommends -y ${PACKAGES} && \\ + apt-get install --no-install-recommends -y %(pkgs)s && \\ apt-get autoremove -y && \\ apt-get autoclean -y \\ )
... the result looks like apt-get install --no-install-recommends -y \ augeas-tools \ autoconf \ automake \ ... Notice the double space between '-y' and '\' as well as the weird indentation. A better solution would be to do varmap["pkgs"] = " \\\n ".join(sorted(pkgs)) followed by sys.stdout.write(textwrap.dedent(""" ... apt-get install --no-install-recommends -y \\ %(pkgs)s && \\ apt-get autoremove -y && \\ ... which is clearer and results in the more pleasant apt-get install --no-install-recommends -y \ augeas-tools \ autoconf \ automake \ ...
- """)) + """) % varmap )
Again, I don't have a problem with using this syntax for string formatting (and here it's actually much clearer than the one we are currently using), but I don't want the script to become inconsistent so we have to stick to a single style. -- Andrea Bolognani / Red Hat / Virtualization
Debian's filesystem layout has a nice advantage over Fedora which is that it can install non-native RPMs in the main root filesystem. It is thus possible to prepare an x86_64 filesystem containing -dev packages for a foreign architecture, along with a GCC cross compiler. QEMU has used this technique to facilitate developer build testing of non-x86 architectures, since few people have access to physical hardware for most of these architectures. For the same reason it would be helpful to libvirt developers. This patch extends the 'dockerfile' command to 'lcitool' so that it accepts a '-x $ARCH' argument. $ lcitool -a dockerfile -h libvirt-debian-9 -p libvirt -x s390x This is only valid when using a 'deb' based distro. With the Debian 9 distro, this supports arm64, armel, armhf, mips, mipsel, mips64el, ppc64el, s390x, which are all the official archs that Debian maintainers currently build libvirt for. With Debian Sid, this is extended to include i386. Debian also builds libvirt on hppa, powerpcspe, sparc64 and x32 which are unofficial ports. Unfortunately it is not possible to reliably add foreign arch packages from the unofficial ports in parallel with those from the native arch, without hitting dependency problems from apt. When an architecture is given, any package name ending in '-dev' will be installed using that architecture variant, while all remaining packages will have their native variant installed. For various reasons (commented inline) a few blacklists are required on a per-arch basis. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/host_vars/libvirt-debian-9/docker.yml | 57 +++++++++++++++ .../host_vars/libvirt-debian-sid/docker.yml | 62 +++++++++++++++++ guests/lcitool | 69 ++++++++++++++++--- 3 files changed, 179 insertions(+), 9 deletions(-) diff --git a/guests/host_vars/libvirt-debian-9/docker.yml b/guests/host_vars/libvirt-debian-9/docker.yml index 0b4ccee..0e613da 100644 --- a/guests/host_vars/libvirt-debian-9/docker.yml +++ b/guests/host_vars/libvirt-debian-9/docker.yml @@ -1,2 +1,59 @@ --- docker_base: debian:9 +cross_build: + blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses + - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev + arches: + arm64: + gcc-pkg-prefix: crossbuild-essential-arm64 + target-prefix: aarch64-linux-gnu + armel: + gcc-pkg-prefix: crossbuild-essential-armel + target-prefix: arm-linux-gnueabi + blacklist: + # Not built on this arch + - libxen-dev + # Arch does not support NUMA concept + - libnuma-dev + armhf: + gcc-pkg-prefix: crossbuild-essential-armhf + target-prefix: arm-linux-gnueabihf + blacklist: + # Arch does not support NUMA concept + - libnuma-dev + mips64el: + gcc-pkg-prefix: gcc-mips64el-linux-gnuabi64 + target-prefix: mips64el-linux-gnuabi64 + blacklist: + # Not built on this arch + - libxen-dev + mips: + gcc-pkg-prefix: gcc-mips-linux-gnu + target-prefix: mips-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + mipsel: + gcc-pkg-prefix: gcc-mipsel-linux-gnu + target-prefix: mipsel-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + ppc64el: + gcc-pkg-prefix: crossbuild-essential-ppc64el + target-prefix: powerpc64le-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + s390x: + gcc-pkg-prefix: gcc-multilib-s390x-linux-gnu + target-prefix: s390x-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev diff --git a/guests/host_vars/libvirt-debian-sid/docker.yml b/guests/host_vars/libvirt-debian-sid/docker.yml index e20a37e..61ecc1f 100644 --- a/guests/host_vars/libvirt-debian-sid/docker.yml +++ b/guests/host_vars/libvirt-debian-sid/docker.yml @@ -1,2 +1,64 @@ --- docker_base: debian:sid +cross_build: + blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses + - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev + # appears to be dropped from the 'sid' tree + - sheepdog + arches: + arm64: + gcc-pkg-prefix: crossbuild-essential-arm64 + target-prefix: aarch64-linux-gnu + armel: + gcc-pkg-prefix: crossbuild-essential-armel + target-prefix: arm-linux-gnueabi + blacklist: + # Not built on this arch + - libxen-dev + # Arch does not support NUMA concept + - libnuma-dev + armhf: + gcc-pkg-prefix: crossbuild-essential-armhf + target-prefix: arm-linux-gnueabihf + blacklist: + # Arch does not support NUMA concept + - libnuma-dev + mips64el: + gcc-pkg-prefix: gcc-mips64el-linux-gnuabi64 + target-prefix: mips64el-linux-gnuabi64 + blacklist: + # Not built on this arch + - libxen-dev + mips: + gcc-pkg-prefix: gcc-mips-linux-gnu + target-prefix: mips-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + mipsel: + gcc-pkg-prefix: gcc-mipsel-linux-gnu + target-prefix: mipsel-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + ppc64el: + gcc-pkg-prefix: crossbuild-essential-ppc64el + target-prefix: powerpc64le-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + s390x: + gcc-pkg-prefix: gcc-multilib-s390x-linux-gnu + target-prefix: s390x-linux-gnu + blacklist: + # Not built on this arch + - libxen-dev + i386: + gcc-pkg-prefix: gcc-multilib-i686-linux-gnu + target-prefix: i686-linux-gnu diff --git a/guests/lcitool b/guests/lcitool index cd757eb..dc5741e 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -343,6 +343,11 @@ class Application: metavar="GIT_REVISION", help="git revision to build (remote/branch)", ) + self._parser.add_argument( + "-x", + metavar="CROSS-ARCH", + help="cross compiler architecture (dockerfile action only)", + ) def _execute_playbook(self, playbook, hosts, projects, git_revision): base = Util.get_base() @@ -402,15 +407,15 @@ class Application: except Exception: raise Error("Failed to run {} on '{}'".format(playbook, hosts)) - def _action_hosts(self, _hosts, _projects, _revision): + def _action_hosts(self, _hosts, _projects, _revision, _cross_arch): for host in self._inventory.expand_pattern("all"): print(host) - def _action_projects(self, _hosts, _projects, _revision): + def _action_projects(self, _hosts, _projects, _revision, _cross_arch): for project in self._projects.expand_pattern("all"): print(project) - def _action_install(self, hosts, _projects, _revision): + def _action_install(self, hosts, _projects, _revision, _cross_arch): base = Util.get_base() flavor = self._config.get_flavor() @@ -482,13 +487,13 @@ class Application: except Exception: raise Error("Failed to install '{}'".format(host)) - def _action_update(self, hosts, projects, git_revision): + def _action_update(self, hosts, projects, git_revision, _cross_arch): self._execute_playbook("update", hosts, projects, git_revision) - def _action_build(self, hosts, projects, git_revision): + def _action_build(self, hosts, projects, git_revision, _cross_arch): self._execute_playbook("build", hosts, projects, git_revision) - def _action_dockerfile(self, hosts, projects, _revision): + def _action_dockerfile(self, hosts, projects, _revision, cross_arch): mappings = self._projects.get_mappings() hosts = self._inventory.expand_pattern(hosts) @@ -501,10 +506,24 @@ class Application: os_name = facts["os_name"] os_version = facts["os_version"] os_full = os_name + os_version + blacklist = [] + cross_build_facts = None if package_format not in ["deb", "rpm"]: raise Error("Host {} doesn't support Dockerfiles".format(host)) + if cross_arch is not None: + if package_format != "deb": + raise Error("cross compilers only supported for debian packages") + if "cross_build" not in facts: + raise Error("cross compilers not supported for this host") + if cross_arch not in facts["cross_build"]["arches"]: + raise Error("unsupported cross compiler architecture, use one of {}".format( + ", ".join(facts["cross_build"]["arches"].keys()))) + cross_build_facts = facts["cross_build"]["arches"][cross_arch] + blacklist.extend(facts["cross_build"].get("blacklist", [])) + blacklist.extend(cross_build_facts.get("blacklist", [])) + projects = self._projects.expand_pattern(projects) for project in projects: if project not in facts["projects"]: @@ -531,18 +550,31 @@ class Application: temp[package] = mappings[package][os_full] pkgs = [] + cross_pkgs = [] for item in temp: pkgname = temp[item] if pkgname is None: continue - if pkgname not in pkgs: - pkgs.append(pkgname) + if pkgname in blacklist: + continue + if cross_arch and pkgname[-4:] == "-dev": + if pkgname not in cross_pkgs: + cross_pkgs.append(pkgname + ":" + cross_arch) + else: + if pkgname not in pkgs: + pkgs.append(pkgname) print("FROM {}".format(facts["docker_base"])) varmap = {} varmap["pkgs"] = "".join([" \\\n " + pkgname for pkgname in sorted(pkgs)]) + if cross_arch: + varmap["cross_arch"] = cross_arch + varmap["cross_pkgs"] = "".join([" \\\n " + pkgname + for pkgname in sorted(cross_pkgs)]) + varmap["cross_gcc"] = cross_build_facts["gcc-pkg-prefix"] + varmap["cross_prefix"] = cross_build_facts["target-prefix"] if package_format == "deb": sys.stdout.write(textwrap.dedent(""" RUN DEBIAN_FRONTEND=noninteractive && \\ @@ -554,6 +586,24 @@ class Application: apt-get autoclean -y \\ ) """) % varmap ) + if cross_arch: + # Intentionally a separate RUN command from the above + # so that the common packages of all cross-built images + # share a docker image layer + sys.stdout.write(textwrap.dedent(""" + RUN DEBIAN_FRONTEND=noninteractive && \\ + ( \\ + dpkg --add-architecture %(cross_arch)s && \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install --no-install-recommends -y %(cross_gcc)s %(cross_pkgs)s && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y \\ + ) + ENV TARGET "%(cross_prefix)s" + ENV CONFIGURE_OPTS "--host=%(cross_prefix)s --target=%(cross_prefix)s" + ENV PKG_CONFIG_LIBDIR "/usr/lib/%(cross_prefix)s/pkgconfig" + """) % varmap ) elif package_format == "rpm": if os_name == "Fedora" and os_version == "Rawhide": sys.stdout.write(textwrap.dedent(""" @@ -577,11 +627,12 @@ class Application: hosts = cmdline.h projects = cmdline.p git_revision = cmdline.g + cross_arch = cmdline.x method = "_action_{}".format(action.replace("-", "_")) if hasattr(self, method): - getattr(self, method).__call__(hosts, projects, git_revision) + getattr(self, method).__call__(hosts, projects, git_revision, cross_arch) else: raise Error("Invalid action '{}'".format(action)) -- 2.20.1
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
With the Debian 9 distro, this supports arm64, armel, armhf, mips, mipsel, mips64el, ppc64el, s390x, which are all the official archs that Debian maintainers currently build libvirt for. With Debian Sid, this is extended to include i386.
Is i386 really not supported as a cross compilation target on Debian 9? It seems like it would be such a low hanging fruit... [...]
+++ b/guests/host_vars/libvirt-debian-9/docker.yml @@ -1,2 +1,59 @@ --- docker_base: debian:9 +cross_build:
I don't think this belongs in docker.yml, since there's nothing specific to Docker here: we could just as well use this information to build a virtual machine in which to perform cross builds. I would create a new fact file, eg. cross-build.yml, for this. To be completely honest I'm not 100% sold on the structure and placement of the data, but I can't quite put my fingers on why that's the case - which of course makes it very difficult to come up with an alternative suggestion ;) So let's use this structure for now, we can always change it later if needed.
+ blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses
I cannot really understand what you're trying to say with this comment, can you try to reword it please?
+ - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev
For these and all other blacklisted packages, you should list the name of the mapping (eg. netcf) rather than the concrete Debian package name (eg. libnetcf-dev). Then of course you'll have to act on the blacklist earlier, before mapping is performed.
+ arches: + arm64: + gcc-pkg-prefix: crossbuild-essential-arm64
This is not a prefix, and the package is not GCC :) I would use 'cross_gcc', which incidentally is also the name of the variable you use in the Python script to store this value. (More on this later, though.)
+ target-prefix: aarch64-linux-gnu
Same here, you later use 'cross_prefix' in the script but also store it as 'TARGET' in the environment... I'd suggest 'cross_target'. [...]
+ mipsel: + gcc-pkg-prefix: gcc-mipsel-linux-gnu
crossbuild-essential-mipsel is available in Debian 9, so you should use that instead. [...]
+++ b/guests/host_vars/libvirt-debian-sid/docker.yml @@ -1,2 +1,64 @@ --- docker_base: debian:sid +cross_build: + blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses + - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev + # appears to be dropped from the 'sid' tree + - sheepdog
So it has! And it's apparently not going to be in the next Ubuntu release, either. This has nothing to do with cross compilation, though: you should just update the regular mappings file, in a separate commit, based on this information. [...]
+ mips64el: + gcc-pkg-prefix: gcc-mips64el-linux-gnuabi64
Debian Sid has crossbuild-essential-* packages available for all cross compilation targets, so please use those everywhere. Another idea would be to drop these from the facts entirely and have a bunch of mappings like cross-gcc-mipsel: Debian9: gcc-mipsel-linux-gnu DebianSid: crossbuild-essential-mipsel instead; then you could programmatically add cross-gcc-$arch to the package list in the script before mapping is performed. That sounds to me like it would be much nicer. [...]
+++ b/guests/lcitool @@ -343,6 +343,11 @@ class Application: metavar="GIT_REVISION", help="git revision to build (remote/branch)", ) + self._parser.add_argument( + "-x", + metavar="CROSS-ARCH", + help="cross compiler architecture (dockerfile action only)", + )
s/CROSS-ARCH/CROSS_ARCH/ [...]
@@ -501,10 +506,24 @@ class Application: os_name = facts["os_name"] os_version = facts["os_version"] os_full = os_name + os_version + blacklist = [] + cross_build_facts = None
if package_format not in ["deb", "rpm"]: raise Error("Host {} doesn't support Dockerfiles".format(host))
+ if cross_arch is not None: + if package_format != "deb": + raise Error("cross compilers only supported for debian packages")
This first check is kinda pointless, since the one right after it would be triggered when trying to generate a Dockerfile for cross compilation and the required information has not been provided by maintainers.
+ if "cross_build" not in facts: + raise Error("cross compilers not supported for this host")
raise Error("Host {} doesn't support cross compilation".format(host)) [...]
+ if cross_arch and pkgname[-4:] == "-dev": + if pkgname not in cross_pkgs: + cross_pkgs.append(pkgname + ":" + cross_arch)
I can see how sticking with the Debian-style architecture names makes this bit trivial, but I'm still entirely convinced we should use the same architecture names as libvirt does. Especially once we'll have integrated this into libvirt's own build system, the difference in names would introduce needless friction, as the target audience (libvirt developers) is certainly more familiar with the architecture names used in that project than Debian's. We can avoid that easily enough, by performing the necessary conversion in the script with a small dedicated utility function. [...]
+ if cross_arch: + # Intentionally a separate RUN command from the above + # so that the common packages of all cross-built images + # share a docker image layer
Oh, clever! :)
+ sys.stdout.write(textwrap.dedent(""" + RUN DEBIAN_FRONTEND=noninteractive && \\ + ( \\ + dpkg --add-architecture %(cross_arch)s && \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install --no-install-recommends -y %(cross_gcc)s %(cross_pkgs)s && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y \\ + )
The same considerations about formatting the list of packages as in patch 7/9 also apply here.
+ ENV TARGET "%(cross_prefix)s" + ENV CONFIGURE_OPTS "--host=%(cross_prefix)s --target=%(cross_prefix)s" + ENV PKG_CONFIG_LIBDIR "/usr/lib/%(cross_prefix)s/pkgconfig"
As you mention in a previous commit message, each ENV statement will create and additional layer, and it's considered best practice to have as few of those as possible. Another reason why I don't like this is that it's different from what we already do for the "cross compilation" target we already support, MinGW: in that case, we have a single image with pristine environment, and we inject the additional variables only when actually running the build. In my opinion, that's a saner way to tweak the build, since the interaction is very explicit and doesn't require any knowledge of other components that might even be hosted, as is the case here, in a separate git repository! Additionally, and I only thought about this now :) did we consider the possibility of providing a single Debian image with all cross compilation packages installed, rather than one image per target architecture? Assuming that's feasible, it would also fit very nicely with how we have a single Fedora image that we use for both MinGW variants... I apologize for the length of this message. If it's any consolation, it clearly shows that your patches were good food for thought :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Feb 07, 2019 at 04:21:34PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
With the Debian 9 distro, this supports arm64, armel, armhf, mips, mipsel, mips64el, ppc64el, s390x, which are all the official archs that Debian maintainers currently build libvirt for. With Debian Sid, this is extended to include i386.
Is i386 really not supported as a cross compilation target on Debian 9? It seems like it would be such a low hanging fruit...
Yes, the i386 gcc simply doesn't exist in 9. No idea why
[...]
+++ b/guests/host_vars/libvirt-debian-9/docker.yml @@ -1,2 +1,59 @@ --- docker_base: debian:9 +cross_build:
I don't think this belongs in docker.yml, since there's nothing specific to Docker here: we could just as well use this information to build a virtual machine in which to perform cross builds.
I would create a new fact file, eg. cross-build.yml, for this.
Ok. In practice this split doesn't seem to actually do anything since we load all files & take the union of their contents.
+ blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses
I cannot really understand what you're trying to say with this comment, can you try to reword it please?
dpkg fails to resolve the deps. It wants augeas-lenses:s390x when it should be satisfied with augeas-lenses:$NATIVE. You can't install both augeas-lenses as they conflict on files.
+ - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev
For these and all other blacklisted packages, you should list the name of the mapping (eg. netcf) rather than the concrete Debian package name (eg. libnetcf-dev). Then of course you'll have to act on the blacklist earlier, before mapping is performed.
I don't find that desirable. This is a debian specific config file, and it is clearer to list the actual debian packages we have trouble with, as that's what apt-get is complaining about when it fails. Using the generic mapping obscures what is going on.
+ arches: + arm64: + gcc-pkg-prefix: crossbuild-essential-arm64
This is not a prefix, and the package is not GCC :)
I would use 'cross_gcc', which incidentally is also the name of the variable you use in the Python script to store this value. (More on this later, though.)
Actually this entire variable can be eliminated, as we can derive it from the target name.
+ target-prefix: aarch64-linux-gnu
Same here, you later use 'cross_prefix' in the script but also store it as 'TARGET' in the environment... I'd suggest 'cross_target'.
Using "cross_" is redundant as this already inside a parent "cross-build" block.
[...]
+ mipsel: + gcc-pkg-prefix: gcc-mipsel-linux-gnu
crossbuild-essential-mipsel is available in Debian 9, so you should use that instead.
Actually, I've eliminated all usage of the crossbuild-essential-* packages. These needlessly pull in the C++ compiler too.
docker_base: debian:sid +cross_build: + blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses + - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev + # appears to be dropped from the 'sid' tree + - sheepdog
So it has! And it's apparently not going to be in the next Ubuntu release, either.
What's missing that we've not seen this problem already ? Is it just because we don't regularlyl rebuild the CI hosts / images ?
+ if cross_arch and pkgname[-4:] == "-dev": + if pkgname not in cross_pkgs: + cross_pkgs.append(pkgname + ":" + cross_arch)
I can see how sticking with the Debian-style architecture names makes this bit trivial, but I'm still entirely convinced we should use the same architecture names as libvirt does.
Especially once we'll have integrated this into libvirt's own build system, the difference in names would introduce needless friction, as the target audience (libvirt developers) is certainly more familiar with the architecture names used in that project than Debian's.
Using the libvirt arch names adds no value. There's no friction as there is no context in which we're using the libvirt arch names & need a mapping to the cross build names. These image configs are distro specific and using the distro's native arch is directly relevant to the build results.
+ ENV TARGET "%(cross_prefix)s" + ENV CONFIGURE_OPTS "--host=%(cross_prefix)s --target=%(cross_prefix)s" + ENV PKG_CONFIG_LIBDIR "/usr/lib/%(cross_prefix)s/pkgconfig"
As you mention in a previous commit message, each ENV statement will create and additional layer, and it's considered best practice to have as few of those as possible.
The difference is the usage context. The ENV statement is intended to provide environment variables to runtime users of the image. The previous ENV usage was merely used internally to the docker build and was polluting the runtime env where it was not required. These ENV statements are intended for direct usage in the docker images at runtime, so you can launch the image with docker and simply type "$TARGET-gcc" or "./configure $CONFIGURE_OPTS" and have it do the right thing.
Another reason why I don't like this is that it's different from what we already do for the "cross compilation" target we already support, MinGW: in that case, we have a single image with pristine environment, and we inject the additional variables only when actually running the build. In my opinion, that's a saner way to tweak the build, since the interaction is very explicit and doesn't require any knowledge of other components that might even be hosted, as is the case here, in a separate git repository!
The mingw images are slightly different in that the Fedora mingw RPMs have invented a "mingw32-configure" program that sets all these env variables already. If that program didn't exist, then it would be desirable to have the same env vars set for the mingw images too.
Additionally, and I only thought about this now :) did we consider the possibility of providing a single Debian image with all cross compilation packages installed, rather than one image per target architecture? Assuming that's feasible, it would also fit very nicely with how we have a single Fedora image that we use for both MinGW variants...
Creating a single image would mean a significantly larger download image size when you only care about testing a single architecture. Because of the way image layer sharing works, having lots of separate images ensures you can have a minimal download size for a single image, while not giving duplication if you need to download all images. We ought to have a separate Fedora image for each MinGW target for this same reason in fact. 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 :|
On Wed, 2019-02-13 at 18:52 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 07, 2019 at 04:21:34PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-05 at 17:53 +0000, Daniel P. Berrangé wrote: [...]
With the Debian 9 distro, this supports arm64, armel, armhf, mips, mipsel, mips64el, ppc64el, s390x, which are all the official archs that Debian maintainers currently build libvirt for. With Debian Sid, this is extended to include i386.
Is i386 really not supported as a cross compilation target on Debian 9? It seems like it would be such a low hanging fruit...
Yes, the i386 gcc simply doesn't exist in 9. No idea why
Pretty weird indeed. But that kinda leads into the question: why having Debian 9 support at all? We're only going to have a single set of buildenv-cross-* images, one per architecture, and those are going to be based on Debian Sid, no? Just like we use Fedora Rawhide for MinGW builds. So adding Debian 9 into the mix seems unnecessary.
[...]
+++ b/guests/host_vars/libvirt-debian-9/docker.yml @@ -1,2 +1,59 @@ --- docker_base: debian:9 +cross_build:
I don't think this belongs in docker.yml, since there's nothing specific to Docker here: we could just as well use this information to build a virtual machine in which to perform cross builds.
I would create a new fact file, eg. cross-build.yml, for this.
Ok. In practice this split doesn't seem to actually do anything since we load all files & take the union of their contents.
It's just to keep things organized :)
+ blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses
I cannot really understand what you're trying to say with this comment, can you try to reword it please?
dpkg fails to resolve the deps. It wants augeas-lenses:s390x when it should be satisfied with augeas-lenses:$NATIVE. You can't install both augeas-lenses as they conflict on files.
Got it.
+ - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev
For these and all other blacklisted packages, you should list the name of the mapping (eg. netcf) rather than the concrete Debian package name (eg. libnetcf-dev). Then of course you'll have to act on the blacklist earlier, before mapping is performed.
I don't find that desirable. This is a debian specific config file, and it is clearer to list the actual debian packages we have trouble with, as that's what apt-get is complaining about when it fails. Using the generic mapping obscures what is going on.
That's true every time package names are involved, and everywhere else we use the abstract names rather than the concrete ones. We should use them here as well, not only to be consistent but also so that we only ever have to update a single file when the concrete package names change.
+ arches: + arm64: + gcc-pkg-prefix: crossbuild-essential-arm64
This is not a prefix, and the package is not GCC :)
I would use 'cross_gcc', which incidentally is also the name of the variable you use in the Python script to store this value. (More on this later, though.)
Actually this entire variable can be eliminated, as we can derive it from the target name.
Alright, I had suggested doing that with the arch name and crossbuild-essential-*, but using the target and gcc-* works just as nicely :)
+ target-prefix: aarch64-linux-gnu
Same here, you later use 'cross_prefix' in the script but also store it as 'TARGET' in the environment... I'd suggest 'cross_target'.
Using "cross_" is redundant as this already inside a parent "cross-build" block.
Alright.
[...]
+ mipsel: + gcc-pkg-prefix: gcc-mipsel-linux-gnu
crossbuild-essential-mipsel is available in Debian 9, so you should use that instead.
Actually, I've eliminated all usage of the crossbuild-essential-* packages. These needlessly pull in the C++ compiler too.
Okay.
docker_base: debian:sid +cross_build: + blacklist: + # Doesn't properly resolve from foreign arch package + # to the native package of augeas-lenses + - libnetcf-dev + # Fails to resolve deps from foreign arch + - wireshark-dev + # dtrace tool doesn't know how to cross-compile + - systemtap-sdt-dev + # appears to be dropped from the 'sid' tree + - sheepdog
So it has! And it's apparently not going to be in the next Ubuntu release, either.
What's missing that we've not seen this problem already ? Is it just because we don't regularlyl rebuild the CI hosts / images ?
Yeah, we hadn't triggered a build for Docker images in a while, and as for the virtual machine running builds on ci.centos.org we don't rebuild those unless there's some issue, so it's happily hanging on to the local copy of sheepdog it installed back when it was still part of Debian Sid.
+ if cross_arch and pkgname[-4:] == "-dev": + if pkgname not in cross_pkgs: + cross_pkgs.append(pkgname + ":" + cross_arch)
I can see how sticking with the Debian-style architecture names makes this bit trivial, but I'm still entirely convinced we should use the same architecture names as libvirt does. Especially once we'll have integrated this into libvirt's own build system, the difference in names would introduce needless friction, as the target audience (libvirt developers) is certainly more familiar with the architecture names used in that project than Debian's.
Using the libvirt arch names adds no value. There's no friction as there is no context in which we're using the libvirt arch names & need a mapping to the cross build names. These image configs are distro specific and using the distro's native arch is directly relevant to the build results.
The context is libvirt developers running a cross build through the build system integration, eg. using your proposed implementation you'd need to use $ make cibuild-cross-arm64 instead of the much more natural (to libvirt developers) $ make cibuild-cross-aarch64 Now, having to mentally map 'aarch64' to 'arm64' might not be a lot of friction, but it's still some friction, and it's completely unnecessary too because we can perform the architecture mapping very easily in lcitool and make it completely transparent to all other tools and users.
+ ENV TARGET "%(cross_prefix)s" + ENV CONFIGURE_OPTS "--host=%(cross_prefix)s --target=%(cross_prefix)s" + ENV PKG_CONFIG_LIBDIR "/usr/lib/%(cross_prefix)s/pkgconfig"
As you mention in a previous commit message, each ENV statement will create and additional layer, and it's considered best practice to have as few of those as possible.
The difference is the usage context. The ENV statement is intended to provide environment variables to runtime users of the image. The previous ENV usage was merely used internally to the docker build and was polluting the runtime env where it was not required.
Okay, point taken.
These ENV statements are intended for direct usage in the docker images at runtime, so you can launch the image with docker and simply type "$TARGET-gcc" or "./configure $CONFIGURE_OPTS" and have it do the right thing.
I guess what I don't like is using something that's Docker-specific to inject this kind of information into the guest OS, because in my mind you should end up with a mostly identical environment whether you're building in a virtual machines created using lcitool or inside a Docker container. The way we get this kind of information into virtual machines is through a combination of static, job-agnostic variables set in the user's .bashrc and job-specific variables passed by whatever process is running the build. Looking at the variables above, TARGET belongs to the former group and CONFIGURE_OPTS to the latter, with PKG_CONFIG_LIBDIR sitting uncomfortably somewhere in between. Now, we're not adding a custom .bashrc at all when using Docker, so I guess using ENV directives instead is fine. But poisoning the user environment by setting PKG_CONFIG_LIBDIR globally might not be, and having a magic CONFIGURE_OPTS that build commands are somehow just supposed to know exists is very much not cool. What I'd like to see instead is something like docker \ -e PKG_CONFIG_LIBDIR='/usr/lib/$TARGET/pkgconfig' \ -e autogen_args='--host=$TARGET --target=$TARGET' \ ... /bin/sh -xc './autogen.sh $autogen_args' but I just realized that such a thing wouldn't really work because the variables won't be expanded inside the container, and so you'd just get the literal string '$TARGET' which is of course no good :( Alright, ENV directives it is I guess :(
Another reason why I don't like this is that it's different from what we already do for the "cross compilation" target we already support, MinGW: in that case, we have a single image with pristine environment, and we inject the additional variables only when actually running the build. In my opinion, that's a saner way to tweak the build, since the interaction is very explicit and doesn't require any knowledge of other components that might even be hosted, as is the case here, in a separate git repository!
The mingw images are slightly different in that the Fedora mingw RPMs have invented a "mingw32-configure" program that sets all these env variables already. If that program didn't exist, then it would be desirable to have the same env vars set for the mingw images too.
Yeah, that's kind of a shortcut that we took in our travis.yml compared to what we do on ci.centos.org or local builds through lcitool, where we call good 'ol configure with appropriate arguments after adding all the necessary variables into the environment. Personally I don't much like the discrepancy, as I believe we should perform all builds the same way regardless of the environment to guarantee consistency, but it is way shorter and more convenient so I guess we're keeping it :) Especially since the issue mentioned above makes it hard to do anything else :(
Additionally, and I only thought about this now :) did we consider the possibility of providing a single Debian image with all cross compilation packages installed, rather than one image per target architecture? Assuming that's feasible, it would also fit very nicely with how we have a single Fedora image that we use for both MinGW variants...
Creating a single image would mean a significantly larger download image size when you only care about testing a single architecture. Because of the way image layer sharing works, having lots of separate images ensures you can have a minimal download size for a single image, while not giving duplication if you need to download all images.
Okay, this makes sense.
We ought to have a separate Fedora image for each MinGW target for this same reason in fact.
First step would be to rip the MinGW stuff out of the regular Fedora Rawhide configuration; after that we can definitely move to separate images. -- Andrea Bolognani / Red Hat / Virtualization
Add a simple makefile that can generate dockerfiles and run local builds of them. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- .gitignore | 1 + dockerfiles/Makefile | 33 +++++++++++++++++++++++++++++++++ guests/lcitool | 15 +++++++++++++-- 3 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 dockerfiles/Makefile diff --git a/.gitignore b/.gitignore index b25c15b..c92cb0e 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,2 @@ *~ +dockerfiles/*.docker diff --git a/dockerfiles/Makefile b/dockerfiles/Makefile new file mode 100644 index 0000000..0075840 --- /dev/null +++ b/dockerfiles/Makefile @@ -0,0 +1,33 @@ + +LCITOOL = ../guests/lcitool + +HOSTS = $(shell $(LCITOOL) -a hosts) + +CROSS_HOST = libvirt-debian-9 + +ARCHES = $(shell $(LCITOOL) -a arches -h $(CROSS_HOST)) + +CONFIGS = $(wildcard ../guests/host_vars/*/docker.yml) + +DOCKERFILES = $(shell $(LCITOOL) -a dockerfiles) + +DOCKERIMAGES = $(DOCKERFILES:%.docker=%) + +all: $(DOCKERFILES) + +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 + +buildenv-%: buildenv-%.docker + docker build --tag $@ -f $< . + +clean: + rm -f *.docker *~ diff --git a/guests/lcitool b/guests/lcitool index dc5741e..a5ae817 100755 --- a/guests/lcitool +++ 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 uncommon actions: dockerfile generate Dockerfile (doesn't access the host) @@ -415,6 +416,16 @@ class Application: for project in self._projects.expand_pattern("all"): print(project) + 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) + for arch in facts.get("cross_build", {}).get("arches", {}).keys(): + print ("buildenv-%s-cross-%s.docker" % (host, arch)) + def _action_install(self, hosts, _projects, _revision, _cross_arch): base = Util.get_base() -- 2.20.1
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
On Tue, 2019-02-05 at 17:52 +0000, Daniel P. Berrangé wrote:
Changed in v2:
- Fix multiple package name mistakes - Modify lcitool to generate cross-arch docker files - Add --no-install-recommended flag to apt-get - Add DEBIAN_FRONTEND=noninteractive env to apt-get - Improve error reporting in lcitool - Add make rule for generating dockerfiles locally
Daniel P. Berrangé (9): guests: use libpcap0.8-dev package on Debian guests: add xfsprogs development package for libvirt guests: fix glusterfs package name on Debian
Would you mind fixing the small issues in these first three patches and pushing them without waiting for the rest of the changes? I realized the images on quay.io are fairly outdated, so I'd like to refresh all Dockerfiles and trigger a rebuild, but it'd be nice if that included your fixes as well. Oh, and I guess without the sheepdog changes from 8/9 the Debian Sid build will certainly fail, so can you post that as a separate patch and I'll ACK it real quick? :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Pavel Hrdina