[libvirt] [jenkins-ci PATCH v5 0/5] Add support for cross compiling libvirt via Debian
Changed in v5: - Remove redundant formatter class - Update commit message - Update readme - Move misplaced patch chunk - Dont set "dest" in arg when not needed - Use metavar for subparser Changed in v4: - Pull in change to use argparse sub-parsers - Refactor way architecture specific package rules are stored to be in the main package mappings data Changed in v3: - Remove sheepdog more generally - Use .format() style printf - Split config to cross-build.yml - Make glusterfs name per-distro customized - Misc code style changes - Rename fields in cross-build.yml - Don't use crossbuild-essential packages 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é (5): lcitool: use subparsers for commands lcitool: avoid repetition when expanding package mappings lcitool: avoid intermediate list of packages mappings: extend mapping to allow per-arch entries lcitool: support generating cross compiler dockerfiles guests/README.markdown | 18 +- guests/host_vars/libvirt-debian-9/main.yml | 44 ++++ guests/host_vars/libvirt-debian-sid/main.yml | 45 ++++ guests/lcitool | 230 +++++++++++++------ guests/playbooks/update/tasks/packages.yml | 32 +++ guests/vars/mappings.yml | 101 +++++++- 6 files changed, 381 insertions(+), 89 deletions(-) -- 2.20.1
Currently only a single global parser is used for all commands. This means that every command accepts every argument which is undesirable as users don't know what to pass. It also prevents the parser from generating useful help information. Python's argparse module supports multi-command binaries in a very easy way by adding subparsers, each of which has their own distinct arguments. It can then generate suitable help text on a per command basis. This also means commands can use positional arguments for data items that are always passed. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/README.markdown | 18 +++--- guests/lcitool | 129 +++++++++++++++++++++++------------------ 2 files changed, 80 insertions(+), 67 deletions(-) diff --git a/guests/README.markdown b/guests/README.markdown index ac9e4f6..6363bc9 100644 --- a/guests/README.markdown +++ b/guests/README.markdown @@ -10,38 +10,38 @@ Usage and examples There are two steps to bringing up a guest: -* `lcitool -a install -h $guest` will perform an unattended installation +* `lcitool install $guest` will perform an unattended installation of `$guest`. Not all guests can be installed this way: see the "FreeBSD" section below; -* `lcitool -a update -h $guest -p $project` will go through all the +* `lcitool update $guest $project` will go through all the post-installation configuration steps required to make the newly-created guest usable and ready to be used for building `$project`; Once those steps have been performed, maintainance will involve running: - lcitool -a update -h $guest -p $project + lcitool update $guest $project periodically to ensure the guest configuration is sane and all installed packages are updated. To get a list of known guests and projects, run - lcitool -a hosts + lcitool hosts and - lcitool -a projects + lcitool projects respectively. You can run operations involving multiple guests and projects at once by providing a list on the command line: for example, running - lcitool -a update -h '*fedora*' -p '*osinfo*' + lcitool update '*fedora*' '*osinfo*' will update all Fedora guests and get them ready to build libosinfo and related projects, while running - lcitool -a update -h all -p 'libvirt,libvirt+mingw*' + lcitool update all 'libvirt,libvirt+mingw*' will update all hosts and prepare them to build libvirt both as a native library and, where supported, as a Windows library using MinGW. @@ -49,7 +49,7 @@ library and, where supported, as a Windows library using MinGW. Once hosts have been prepared following the steps above, you can use `lcitool` to perform builds as well: for example, running - lcitool -a build -h '*debian*' -p libvirt-python + lcitool build '*debian*' libvirt-python will fetch libvirt-python's `master` branch from the upstream repository and build it on all Debian hosts. @@ -58,7 +58,7 @@ You can add more git repositories by tweaking the `git_urls` dictionary defined in `playbooks/build/jobs/defaults.yml` and then build arbitrary branches out of those with - lcitool -a build -h all -p libvirt -g github/cool-feature + lcitool build -g github/cool-feature all libvirt Host setup diff --git a/guests/lcitool b/guests/lcitool index c2dec81..ec467b7 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -304,45 +304,66 @@ class Application: self._parser = argparse.ArgumentParser( conflict_handler="resolve", - formatter_class=argparse.RawDescriptionHelpFormatter, description="libvirt CI guest management tool", - epilog=textwrap.dedent(""" - common actions: - install perform unattended host installation - update prepare hosts and keep them updated - build build projects on hosts + ) - informational actions: - hosts list all known hosts - projects list all known projects + subparsers = self._parser.add_subparsers(metavar="ACTION") + subparsers.required = True - uncommon actions: - dockerfile generate Dockerfile (doesn't access the host) + def add_hosts_arg(parser): + parser.add_argument( + "hosts", + help="list of hosts to act on (accepts globs)", + ) - glob patterns are supported for HOSTS and PROJECTS - """), - ) - self._parser.add_argument( - "-a", - metavar="ACTION", - required=True, - help="action to perform (see below)", - ) - self._parser.add_argument( - "-h", - metavar="HOSTS", - help="list of hosts to act on", - ) - self._parser.add_argument( - "-p", - metavar="PROJECTS", - help="list of projects to consider", - ) - self._parser.add_argument( - "-g", - metavar="GIT_REVISION", - help="git revision to build (remote/branch)", - ) + def add_projects_arg(parser): + parser.add_argument( + "projects", + help="list of projects to consider (accepts globs)", + ) + + def add_gitrev_arg(parser): + parser.add_argument( + "-g", "--git-revision", + help="git revision to build (remote/branch)", + ) + + installparser = subparsers.add_parser( + "install", help="perform unattended host installation") + installparser.set_defaults(func=self._action_install) + + add_hosts_arg(installparser) + + updateparser = subparsers.add_parser( + "update", help="prepare hosts and keep them updated") + updateparser.set_defaults(func=self._action_update) + + add_hosts_arg(updateparser) + add_projects_arg(updateparser) + add_gitrev_arg(updateparser) + + buildparser = subparsers.add_parser( + "build", help="build projects on hosts") + buildparser.set_defaults(func=self._action_build) + + add_hosts_arg(buildparser) + add_projects_arg(buildparser) + add_gitrev_arg(buildparser) + + hostsparser = subparsers.add_parser( + "hosts", help="list all known hosts") + hostsparser.set_defaults(func=self._action_hosts) + + projectsparser = subparsers.add_parser( + "projects", help="list all known projects") + projectsparser.set_defaults(func=self._action_projects) + + dockerfileparser = subparsers.add_parser( + "dockerfile", help="generate Dockerfile (doesn't access the host)") + dockerfileparser.set_defaults(func=self._action_dockerfile) + + add_hosts_arg(dockerfileparser) + add_projects_arg(dockerfileparser) def _execute_playbook(self, playbook, hosts, projects, git_revision): base = Util.get_base() @@ -403,20 +424,20 @@ class Application: raise Error( "Failed to run {} on '{}': {}".format(playbook, hosts, ex)) - def _action_hosts(self, _hosts, _projects, _revision): + def _action_hosts(self, args): for host in self._inventory.expand_pattern("all"): print(host) - def _action_projects(self, _hosts, _projects, _revision): + def _action_projects(self, args): for project in self._projects.expand_pattern("all"): print(project) - def _action_install(self, hosts, _projects, _revision): + def _action_install(self, args): base = Util.get_base() flavor = self._config.get_flavor() - for host in self._inventory.expand_pattern(hosts): + for host in self._inventory.expand_pattern(args.hosts): facts = self._inventory.get_facts(host) # Both memory size and disk size are stored as GiB in the @@ -483,16 +504,18 @@ class Application: except Exception as ex: raise Error("Failed to install '{}': {}".format(host, ex)) - def _action_update(self, hosts, projects, git_revision): - self._execute_playbook("update", hosts, projects, git_revision) + def _action_update(self, args): + self._execute_playbook("update", args.hosts, args.projects, + args.git_revision) - def _action_build(self, hosts, projects, git_revision): - self._execute_playbook("build", hosts, projects, git_revision) + def _action_build(self, args): + self._execute_playbook("build", args.hosts, args.projects, + args.git_revision) - def _action_dockerfile(self, hosts, projects, _revision): + def _action_dockerfile(self, args): mappings = self._projects.get_mappings() - hosts = self._inventory.expand_pattern(hosts) + hosts = self._inventory.expand_pattern(args.hosts) if len(hosts) > 1: raise Error("Can't generate Dockerfile for multiple hosts") host = hosts[0] @@ -506,7 +529,7 @@ class Application: if package_format not in ["deb", "rpm"]: raise Error("Host {} doesn't support Dockerfiles".format(host)) - projects = self._projects.expand_pattern(projects) + projects = self._projects.expand_pattern(args.projects) for project in projects: if project not in facts["projects"]: raise Error( @@ -572,18 +595,8 @@ class Application: """).format(**varmap)) def run(self): - cmdline = self._parser.parse_args() - action = cmdline.a - hosts = cmdline.h - projects = cmdline.p - git_revision = cmdline.g - - method = "_action_{}".format(action.replace("-", "_")) - - if hasattr(self, method): - getattr(self, method).__call__(hosts, projects, git_revision) - else: - raise Error("Invalid action '{}'".format(action)) + args = self._parser.parse_args() + args.func(args) if __name__ == "__main__": -- 2.20.1
On Tue, 2019-02-26 at 11:00 +0000, Daniel P. Berrangé wrote:
+++ b/guests/README.markdown [...] @@ -58,7 +58,7 @@ You can add more git repositories by tweaking the `git_urls` dictionary defined in `playbooks/build/jobs/defaults.yml` and then build arbitrary branches out of those with
- lcitool -a build -h all -p libvirt -g github/cool-feature + lcitool build -g github/cool-feature all libvirt
Host setup
You forgot to update the line 0 0 * * * ~/libvirt-jenkins-ci/guests/lcitool -a update -h all -p all in this section of the document. With that fixed, Reviewed-by: Andrea Bolognani <abologna@redhat.com> I'm not sure why you decided to post v5 before I got around to review the remaining two patches, but since you have R-b's for these first three patches now and they make sense even on their own, feel free to push them without waiting any further. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 26, 2019 at 12:25:12PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-26 at 11:00 +0000, Daniel P. Berrangé wrote:
+++ b/guests/README.markdown [...] @@ -58,7 +58,7 @@ You can add more git repositories by tweaking the `git_urls` dictionary defined in `playbooks/build/jobs/defaults.yml` and then build arbitrary branches out of those with
- lcitool -a build -h all -p libvirt -g github/cool-feature + lcitool build -g github/cool-feature all libvirt
Host setup
You forgot to update the line
0 0 * * * ~/libvirt-jenkins-ci/guests/lcitool -a update -h all -p all
in this section of the document. With that fixed,
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
I'm not sure why you decided to post v5 before I got around to review the remaining two patches, but since you have R-b's for these first three patches now and they make sense even on their own, feel free to push them without waiting any further.
I didn't see any point in delaying these patches since they are useful regardless of the cross-arch stuff, so it was easy to just repost with those updates since you hadn't done the other reviews yesterday. 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 Tue, 2019-02-26 at 11:29 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 26, 2019 at 12:25:12PM +0100, Andrea Bolognani wrote:
I'm not sure why you decided to post v5 before I got around to review the remaining two patches, but since you have R-b's for these first three patches now and they make sense even on their own, feel free to push them without waiting any further.
I didn't see any point in delaying these patches since they are useful regardless of the cross-arch stuff, so it was easy to just repost with those updates since you hadn't done the other reviews yesterday.
Fair enough - and it clearly worked :) -- Andrea Bolognani / Red Hat / Virtualization
Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index ec467b7..46af92b 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -541,18 +541,14 @@ class Application: temp = {} + keys = ["default", package_format, os_name, os_full] # 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): - if "default" in mappings[package]: - temp[package] = mappings[package]["default"] - if package_format in mappings[package]: - temp[package] = mappings[package][package_format] - if os_name in mappings[package]: - temp[package] = mappings[package][os_name] - if os_full in mappings[package]: - temp[package] = mappings[package][os_full] + for key in keys: + if key in mappings[package]: + temp[package] = mappings[package][key] pkgs = [] for item in temp: -- 2.20.1
We can purge any packages which expand to None straight away, and simply convert to a set to get rid of duplicates. Reviewed-by: Andrea Bolognani <abologna@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 46af92b..88bc945 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -539,8 +539,7 @@ class Application: ) ) - temp = {} - + pkgs = {} keys = ["default", package_format, os_name, os_full] # We need to add the base project manually here: the standard # machinery hides it because it's an implementation detail @@ -548,21 +547,17 @@ class Application: for package in self._projects.get_packages(project): for key in keys: if key in mappings[package]: - temp[package] = mappings[package][key] + pkgs[package] = mappings[package][key] - pkgs = [] - for item in temp: - pkgname = temp[item] - if pkgname is None: - continue - if pkgname in pkgs: - continue - pkgs.append(pkgname) + if package not in pkgs: + continue + if pkgs[package] is None: + del pkgs[package] print("FROM {}".format(facts["docker_base"])) varmap = {} - varmap["pkgs"] = " \\\n ".join(sorted(pkgs)) + varmap["pkgs"] = " \\\n ".join(sorted(set(pkgs.values()))) if package_format == "deb": sys.stdout.write(textwrap.dedent(""" RUN export DEBIAN_FRONTEND=noninteractive && \\ -- 2.20.1
For example to prevent Xen being installed on any s390x xen: default-s390x: deb: libxen-dev Fedora: xen-devel Or the inverse to only install Xen on x86_64 on Debian, but allow it on all archs on Fedora xen: deb-x86_64: libxen-dev Fedora: xen-devel Note that the architecture specific matches are considered after all the non-architcture matches. The mappings are updated to blacklist libnuma on arm6/7 for Debian since it is not built for those archs. xen is whitelisted to only be used on x86_64, arm7 and aarch64 for Debian, since the majority of other architectures don't support it. The dockerfile generator is updated to apply arch filtering based on the host arch. The ansible playbook is not actually implementing support for non-x86_64 architectures in this commit. It is just hardcoding x86_64, which is enough to ensure the changes in the mappings.yml file are a no-op initially. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- guests/lcitool | 15 +++++++- guests/playbooks/update/tasks/packages.yml | 32 +++++++++++++++++ guests/vars/mappings.yml | 42 +++++++++++++++++++--- 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/guests/lcitool b/guests/lcitool index 88bc945..263ab0d 100755 --- a/guests/lcitool +++ b/guests/lcitool @@ -20,6 +20,7 @@ import argparse import fnmatch import json import os +import platform import random import string import subprocess @@ -302,6 +303,8 @@ class Application: self._inventory = Inventory() self._projects = Projects() + self._native_arch = self._libvirt_host_arch() + self._parser = argparse.ArgumentParser( conflict_handler="resolve", description="libvirt CI guest management tool", @@ -424,6 +427,15 @@ class Application: raise Error( "Failed to run {} on '{}': {}".format(playbook, hosts, ex)) + def _libvirt_host_arch(self): + # Same canonicalization as libvirt virArchFromHost + arch = platform.machine() + if arch in ["i386", "i486", "i586"]: + arch = "i686" + if arch == "amd64": + arch = "x86_64" + return arch + def _action_hosts(self, args): for host in self._inventory.expand_pattern("all"): print(host) @@ -540,7 +552,8 @@ class Application: ) pkgs = {} - keys = ["default", package_format, os_name, os_full] + base_keys = ["default", package_format, os_name, os_full] + keys = base_keys + [k + "-" + self._native_arch for k in base_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"]: diff --git a/guests/playbooks/update/tasks/packages.yml b/guests/playbooks/update/tasks/packages.yml index 7fdfc45..b3b8a27 100644 --- a/guests/playbooks/update/tasks/packages.yml +++ b/guests/playbooks/update/tasks/packages.yml @@ -52,6 +52,38 @@ when: - mappings[item][os_name + os_version] is defined +- name: '{{ project }}: Look up mappings (default with arch)' + set_fact: + temp: '{{ temp|combine({ item: mappings[item]["default" + "-" + "x86_64"] }) }}' + with_items: + '{{ packages }}' + when: + - mappings[item]["default" + "-" + "x86_64"] is defined + +- name: '{{ project }}: Look up mappings (package format with arch)' + set_fact: + temp: '{{ temp|combine({ item: mappings[item][package_format + "-" + "x86_64"] }) }}' + with_items: + '{{ packages }}' + when: + - mappings[item][package_format + "-" + "x86_64"] is defined + +- name: '{{ project }}: Look up mappings (OS name with arch)' + set_fact: + temp: '{{ temp|combine({ item: mappings[item][os_name + "-" + "x86_64"] }) }}' + with_items: + '{{ packages }}' + when: + - mappings[item][os_name + "-" + "x86_64"] is defined + +- name: '{{ project }}: Look up mappings (OS version with arch)' + set_fact: + temp: '{{ temp|combine({ item: mappings[item][os_name + os_version + "-" + "x86_64"] }) }}' + with_items: + '{{ packages }}' + when: + - mappings[item][os_name + os_version + "-" + "x86_64"] is defined + - set_fact: flattened: [] diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index 8ff2f34..ea46ce4 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -7,9 +7,25 @@ # priority: # # - default -# - package format (deb, pkg, rpm) -# - OS name (CentOS, Debian, Fedora, FreeBSD, Ubuntu) -# - OS version (CentOS7, Debian9, FedoraRawhide, Ubuntu18 and so on) +# - package format +# - OS name +# - OS version +# - default with arch +# - package format with arch +# - OS name with arch +# - OS version with arch +# +# Valid package formats are +# - deb, pkg, rpm +# +# Valid OS names are: +# - CentOS, Debian, Fedora, FreeBSD, Ubuntu +# +# Valid OS versions are: +# - CentOS7, Debian9, FedoraRawhide, Ubuntu18 and so on +# +# The 'with arch' levels take a suffix "-$ARCH" where $ARCH +# is a libvirt arch name. # # So something like # @@ -27,6 +43,20 @@ # # will result in the 'ccache' package being installed everywhere except # for CentOS, where nothing will be installed. +# +# For example to prevent Xen being installed on any s390x +# +# xen: +# default-s390x: +# deb: libxen-dev +# Fedora: xen-devel +# +# Or the inverse to only install Xen on x86_64 +# +# xen: +# deb-x86_64: libxen-dev +# Fedora-x86_64: xen-devel +# mappings: @@ -278,6 +308,8 @@ mappings: libnuma: deb: libnuma-dev + deb-armv6l: + deb-armv7l: rpm: numactl-devel libparted: @@ -821,7 +853,9 @@ mappings: Debian8: xen: - deb: libxen-dev + deb-x86_64: libxen-dev + deb-armv7l: libxen-dev + deb-aarch64: libxen-dev Fedora: xen-devel xfsprogs: -- 2.20.1
On Tue, 2019-02-26 at 11:00 +0000, Daniel P. Berrangé wrote:
For example to prevent Xen being installed on any s390x
xen: default-s390x: deb: libxen-dev Fedora: xen-devel
Or the inverse to only install Xen on x86_64 on Debian, but allow it on all archs on Fedora
xen: deb-x86_64: libxen-dev Fedora: xen-devel
Note that the architecture specific matches are considered after all the non-architcture matches.
I feel like the order entries are processed with this implementation can be quite counter-intuitive. The root problem of course is that up until this point we have had a single axis to work on, and we're now introducing a second one which is independent of the existing one but which we still have to fit along with it into the same flat hierarchy somehow... Consider an example such as foo: Fedora: oldfoo Fedora-s390x: FedoraRawhide: foo The general intuition up until now has been that appending something to a label would make it more specific, and only one item on each level would possibly match, but now both Fedora-s390x and FedoraRawhide appear to be on the same level and *both* could apply to an s390x Fedora Rawhide machine, making it less clear which one would take precedence. I suggest that the example above would be rewritten as foo: Fedora: oldfoo FedoraRawhide: foo s390x-Fedora: where two changes happened: first of all, the lines have been shuffled around to match the order they would actually be processed, which is something that we could have done with the above as well, but more importantly the architecture name is now *prepended* to the existing labels to clearly signal that it's part of a completely separate hierarchy. The ambiguity is now gone entirely, and which entry has precedence is easier to see at a glance. Implementing this alternative proposal requires only very minor tweaks to the code you posted. [...]
@@ -424,6 +427,15 @@ class Application: raise Error( "Failed to run {} on '{}': {}".format(playbook, hosts, ex))
+ def _libvirt_host_arch(self): + # Same canonicalization as libvirt virArchFromHost + arch = platform.machine() + if arch in ["i386", "i486", "i586"]: + arch = "i686" + if arch == "amd64": + arch = "x86_64" + return arch
This should be a @staticmethod in the Utils class. I'd also rename it to get_native_arch(). [...]
@@ -278,6 +308,8 @@ mappings:
libnuma: deb: libnuma-dev + deb-armv6l: + deb-armv7l: rpm: numactl-devel
libparted: @@ -821,7 +853,9 @@ mappings: Debian8:
xen: - deb: libxen-dev + deb-x86_64: libxen-dev + deb-armv7l: libxen-dev + deb-aarch64: libxen-dev Fedora: xen-devel
Updating the documentation as you implement new features is good, but these last two hunks should go in a separate commit. -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 26, 2019 at 03:00:58PM +0100, Andrea Bolognani wrote:
On Tue, 2019-02-26 at 11:00 +0000, Daniel P. Berrangé wrote:
For example to prevent Xen being installed on any s390x
xen: default-s390x: deb: libxen-dev Fedora: xen-devel
Or the inverse to only install Xen on x86_64 on Debian, but allow it on all archs on Fedora
xen: deb-x86_64: libxen-dev Fedora: xen-devel
Note that the architecture specific matches are considered after all the non-architcture matches.
I feel like the order entries are processed with this implementation can be quite counter-intuitive. The root problem of course is that up until this point we have had a single axis to work on, and we're now introducing a second one which is independent of the existing one but which we still have to fit along with it into the same flat hierarchy somehow...
Consider an example such as
foo: Fedora: oldfoo Fedora-s390x: FedoraRawhide: foo
The general intuition up until now has been that appending something to a label would make it more specific, and only one item on each level would possibly match, but now both Fedora-s390x and FedoraRawhide appear to be on the same level and *both* could apply to an s390x Fedora Rawhide machine, making it less clear which one would take precedence.
Yes, I wasn't entirely happy with that.
I suggest that the example above would be rewritten as
foo: Fedora: oldfoo FedoraRawhide: foo s390x-Fedora:
where two changes happened: first of all, the lines have been shuffled around to match the order they would actually be processed, which is something that we could have done with the above as well, but more importantly the architecture name is now *prepended* to the existing labels to clearly signal that it's part of a completely separate hierarchy. The ambiguity is now gone entirely, and which entry has precedence is easier to see at a glance.
I wouldn't say its unambigous, but it is less bad than before which is nice :-) 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 :|
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 dockerfile -x s390x libvirt-debian-9 libvirt This is only valid when using a 'deb' based distro. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- 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(-) diff --git a/guests/host_vars/libvirt-debian-9/main.yml b/guests/host_vars/libvirt-debian-9/main.yml index ec7e6b4..3bf4ae1 100644 --- a/guests/host_vars/libvirt-debian-9/main.yml +++ 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 + armv6l: + package_arch: armel + abi: arm-linux-gnueabi + cross_gcc: gcc-arm-linux-gnueabi + armv7l: + package_arch: armhf + abi: arm-linux-gnueabihf + cross_gcc: gcc-arm-linux-gnueabihf + i686: + package_arch: i386 + abi: i386-linux-gnu + mips: + package_arch: mips + abi: mips-linux-gnu + cross_gcc: gcc-mips-linux-gnu + mipsel: + package_arch: mipsel + abi: mipsel-linux-gnu + cross_gcc: gcc-mipsel-linux-gnu + mips64: + package_arch: mips64 + abi: mips64-linux-gnu + cross_gcc: gcc-mips64-linux-gnu + mips64el: + package_arch: mips64el + abi: mips64el-linux-gnu + cross_gcc: gcc-mips64el-linux-gnu + ppc64el: + package_arch: ppc64el + abi: ppc64el-linux-gnu + cross_gcc: gcc-ppc64el-linux-gnu + s390x: + package_arch: s390x + abi: s390x-linux-gnu + cross_gcc: gcc-s390x-linux-gnu + x86_64: + package_arch: amd64 + abi: x86_64-linux-gnu diff --git a/guests/host_vars/libvirt-debian-sid/main.yml b/guests/host_vars/libvirt-debian-sid/main.yml index 1c7a29b..b14a564 100644 --- a/guests/host_vars/libvirt-debian-sid/main.yml +++ 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 + armv6l: + package_arch: armel + abi: arm-linux-gnueabi + cross_gcc: gcc-arm-linux-gnueabi + armv7l: + package_arch: armhf + abi: arm-linux-gnueabihf + cross_gcc: gcc-arm-linux-gnueabihf + i686: + package_arch: i386 + abi: i686-linux-gnu + cross_gcc: gcc-i686-linux-gnu + mips: + package_arch: mips + abi: mips-linux-gnu + cross_gcc: gcc-mips-linux-gnu + mipsel: + package_arch: mipsel + abi: mipsel-linux-gnu + cross_gcc: gcc-mipsel-linux-gnu + mips64: + package_arch: mips64 + abi: mips64-linux-gnu + cross_gcc: gcc-mips64-linux-gnu + mips64el: + package_arch: mips64el + abi: mips64el-linux-gnu + cross_gcc: gcc-mips64el-linux-gnu + ppc64el: + package_arch: ppc64el + abi: ppc64el-linux-gnu + cross_gcc: gcc-ppc64el-linux-gnu + s390x: + package_arch: s390x + abi: s390x-linux-gnu + cross_gcc: gcc-s390x-linux-gnu + x86_64: + package_arch: amd64 + abi: x86_64-linux-gnu diff --git a/guests/lcitool b/guests/lcitool index 263ab0d..0fb30a5 100755 --- a/guests/lcitool +++ 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", + ) def _execute_playbook(self, playbook, hosts, projects, git_revision): base = Util.get_base() @@ -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 if package_format not in ["deb", "rpm"]: raise Error("Host {} doesn't support Dockerfiles".format(host)) + 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] + projects = self._projects.expand_pattern(args.projects) for project in projects: if project not in facts["projects"]: @@ -552,25 +569,50 @@ class Application: ) pkgs = {} + cross_pkgs = {} base_keys = ["default", package_format, os_name, os_full] - keys = base_keys + [k + "-" + self._native_arch for k in base_keys] + cross_keys = [] + if args.cross_arch: + keys = base_keys + [k + "-" + args.cross_arch for k in base_keys] + cross_keys = [k + "-cross-arch" for k in base_keys] + else: + keys = base_keys + [k + "-" + self._native_arch for k in base_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" + for key in cross_keys: + if key in mappings[package]: + policy = mappings[package][key] + if policy not in ["native", "foreign", "skip"]: + raise Error("Unexpected policy {} for {}", + policy, package) + for key in keys: if key in mappings[package]: pkgs[package] = mappings[package][key] if package not in pkgs: continue - if pkgs[package] is None: + if policy == "foreign" and pkgs[package] is not None: + cross_pkgs[package] = pkgs[package] + if pkgs[package] is None or policy in ["skip", "foreign"]: del pkgs[package] print("FROM {}".format(facts["docker_base"])) varmap = {} varmap["pkgs"] = " \\\n ".join(sorted(set(pkgs.values()))) + if args.cross_arch: + if package_format != "deb": + raise Error("Cannot install foreign {} packages".format(package_format)) + varmap["cross_arch"] = cross_build_facts["package_arch"] + pkg_names = [p + ":" + cross_build_facts["package_arch"] for p in cross_pkgs.values()] + pkg_names.append(cross_build_facts["cross_gcc"]) + varmap["cross_pkgs"] = " \\\n ".join(sorted(set(pkg_names))) + varmap["cross_target"] = cross_build_facts["abi"] if package_format == "deb": sys.stdout.write(textwrap.dedent(""" RUN export DEBIAN_FRONTEND=noninteractive && \\ @@ -581,6 +623,25 @@ class Application: apt-get autoremove -y && \\ apt-get autoclean -y """).format(**varmap)) + if args.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 export DEBIAN_FRONTEND=noninteractive && \\ + dpkg --add-architecture {cross_arch} && \\ + apt-get update && \\ + apt-get dist-upgrade -y && \\ + apt-get install --no-install-recommends -y \\ + {cross_pkgs} && \\ + apt-get autoremove -y && \\ + apt-get autoclean -y + + ENV TARGET "{cross_target}" + ENV CONFIGURE_OPTS "--host={cross_target} \\ + --target={cross_target}" + ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_target}/pkgconfig" + """).format(**varmap)) elif package_format == "rpm": if os_name == "Fedora" and os_version == "Rawhide": sys.stdout.write(textwrap.dedent(""" diff --git a/guests/vars/mappings.yml b/guests/vars/mappings.yml index ea46ce4..ebf7e60 100644 --- a/guests/vars/mappings.yml +++ b/guests/vars/mappings.yml @@ -57,11 +57,15 @@ # deb-x86_64: libxen-dev # Fedora-x86_64: xen-devel # +# In parallel with this 'cross-arch: native|foreign|skip' entries can +# used to indicate the policy when setting up a cross-architecture +# build environment. If omitted 'native' is assumed mappings: apparmor: deb: libapparmor-dev + deb-cross-arch: foreign augeas: default: augeas @@ -80,6 +84,7 @@ mappings: avahi: deb: libavahi-client-dev + deb-cross-arch: foreign pkg: avahi rpm: avahi-devel @@ -106,6 +111,7 @@ mappings: cyrus-sasl: deb: libsasl2-dev + deb-cross-arch: foreign pkg: cyrus-sasl rpm: cyrus-sasl-devel @@ -116,6 +122,7 @@ mappings: device-mapper: deb: libdevmapper-dev + deb-cross-arch: foreign rpm: device-mapper-devel dnsmasq: @@ -124,6 +131,7 @@ mappings: dtrace: deb: systemtap-sdt-dev + deb-cross-arch: skip rpm: systemtap-sdt-devel dwarves: @@ -144,6 +152,7 @@ mappings: fuse: deb: libfuse-dev + deb-cross-arch: foreign pkg: fusefs-libs rpm: fuse-devel @@ -159,19 +168,23 @@ mappings: glib2: deb: libglib2.0-dev + deb-cross-arch: foreign pkg: glib rpm: glib2-devel glibc: deb: libc6-dev + deb-cross-arch: foreign rpm: glibc-devel glibc-static: deb: libc6-dev + deb-cross-arch: foreign rpm: glibc-static glusterfs: deb: libglusterfs-dev + deb-cross-arch: foreign rpm: glusterfs-api-devel Debian8: glusterfs-common Debian9: glusterfs-common @@ -183,6 +196,7 @@ mappings: gnutls: deb: libgnutls28-dev + deb-cross-arch: foreign pkg: gnutls rpm: gnutls-devel @@ -192,11 +206,13 @@ mappings: gobject-introspection: deb: libgirepository1.0-dev + deb-cross-arch: foreign pkg: gobject-introspection rpm: gobject-introspection-devel gtk3: deb: libgtk-3-dev + deb-cross-arch: foreign pkg: gtk3 rpm: gtk3-devel @@ -211,6 +227,7 @@ mappings: gtk-vnc2: deb: libgtk-vnc-2.0-dev + deb-cross-arch: foreign pkg: gtk-vnc rpm: gtk-vnc2-devel @@ -243,32 +260,39 @@ mappings: json-glib: deb: libjson-glib-dev + deb-cross-arch: foreign pkg: json-glib rpm: json-glib-devel libacl: deb: libacl1-dev + deb-cross-arch: foreign rpm: libacl-devel libarchive: deb: libarchive-dev + deb-cross-arch: foreign pkg: libarchive rpm: libarchive-devel libattr: deb: libattr1-dev + deb-cross-arch: foreign rpm: libattr-devel libaudit: deb: libaudit-dev + deb-cross-arch: foreign rpm: audit-libs-devel libblkid: deb: libblkid-dev + deb-cross-arch: foreign rpm: libblkid-devel libcap-ng: deb: libcap-ng-dev + deb-cross-arch: foreign rpm: libcap-ng-devel libcmpiutil: @@ -276,67 +300,81 @@ mappings: libconfig: deb: libconfig-dev + deb-cross-arch: foreign pkg: libconfig rpm: libconfig-devel libcurl: deb: libcurl4-gnutls-dev + deb-cross-arch: foreign pkg: curl rpm: libcurl-devel libdbus: deb: libdbus-1-dev + deb-cross-arch: foreign pkg: dbus rpm: dbus-devel libgovirt: rpm: libgovirt-devel Debian: libgovirt-dev + Debian-cross-arch: foreign Debian8: libiscsi: deb: libiscsi-dev + deb-cross-arch: foreign rpm: libiscsi-devel libnl3: deb: libnl-3-dev + deb-cross-arch: foreign rpm: libnl3-devel libnlroute3: deb: libnl-route-3-dev + deb-cross-arch: foreign rpm: libnl3-devel libnuma: deb: libnuma-dev + deb-cross-arch: foreign deb-armv6l: deb-armv7l: rpm: numactl-devel libparted: deb: libparted-dev + deb-cross-arch: foreign rpm: parted-devel libpcap: deb: libpcap0.8-dev + deb-cross-arch: foreign pkg: libpcap rpm: libpcap-devel libpciaccess: deb: libpciaccess-dev + deb-cross-arch: foreign pkg: libpciaccess rpm: libpciaccess-devel librbd: deb: librbd-dev + deb-cross-arch: foreign Fedora: librbd-devel CentOS7: librbd1-devel libselinux: deb: libselinux1-dev + deb-cross-arch: foreign rpm: libselinux-devel libsoup: deb: libsoup2.4-dev + deb-cross-arch: foreign pkg: libsoup rpm: libsoup-devel @@ -344,15 +382,18 @@ mappings: pkg: libssh rpm: libssh-devel Debian: libssh-gcrypt-dev + Debian-cross-arch: foreign Ubuntu: libssh-dev libssh2: deb: libssh2-1-dev + deb-cross-arch: foreign pkg: libssh2 rpm: libssh2-devel libtirpc: deb: libtirpc-dev + deb-cross-arch: foreign rpm: libtirpc-devel libtool: @@ -364,20 +405,24 @@ mappings: libudev: deb: libudev-dev + deb-cross-arch: foreign rpm: libudev-devel libuuid: deb: uuid-dev + deb-cross-arch: foreign pkg: e2fsprogs-libuuid rpm: libuuid-devel libxml2: deb: libxml2-dev + deb-cross-arch: foreign pkg: libxml2 rpm: libxml2-devel libxslt: deb: libxslt1-dev + deb-cross-arch: foreign pkg: libxslt rpm: libxslt-devel @@ -554,6 +599,7 @@ mappings: netcf: deb: libnetcf-dev + deb-cross-arch: skip rpm: netcf-devel numad: @@ -714,6 +760,7 @@ mappings: python2-devel: deb: python-dev + deb-cross-arch: foreign pkg: python2 rpm: python2-devel @@ -738,6 +785,7 @@ mappings: python3-devel: deb: python3-dev + deb-cross-arch: foreign pkg: python3 Fedora: python3-devel @@ -783,6 +831,7 @@ mappings: readline: deb: libreadline-dev + deb-cross-arch: foreign pkg: readline rpm: readline-devel @@ -797,6 +846,7 @@ mappings: sanlock: deb: libsanlock-dev + deb-cross-arch: foreign rpm: sanlock-devel screen: @@ -818,6 +868,7 @@ mappings: spice-gtk3: deb: libspice-client-gtk-3.0-dev + deb-cross-arch: foreign pkg: spice-gtk rpm: spice-gtk3-devel @@ -849,10 +900,12 @@ mappings: wireshark: deb: wireshark-dev + deb-cross-arch: skip Fedora: wireshark-devel Debian8: xen: + deb-cross-arch: foreign deb-x86_64: libxen-dev deb-armv7l: libxen-dev deb-aarch64: libxen-dev @@ -860,6 +913,7 @@ mappings: xfsprogs: deb: xfslibs-dev + deb-cross-arch: foreign rpm: xfsprogs-devel xmllint: @@ -872,14 +926,17 @@ mappings: xz: deb: liblzma-dev + deb-cross-arch: foreign rpm: xz-devel xz-static: deb: liblzma-dev + deb-cross-arch: foreign Fedora: xz-static yajl: deb: libyajl-dev + deb-cross-arch: foreign pkg: yajl rpm: yajl-devel @@ -890,8 +947,10 @@ mappings: zlib: deb: zlib1g-dev + deb-cross-arch: foreign rpm: zlib-devel zlib-static: deb: zlib1g-dev + deb-cross-arch: foreign rpm: zlib-static -- 2.20.1
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
On Tue, Feb 26, 2019 at 07:04:16PM +0100, Andrea Bolognani wrote:
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 moved it back here as the pacakge_arch / abi fields are not specific to cross builds. They're general information about the distro / toolchain... but this all goes away with a static mapping in the code instead so it doesn't matter.
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...
What we do with MinGW right now is not a good example to follow. There are enough differences betweeen software versions that I see failures building on stable Fedora that don't appear on rawhide & vica-versa. This is particularly causing me pain for virt-viewer release builds as the msi build process is frequently failing due to changed DLL versions. So when I add MSI builds to the CI, I'll want MinGW packages across mulitple distro versions. The other problem is that both rawhide & sid are rolling unstable distros which break. Right now I can't even test the cross arch builds on Sid because some recent update has broken the ability to install the cross compiler toolchain at all. So only supporting the unstable Sid is not a good idea.
+ 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 'abi' is what is prefixed on the toolchain binaries, and so for this purpose i686-linux-gnu is actually correct: https://packages.debian.org/sid/amd64/gcc-i686-linux-gnu/filelist
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 :(
Actually we can.
[...]
+++ 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'.
This dict goes away if we use a static mapping table.
[...]
@@ -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 :)
The compelling reason for using mappings.xml is that it provides a single source of information for packages. This way when adding new packages to the list, it is more likely we'll remember to add the right cross arch info at the same time.
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.
"foreign" is the logical inverse of "native", so I think it is actually the right word to use here. "cross" is misleading as we're not installing a cross-built package, we're installing the native built package from the foreign architecture. 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 Thu, 2019-02-28 at 15:52 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 26, 2019 at 07:04:16PM +0100, Andrea Bolognani wrote: [...]
+++ 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 moved it back here as the pacakge_arch / abi fields are not specific to cross builds. They're general information about the distro / toolchain...
but this all goes away with a static mapping in the code instead so it doesn't matter.
Okay, fair enough. For whatever local override we might need though, eg. to disable cross-building for i386 on Debian 9, we still want to use a separate facts file.
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...
What we do with MinGW right now is not a good example to follow. There are enough differences betweeen software versions that I see failures building on stable Fedora that don't appear on rawhide & vica-versa. This is particularly causing me pain for virt-viewer release builds as the msi build process is frequently failing due to changed DLL versions. So when I add MSI builds to the CI, I'll want MinGW packages across mulitple distro versions.
The other problem is that both rawhide & sid are rolling unstable distros which break. Right now I can't even test the cross arch builds on Sid because some recent update has broken the ability to install the cross compiler toolchain at all. So only supporting the unstable Sid is not a good idea.
Alright, makes sense.
+ 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 'abi' is what is prefixed on the toolchain binaries, and so for this purpose i686-linux-gnu is actually correct:
https://packages.debian.org/sid/amd64/gcc-i686-linux-gnu/filelist
Okay, so I guess you need to have cross_target=i686-linux-gnu for ENV CONFIGURE_OPTS "--host={cross_target} \ --target={cross_target}" to work; however, that will cause issues for ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_target}/pkgconfig" because as shown above paths look like /usr/lib/i386-linux-gnu/pkgconfig/glib-2.0.pc and so on. What an inconsistent mess! Does it mean we need to carry around both? :( ... Actually, it doesn't look like it: I just tried cross-building for i686 (on sid/x86_64) by simply passing --host=i686-linux-gnu --target=i686-linux-gnu and the build system was able to locate all necessary dependencies despite not having set $PKG_CONFIG_LIBDIR in the environment! This seems to be thanks to i686-linux-gnu-pkg-config, which is a symlink to /usr/share/pkg-config-crosswrapper, being picked up and invoked instead of the native one. tl;dr It looks like we can generate 'cross_gcc' programmatically and we can even get rid of one ENV statement! Neat :) [...]
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.
"foreign" is the logical inverse of "native", so I think it is actually the right word to use here. "cross" is misleading as we're not installing a cross-built package, we're installing the native built package from the foreign architecture.
Sure, let's keep it that way then. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Feb 28, 2019 at 06:46:41PM +0100, Andrea Bolognani wrote:
On Thu, 2019-02-28 at 15:52 +0000, Daniel P. Berrangé wrote:
On Tue, Feb 26, 2019 at 07:04:16PM +0100, Andrea Bolognani wrote: [...]
+ 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 'abi' is what is prefixed on the toolchain binaries, and so for this purpose i686-linux-gnu is actually correct:
https://packages.debian.org/sid/amd64/gcc-i686-linux-gnu/filelist
Okay, so I guess you need to have cross_target=i686-linux-gnu for
ENV CONFIGURE_OPTS "--host={cross_target} \ --target={cross_target}"
to work; however, that will cause issues for
ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_target}/pkgconfig"
because as shown above paths look like
/usr/lib/i386-linux-gnu/pkgconfig/glib-2.0.pc
and so on. What an inconsistent mess! Does it mean we need to carry around both? :(
... Actually, it doesn't look like it: I just tried cross-building for i686 (on sid/x86_64) by simply passing
--host=i686-linux-gnu --target=i686-linux-gnu
and the build system was able to locate all necessary dependencies despite not having set $PKG_CONFIG_LIBDIR in the environment!
This seems to be thanks to i686-linux-gnu-pkg-config, which is a symlink to /usr/share/pkg-config-crosswrapper, being picked up and invoked instead of the native one.
Interesting. I'll have to check this again. In my previous versions of this series, pkgconfig was mistakenly finding the native .pc files, and PKG_CONFIG_LIBDIR was needed to fix it. So that can't have been using the i686-linux-gnu-pkg-config binary before. I'll try and see what changed.. 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 Thu, 2019-02-28 at 18:03 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 28, 2019 at 06:46:41PM +0100, Andrea Bolognani wrote:
Okay, so I guess you need to have cross_target=i686-linux-gnu for
ENV CONFIGURE_OPTS "--host={cross_target} \ --target={cross_target}"
to work; however, that will cause issues for
ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_target}/pkgconfig"
because as shown above paths look like
/usr/lib/i386-linux-gnu/pkgconfig/glib-2.0.pc
and so on. What an inconsistent mess! Does it mean we need to carry around both? :(
... Actually, it doesn't look like it: I just tried cross-building for i686 (on sid/x86_64) by simply passing
--host=i686-linux-gnu --target=i686-linux-gnu
and the build system was able to locate all necessary dependencies despite not having set $PKG_CONFIG_LIBDIR in the environment!
This seems to be thanks to i686-linux-gnu-pkg-config, which is a symlink to /usr/share/pkg-config-crosswrapper, being picked up and invoked instead of the native one.
Interesting. I'll have to check this again. In my previous versions of this series, pkgconfig was mistakenly finding the native .pc files, and PKG_CONFIG_LIBDIR was needed to fix it. So that can't have been using the i686-linux-gnu-pkg-config binary before. I'll try and see what changed..
Unfortunately that only seems to apply to Debian Sid: the version of pkg-config in Debian 9 is quite a bit older and apparently not as smart. So we need to have $PKG_CONFIG_LIBDIR in the environment after all, and also deal with the mess described above somehow :( -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 01, 2019 at 05:19:29PM +0100, Andrea Bolognani wrote:
On Thu, 2019-02-28 at 18:03 +0000, Daniel P. Berrangé wrote:
On Thu, Feb 28, 2019 at 06:46:41PM +0100, Andrea Bolognani wrote:
Okay, so I guess you need to have cross_target=i686-linux-gnu for
ENV CONFIGURE_OPTS "--host={cross_target} \ --target={cross_target}"
to work; however, that will cause issues for
ENV PKG_CONFIG_LIBDIR "/usr/lib/{cross_target}/pkgconfig"
because as shown above paths look like
/usr/lib/i386-linux-gnu/pkgconfig/glib-2.0.pc
and so on. What an inconsistent mess! Does it mean we need to carry around both? :(
... Actually, it doesn't look like it: I just tried cross-building for i686 (on sid/x86_64) by simply passing
--host=i686-linux-gnu --target=i686-linux-gnu
and the build system was able to locate all necessary dependencies despite not having set $PKG_CONFIG_LIBDIR in the environment!
This seems to be thanks to i686-linux-gnu-pkg-config, which is a symlink to /usr/share/pkg-config-crosswrapper, being picked up and invoked instead of the native one.
Interesting. I'll have to check this again. In my previous versions of this series, pkgconfig was mistakenly finding the native .pc files, and PKG_CONFIG_LIBDIR was needed to fix it. So that can't have been using the i686-linux-gnu-pkg-config binary before. I'll try and see what changed..
Unfortunately that only seems to apply to Debian Sid: the version of pkg-config in Debian 9 is quite a bit older and apparently not as smart.
Ah ha, that explains it - I only tested that Deb9 originally.
So we need to have $PKG_CONFIG_LIBDIR in the environment after all, and also deal with the mess described above somehow :(
Yeah, I'll think about how to deal with that mess 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 :|
participants (2)
-
Andrea Bolognani -
Daniel P. Berrangé