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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
