[libvirt] [jenkins-ci PATCH v5 5/5] lcitool: support generating cross compiler dockerfiles
Andrea Bolognani
abologna at redhat.com
Tue Feb 26 18:04:16 UTC 2019
On Tue, 2019-02-26 at 11:00 +0000, Daniel P. Berrangé wrote:
> Debian's filesystem layout has a nice advantage over Fedora which is
> that it can install non-native RPMs
s/RPMs/packages/ ;)
[...]
> guests/host_vars/libvirt-debian-9/main.yml | 44 +++++++++++++
> guests/host_vars/libvirt-debian-sid/main.yml | 45 ++++++++++++++
> guests/lcitool | 65 +++++++++++++++++++-
> guests/vars/mappings.yml | 59 ++++++++++++++++++
> 4 files changed, 211 insertions(+), 2 deletions(-)
It would be great if the changes to code and inventory were in
separate commits. It should be trivial to split: just add the new
information to the inventory first, and code relying on them being
present last.
[...]
> +++ b/guests/host_vars/libvirt-debian-9/main.yml
> @@ -21,3 +21,47 @@ os_name: 'Debian'
> os_version: '9'
>
> ansible_python_interpreter: /usr/bin/python3
> +
> +arches:
> + aarch64:
> + package_arch: arm64
> + abi: aarch64-linux-gnu
> + cross_gcc: gcc-aarch64-linux-gnu
We previously agreed to store this information in cross-build.yml
rather than main.yml, and that's what it looked like in v3... Can
you please move it back there?
I also questioned a few respins back whether we need to have the
Debian 9 configuration at all. For MinGW builds we use Fedora Rawhide
as the base, so using Debian Sid for cross-builds would make sense,
especially since it supports more target architectures: we are only
going to build a single set of buildenv-cross-* container images
anyway...
[...]
> +++ b/guests/host_vars/libvirt-debian-sid/main.yml
> @@ -21,3 +21,48 @@ os_name: 'Debian'
> os_version: 'Sid'
>
> ansible_python_interpreter: /usr/bin/python3
> +
> +arches:
> + aarch64:
> + package_arch: arm64
> + abi: aarch64-linux-gnu
> + cross_gcc: gcc-aarch64-linux-gnu
Pretty much all this information is static: the mapping between
libvirt/kernel architecture name and Debian architecture name is
well defined and can't really change, and the same should apply to
the corresponding ABI names, so I would personally just implement
the mapping in Python, right next to the function we already have
introduced in the previous commit to fetch the native architecture
of the machine lcitool is running on.
As for the cross compiler, we can obtain the package name by
prepending "gcc-" to the ABI name, so once again storing it in the
inventory is unnecessary.
[...]
> + i686:
> + package_arch: i386
> + abi: i686-linux-gnu
> + cross_gcc: gcc-i686-linux-gnu
The value of 'abi' doesn't look right: based on eg.
https://packages.debian.org/sid/i386/libglib2.0-dev/filelist
it should be 'i386-linux-gnu' instead.
The value of 'cross_gcc', though, is unfortunately correct, which
means that in this case we can't automatically figure out the name
of the cross-compiler package from the ABI like I described above.
Bummer :(
That said, storing this much redundant information because of a
single exception seems wasteful. Perhaps lcitool could behave the
following way:
* figure out 'package_arch' and 'abi' based on 'cross_arch' as
specified on the command line, using a static mapping function
implemented in Python;
* if the facts for the host contain arches[arch]["cross_gcc"],
then use that value; otherwise, construct 'cross_gcc' by
prepending "gcc-" to 'abi' - this will take care of the ugly
exception above;
* if we only have Debian Sid to take into account, then we can
simply block attempts to "cross-compile" to the native
architecture, but if such special-casing is not considered
appropriate we can reuse the same logic mentioned above by
having an empty value for arches["x86_64"]["cross_gcc"]. Note
though that the check against the native architecture is
probably what we want, because it's more accurate in general:
gcc-x86-64-linux-gnu might not be available on x86_64, but it
is on ppc64le and aarch64...
[...]
> + mips64:
> + package_arch: mips64
> + abi: mips64-linux-gnu
> + cross_gcc: gcc-mips64-linux-gnu
Debian doesn't have a 'mips64' architecture.
> + mips64el:
> + package_arch: mips64el
> + abi: mips64el-linux-gnu
This should be
abi: mips64el-linux-gnuabi64
> + ppc64el:
> + package_arch: ppc64el
> + abi: ppc64el-linux-gnu
This should be
abi: powerpc64le-linux-gnu
[...]
> +++ b/guests/lcitool
> @@ -367,6 +367,10 @@ class Application:
>
> add_hosts_arg(dockerfileparser)
> add_projects_arg(dockerfileparser)
> + dockerfileparser.add_argument(
> + "-x", "--cross-arch",
> + help="cross compiler architecture",
> + )
You added the add_gitrev_arg() helper for --git-revision despite the
fact that it's only used by a single action, so it would make sense
to have add_crossarch_arg() here for consistency.
[...]
> @@ -537,10 +541,23 @@ class Application:
> os_name = facts["os_name"]
> os_version = facts["os_version"]
> os_full = os_name + os_version
> + cross_facts = None
You initialize 'cross_facts' here...
[...]
> + if args.cross_arch is not None:
> + if "arches" not in facts:
> + raise Error("Non x86_64 arches not supported for this host")
> + if args.cross_arch not in facts["arches"]:
> + raise Error("Arch {} not supported for this host".format(
> + args.cross_arch))
> + if "cross_gcc" not in facts["arches"][args.cross_arch]:
> + raise Error("Arch {} cross compiler not supported for this host".format
> + (args.cross_arch))
> +
> + cross_build_facts = facts["arches"][args.cross_arch]
... but then set 'cross_build_facts' and use the latter from here
on. And this is why dynamic languages are so much fun! O:-)
I'd stick with 'cross_facts' for the variable name, since we already
have 'pkgs'/'cross_pkgs' and 'keys'/'cross_keys'.
[...]
> # We need to add the base project manually here: the standard
> # machinery hides it because it's an implementation detail
> for project in projects + ["base"]:
> for package in self._projects.get_packages(project):
> + policy = "native"
I'd call this variable 'cross_policy' for consistency with the other
variables used in the cross-build context. More comment on the
functionality it's connected to further down.
[...]
> @@ -80,6 +84,7 @@ mappings:
>
> avahi:
> deb: libavahi-client-dev
> + deb-cross-arch: foreign
> pkg: avahi
> rpm: avahi-devel
So, I'll come right out and say that I don't love the idea of storing
this information in the mappings file. That said, doing so allows us
to reuse a lot of existing infrastructure and I don't really have a
better alternative to offer at the moment, so let's just do it this
way :)
We should, however, apply the same changes as for the architecture
information and put the new part first; moreover, I would suggest
using something like 'cross-policy' instead of 'cross-arch', because
the latter is not very descriptive.
One last point is about the possible values: 'skip' and 'native' are
perfectly intuitive, but I feel like 'foreign' is not that great a
name, and it also already has a meaning in the context of Debian's
Multi-Arch implementation which might muddy the waters for people.
I think 'cross' would be a better value.
--
Andrea Bolognani / Red Hat / Virtualization
More information about the libvir-list
mailing list