From 2f1cb0edb1df9a6f1ca908a50b31e2c06910abea Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Mon, 25 Sep 2023 14:24:57 +0200 Subject: [PATCH 1/8] Remove any duplicated code loading configuration from ENV The current conf.get_config() function already handles loading from ENV. Also, always use osc.build.calculate_build_root() instead of making a custom variable substitution. --- osc/build.py | 28 ++++------------------------ osc/commandline.py | 5 ++--- osc/conf.py | 4 +++- 3 files changed, 9 insertions(+), 28 deletions(-) diff --git a/osc/build.py b/osc/build.py index ffc3a5956c..4136aa10fb 100644 --- a/osc/build.py +++ b/osc/build.py @@ -606,19 +606,19 @@ def calculate_prj_pac(store, opts, descr): def calculate_build_root(apihost, prj, pac, repo, arch): - buildroot = os.environ.get('OSC_BUILD_ROOT', conf.config['build-root']) \ + buildroot = conf.config["build-root"] \ % {'repo': repo, 'arch': arch, 'project': prj, 'package': pac, 'apihost': apihost} return buildroot def build_as_user(): - if os.environ.get('OSC_SU_WRAPPER', conf.config['su-wrapper']).split(): + if conf.config["su-wrapper"]: return False return True def su_wrapper(cmd): - sucmd = os.environ.get('OSC_SU_WRAPPER', conf.config['su-wrapper']).split() + sucmd = conf.config['su-wrapper'].split() if sucmd: if sucmd[0] == 'su': if sucmd[-1] == '-c': @@ -782,18 +782,6 @@ def main(apiurl, store, opts, argv): if opts.wipe: buildargs.append("--wipe") - orig_build_root = config['build-root'] - # make it possible to override configuration of the rc file - for var in ['OSC_PACKAGECACHEDIR', 'OSC_SU_WRAPPER', 'OSC_BUILD_ROOT']: - val = os.getenv(var) - if val: - if var.startswith('OSC_'): - var = var[4:] - var = var.lower().replace('_', '-') - if var in config: - print('Overriding config value for %s=\'%s\' with \'%s\'' % (var, config[var], val)) - config[var] = val - pacname = pac if pacname == '_repository': if not opts.local_package: @@ -805,15 +793,7 @@ def main(apiurl, store, opts, argv): pacname = os.path.splitext(os.path.basename(build_descr))[0] apihost = urlsplit(apiurl)[1] if not build_root: - build_root = config['build-root'] - if build_root == orig_build_root: - # ENV var was not set - build_root = config['api_host_options'][apiurl].get('build-root', build_root) - try: - build_root = build_root % {'repo': repo, 'arch': arch, - 'project': prj, 'package': pacname, 'apihost': apihost} - except KeyError: - pass + build_root = calculate_build_root(apihost, prj, pacname, repo, arch) # We configure sccache after pacname, so that in default cases we can have an sccache for each # package to prevent cross-cache polutions. It helps to make the local-use case a bit nicer. diff --git a/osc/commandline.py b/osc/commandline.py index 8ffdeef1c9..91c6edf260 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -6419,10 +6419,9 @@ def do_localbuildlog(self, subcmd, opts, *args): raise oscerr.WrongArgs('Wrong number of arguments.') # TODO: refactor/unify buildroot calculation and move it to core.py - buildroot = os.environ.get('OSC_BUILD_ROOT', conf.config['build-root']) apihost = urlsplit(self.get_api_url())[1] - buildroot = buildroot % {'project': project, 'package': package, - 'repo': repo, 'arch': arch, 'apihost': apihost} + buildroot = osc_build.calculate_build_root(apihost, project, package, repo, arch) + offset = 0 if opts.offset: offset = int(opts.offset) diff --git a/osc/conf.py b/osc/conf.py index 77f100579e..27d8291c48 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -1005,6 +1005,8 @@ def apiurl_aliases(self): Supported substitutions: ``%(repo)s``, ``%(arch)s``, ``%(project)s``, ``%(package)s`` and ``%(apihost)s`` where ``apihost`` is the hostname extracted from the currently used ``apiurl``. + NOTE: The configuration holds the original unexpanded string. Call ``osc.build.get_build_root()`` with proper arguments to retrieve an actual path. + Passed as ``--root `` to the build tool. """ ), @@ -1745,7 +1747,7 @@ def get_config(override_conffile=None, Configure osc. The configuration options are loaded with the following priority: - 1. environment variables: OSC_ + 1. environment variables: OSC_ 2. override arguments provided to get_config() 3. oscrc config file """ From 8eb360234ecfbda713e4736080147b890e32cc98 Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Mon, 25 Sep 2023 14:56:50 +0200 Subject: [PATCH 2/8] Add rootless build support to 'build' command for 'kvm' and 'podman' vm types To avoid filesystem permission collisions with the builds using su_wrapper, use an alternative buildroot path that appends username to '/var/tmp/build-root' for the rootless builds. --- osc/build.py | 34 ++++++++++++++++++++++++++++------ osc/commandline.py | 7 ++++--- osc/conf.py | 9 ++++++--- 3 files changed, 38 insertions(+), 12 deletions(-) diff --git a/osc/build.py b/osc/build.py index 4136aa10fb..c78ac623ec 100644 --- a/osc/build.py +++ b/osc/build.py @@ -4,6 +4,7 @@ # either version 2, or (at your option) any later version. import fnmatch +import getpass import glob import os import re @@ -605,9 +606,24 @@ def calculate_prj_pac(store, opts, descr): return project, package -def calculate_build_root(apihost, prj, pac, repo, arch): - buildroot = conf.config["build-root"] \ - % {'repo': repo, 'arch': arch, 'project': prj, 'package': pac, 'apihost': apihost} +def calculate_build_root_user(vm_type): + if vm_type in ("kvm", "podman"): + return getpass.getuser() + return None + + +def calculate_build_root(apihost, prj, pac, repo, arch, user=None): + user = user or "" + dash_user = f"-{user:s}" if user else "" + buildroot = conf.config["build-root"] % { + 'apihost': apihost, + 'project': prj, + 'package': pac, + 'repo': repo, + 'arch': arch, + "user": user, + "dash_user": dash_user, + } return buildroot @@ -632,9 +648,12 @@ def su_wrapper(cmd): def run_build(opts, *args): cmd = [conf.config['build-cmd']] cmd += args - cmd = su_wrapper(cmd) + user = calculate_build_root_user(opts.vm_type) + if not user: + cmd = su_wrapper(cmd) + if not opts.userootforbuild: cmd.append('--norootforbuild') return run_external(cmd[0], *cmd[1:]) @@ -793,7 +812,8 @@ def main(apiurl, store, opts, argv): pacname = os.path.splitext(os.path.basename(build_descr))[0] apihost = urlsplit(apiurl)[1] if not build_root: - build_root = calculate_build_root(apihost, prj, pacname, repo, arch) + user = calculate_build_root_user(vm_type) + build_root = calculate_build_root(apihost, prj, pacname, repo, arch, user) # We configure sccache after pacname, so that in default cases we can have an sccache for each # package to prevent cross-cache polutions. It helps to make the local-use case a bit nicer. @@ -1472,7 +1492,9 @@ def __str__(self): cmd += specialcmdopts + vm_options + buildargs cmd += [build_descr] - cmd = su_wrapper(cmd) + # determine if we're building under root (user == None) and use su_wrapper accordingly + if calculate_build_root_user(vm_type) is None: + cmd = su_wrapper(cmd) # change personality, if needed if hostarch != bi.buildarch and bi.buildarch in change_personality: diff --git a/osc/commandline.py b/osc/commandline.py index 91c6edf260..dcc401a360 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -7267,7 +7267,8 @@ def do_build(self, subcmd, opts, *args): repo, arch, build_descr = args prj, pac = osc_build.calculate_prj_pac(store, opts, build_descr) apihost = urlsplit(self.get_api_url())[1] - build_root = osc_build.calculate_build_root(apihost, prj, pac, repo, arch) + user = osc_build.calculate_build_root_user(opts.vm_type) + build_root = osc_build.calculate_build_root(apihost, prj, pac, repo, arch, user) print(build_root) return @@ -7280,8 +7281,8 @@ def do_build(self, subcmd, opts, *args): repo, arch, build_descr = args prj, pac = osc_build.calculate_prj_pac(store, opts, build_descr) apihost = urlsplit(self.get_api_url())[1] - build_root = osc_build.calculate_build_root(apihost, prj, pac, repo, - arch) + user = osc_build.calculate_build_root_user(opts.vm_type) + build_root = osc_build.calculate_build_root(apihost, prj, pac, repo, arch, user) if opts.wipe and not opts.force: # Confirm delete print("Really wipe '%s'? [y/N]: " % build_root) diff --git a/osc/conf.py b/osc/conf.py index 27d8291c48..3240589342 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -997,13 +997,16 @@ def apiurl_aliases(self): ) # type: ignore[assignment] build_root: str = Field( - default="/var/tmp/build-root/%(repo)s-%(arch)s", + default="/var/tmp/build-root%(dash_user)s/%(repo)s-%(arch)s", description=textwrap.dedent( """ Path to the build root directory. - Supported substitutions: ``%(repo)s``, ``%(arch)s``, ``%(project)s``, ``%(package)s`` and ``%(apihost)s`` - where ``apihost`` is the hostname extracted from the currently used ``apiurl``. + Supported substitutions: ``%(repo)s``, ``%(arch)s``, ``%(project)s``, ``%(package)s``, ``%(apihost)s``, ``%(user)s``, ``%(dash_user)s`` + where:: + + - ``apihost`` is the hostname extracted from the currently used ``apiurl``. + - ``dash_user`` is the username prefixed with a dash. If ``user`` is empty, ``dash_user`` is also empty. NOTE: The configuration holds the original unexpanded string. Call ``osc.build.get_build_root()`` with proper arguments to retrieve an actual path. From 88c2bf4fea06c2fd08a14935b41b0767952a9733 Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 26 Sep 2023 13:34:02 +0200 Subject: [PATCH 3/8] Avoid adding a newline to prompt in 'wipe' command --- osc/commandline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/commandline.py b/osc/commandline.py index dcc401a360..68def437ce 100644 --- a/osc/commandline.py +++ b/osc/commandline.py @@ -7285,7 +7285,7 @@ def do_build(self, subcmd, opts, *args): build_root = osc_build.calculate_build_root(apihost, prj, pac, repo, arch, user) if opts.wipe and not opts.force: # Confirm delete - print("Really wipe '%s'? [y/N]: " % build_root) + print("Really wipe '%s'? [y/N]: " % build_root, end="") choice = raw_input().lower() if choice != 'y': print('Aborting') From 28efb4396a20b913f22891c53cecb9c9f9ccf54d Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 26 Sep 2023 13:37:30 +0200 Subject: [PATCH 4/8] Rename conf.Options.build_type to vm_type to be consistent with obs build and osc --vm-type option --- osc/conf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/conf.py b/osc/conf.py index 3240589342..79c64bbe75 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -970,7 +970,7 @@ def apiurl_aliases(self): ini_key="build-jobs", ) # type: ignore[assignment] - build_type: Optional[str] = Field( + vm_type: Optional[str] = Field( default=None, description=textwrap.dedent( """ From 47f32e6ef19f41104ba59734c531f86151991c72 Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Tue, 26 Sep 2023 13:39:23 +0200 Subject: [PATCH 5/8] Update list of supported vm_type values in conf.Options.vm_type --- osc/conf.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/osc/conf.py b/osc/conf.py index 79c64bbe75..b5bf6bca26 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -976,11 +976,21 @@ def apiurl_aliases(self): """ Type of the build environment passed the build tool as the ``--vm-type`` option: - - : chroot build - - kvm: KVM VM build (needs build-device, build-swap, build-memory) - - xen: XEN VM build (needs build-device, build-swap, build-memory) - - qemu: [EXPERIMENTAL] QEMU VM build - - lxc: [EXPERIMENTAL] LXC build + - chroot build + - kvm KVM VM build (rootless, needs build-device, build-swap, build-memory) + - xen XEN VM build (needs build-device, build-swap, build-memory) + - qemu [EXPERIMENTAL] QEMU VM build + - lxc [EXPERIMENTAL] LXC build + - uml + - zvm + - openstack + - ec2 + - docker + - podman (rootless) + - pvm + - nspawn + + See ``build --help`` for more details about supported options. """ ), ini_key="build-type", From 41ce9326737e661e6533f5f00f768b865252c35c Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Wed, 27 Sep 2023 09:21:46 +0200 Subject: [PATCH 6/8] Fix 'build' command to pass '--vm-type' option to the underlying build tool --- osc/build.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osc/build.py b/osc/build.py index c78ac623ec..ff6a2f782b 100644 --- a/osc/build.py +++ b/osc/build.py @@ -648,7 +648,9 @@ def su_wrapper(cmd): def run_build(opts, *args): cmd = [conf.config['build-cmd']] cmd += args - cmd = su_wrapper(cmd) + + if opts.vm_type: + cmd.extend(["--vm-type", opts.vm_type]) user = calculate_build_root_user(opts.vm_type) if not user: From a33d4c2d41bb338410910bb94771ced08b606d8e Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Fri, 29 Sep 2023 09:53:59 +0200 Subject: [PATCH 7/8] Print a hint to clean the build root after a failed build --- osc/build.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osc/build.py b/osc/build.py index ff6a2f782b..95a2164b3a 100644 --- a/osc/build.py +++ b/osc/build.py @@ -1510,7 +1510,12 @@ def __str__(self): rc = run_external(cmd[0], *cmd[1:]) if rc: print() - print('The buildroot was:', build_root) + print(f"Build failed with exit code {rc}") + print(f"The buildroot was: {build_root}") + print() + print("Cleaning the build root may fix the problem or allow you to start debugging from a well-defined state:") + print(" - add '--clean' option to your 'osc build' command") + print(" - run 'osc wipe [--vm-type=...]' prior running your 'osc build' command again") sys.exit(rc) except KeyboardInterrupt as keyboard_interrupt_exception: print("keyboard interrupt, killing build ...") From a6946587e1b2115d0d2554c5feaa0314ab6f4fde Mon Sep 17 00:00:00 2001 From: Daniel Mach Date: Fri, 29 Sep 2023 09:54:50 +0200 Subject: [PATCH 8/8] Fix reading configuration from ENV --- osc/conf.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osc/conf.py b/osc/conf.py index b5bf6bca26..77b976a2b0 100644 --- a/osc/conf.py +++ b/osc/conf.py @@ -1869,7 +1869,7 @@ def get_config(override_conffile=None, # priority: env, overrides, config if env_key in os.environ: - value = os.environ["env_key"] + value = os.environ[env_key] elif name in overrides: value = overrides.pop(name) elif ini_key in overrides: