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