Message ID | 20250202200319.82231-3-uvv.mail@gmail.com |
---|---|
State | New |
Headers | show |
Series | [1/3] bitbake.conf: Add conditional host tool getent | expand |
Thank you for your submission. Patchtest identified one or more issues with the patch. Please see the log below for more information: --- Testing patch /home/patchtest/share/mboxes/3-3-systemd-Build-the-systemctl-executable.patch FAIL: test Upstream-Status presence: Upstream-Status is Submitted, but it is not mentioned where (test_patch.TestPatch.test_upstream_status_presence_format) FAIL: test lic files chksum modified not mentioned: LIC_FILES_CHKSUM changed without "License-Update:" tag and description in commit message (test_metadata.TestMetadata.test_lic_files_chksum_modified_not_mentioned) PASS: pretest src uri left files (test_metadata.TestMetadata.pretest_src_uri_left_files) PASS: test CVE check ignore (test_metadata.TestMetadata.test_cve_check_ignore) PASS: test CVE tag format (test_patch.TestPatch.test_cve_tag_format) PASS: test Signed-off-by presence (test_mbox.TestMbox.test_signed_off_by_presence) PASS: test Signed-off-by presence (test_patch.TestPatch.test_signed_off_by_presence) PASS: test author valid (test_mbox.TestMbox.test_author_valid) PASS: test commit message presence (test_mbox.TestMbox.test_commit_message_presence) PASS: test commit message user tags (test_mbox.TestMbox.test_commit_message_user_tags) PASS: test max line length (test_metadata.TestMetadata.test_max_line_length) PASS: test mbox format (test_mbox.TestMbox.test_mbox_format) PASS: test non-AUH upgrade (test_mbox.TestMbox.test_non_auh_upgrade) PASS: test shortlog format (test_mbox.TestMbox.test_shortlog_format) PASS: test shortlog length (test_mbox.TestMbox.test_shortlog_length) PASS: test src uri left files (test_metadata.TestMetadata.test_src_uri_left_files) PASS: test target mailing list (test_mbox.TestMbox.test_target_mailing_list) SKIP: pretest pylint: No python related patches, skipping test (test_python_pylint.PyLint.pretest_pylint) SKIP: test bugzilla entry format: No bug ID found (test_mbox.TestMbox.test_bugzilla_entry_format) SKIP: test lic files chksum presence: No added recipes, skipping test (test_metadata.TestMetadata.test_lic_files_chksum_presence) SKIP: test license presence: No added recipes, skipping test (test_metadata.TestMetadata.test_license_presence) SKIP: test pylint: No python related patches, skipping test (test_python_pylint.PyLint.test_pylint) SKIP: test series merge on head: Merge test is disabled for now (test_mbox.TestMbox.test_series_merge_on_head) SKIP: test summary presence: No added recipes, skipping test (test_metadata.TestMetadata.test_summary_presence) --- Please address the issues identified and submit a new revision of the patch, or alternatively, reply to this email with an explanation of why the patch should be accepted. If you believe these results are due to an error in patchtest, please submit a bug at https://bugzilla.yoctoproject.org/ (use the 'Patchtest' category under 'Yocto Project Subprojects'). For more information on specific failures, see: https://wiki.yoctoproject.org/wiki/Patchtest. Thank you!
Hi, On Sun, Feb 02, 2025 at 09:03:19PM +0100, Vyacheslav Yurkov via lists.openembedded.org wrote: > From: Vyacheslav Yurkov <uvv.mail@gmail.com> > > Instead of the python re-implementation build the actual systemctl from > the systemd source tree. Could you elaborate why? Some usecase or feature not supported by the python wrapper? Which one? Maybe it makes sense to have a self or runtime test for this usecase. Then the native toolchain set seems to be increasing. While systemd-boot-native is also trying to be simple for ukify.py, it could expand if tools like systemd-measure for TPM calculations would get added. And systemd-sbsign instead of sbsign for secureboot signatures. Maybe it would make sense to cambine all in a full systemd-native recipe? > Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com> > --- > .../systemd/systemd-systemctl-native.bb | 20 +- > ...-meson-add-install-tag-for-systemctl.patch | 24 ++ > .../systemd/systemd-systemctl/systemctl | 366 ------------------ > 3 files changed, 32 insertions(+), 378 deletions(-) > create mode 100644 meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch > delete mode 100755 meta/recipes-core/systemd/systemd-systemctl/systemctl > > diff --git a/meta/recipes-core/systemd/systemd-systemctl-native.bb b/meta/recipes-core/systemd/systemd-systemctl-native.bb > index ffa024caef..57bb1ab830 100644 > --- a/meta/recipes-core/systemd/systemd-systemctl-native.bb > +++ b/meta/recipes-core/systemd/systemd-systemctl-native.bb > @@ -1,17 +1,13 @@ > -SUMMARY = "Wrapper for enabling systemd services" > +SUMMARY = "Systemctl executable from systemd" > > -LICENSE = "MIT" > -LIC_FILES_CHKSUM = "file://${COREBASE}/meta/COPYING.MIT;md5=3da9cfbcb788c80a0384361b4de20420" > +require systemd.inc > > +DEPENDS = "gperf-native libcap-native util-linux-native python3-jinja2-native" > > -inherit native > +inherit pkgconfig meson native > > -SRC_URI = "file://systemctl" > +SRC_URI = "file://0001-meson-add-install-tag-for-systemctl.patch" > > -S = "${WORKDIR}/sources" > -UNPACKDIR = "${S}" > - > -do_install() { > - install -d ${D}${bindir} > - install -m 0755 ${S}/systemctl ${D}${bindir} > -} > +MESON_TARGET = "systemctl:executable" > +MESON_INSTALL_TAGS = "systemctl" > +EXTRA_OEMESON:append = " -Dlink-systemctl-shared=false" > diff --git a/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch b/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch > new file mode 100644 > index 0000000000..a4b3e5a47e > --- /dev/null > +++ b/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch > @@ -0,0 +1,24 @@ > +From fe2eb1dde51648b0e5825766e2f27a859de8521c Mon Sep 17 00:00:00 2001 > +From: Vyacheslav Yurkov <uvv.mail@gmail.com> > +Date: Sun, 2 Feb 2025 10:13:38 +0100 > +Subject: [PATCH] meson: add install tag for systemctl > + > +Upstream-Status: Submitted Would be nice to link the URL here. Cheers, -Mikko
On 2 Feb 2025, at 21:03, Vyacheslav Yurkov via lists.openembedded.org <uvv.mail=gmail.com@lists.openembedded.org> wrote: > Instead of the python re-implementation build the actual systemctl from > the systemd source tree. Very glad to see this - I tried some time ago but the dependency tree at the time was quite invasive to remove so I gave up. Have you verified that the final rootfs is identical (or in fact better) with those compared to our reimplementation? > +DEPENDS = "gperf-native libcap-native util-linux-native python3-jinja2-native" I’d hope you can remove some more of those dependencies as we just need systemctl? > +Upstream-Status: Submitted Submitted where? Thanks Ross
On Mon, Feb 3, 2025 at 8:54 AM Ross Burton via lists.openembedded.org <ross.burton=arm.com@lists.openembedded.org> wrote: > > On 2 Feb 2025, at 21:03, Vyacheslav Yurkov via lists.openembedded.org <uvv.mail=gmail.com@lists.openembedded.org> wrote: > > Instead of the python re-implementation build the actual systemctl from > > the systemd source tree. > > Very glad to see this - I tried some time ago but the dependency tree at the time was quite invasive to remove so I gave up. > As did I, before I did the shell -> python port. In theory I agree this is a good thing. > Have you verified that the final rootfs is identical (or in fact better) with those compared to our reimplementation? > > > +DEPENDS = "gperf-native libcap-native util-linux-native python3-jinja2-native" > > I’d hope you can remove some more of those dependencies as we just need systemctl? > The getent HOSTTOOLS in the other patch concerns me - that feels like it could easily be a host contamination problem. > > +Upstream-Status: Submitted > > Submitted where? > A nervousness I have with this, is this makes a systemd enabled image significantly dependent on systemd at build time, and the history of systemd taking patches which don't match their use cases (which I don't believe this does) is frankly very poor.
On Mon, 3 Feb 2025 at 08:54, Mikko Rapeli via lists.openembedded.org <mikko.rapeli=linaro.org@lists.openembedded.org> wrote: > Could you elaborate why? Some usecase or feature not supported by the python > wrapper? Which one? You got this backwards Mikko. It's you who should elaborate why we should continue using a custom replacement. As long as this change passes the autobuilder, it works as intended, and needs no further justification. Alex
Hi, On Mon, Feb 03, 2025 at 12:07:20PM +0100, Alexander Kanavin wrote: > On Mon, 3 Feb 2025 at 08:54, Mikko Rapeli via lists.openembedded.org > <mikko.rapeli=linaro.org@lists.openembedded.org> wrote: > > Could you elaborate why? Some usecase or feature not supported by the python > > wrapper? Which one? > > You got this backwards Mikko. It's you who should elaborate why we > should continue using a custom replacement. > > As long as this change passes the autobuilder, it works as intended, > and needs no further justification. So changes without justification are fine then? I'm not against this change, I would just like to understand why it's needed or proposed right now. A reason could be just to reduce custom patches in oe-core. That would tell me that no actual usecase is currently broken, apart from ukify.py and secureboot for which I've sent a patch separately and which was seen in meta-arm side testing. Cheers, -Mikko
On Mon, 3 Feb 2025 at 12:27, Mikko Rapeli <mikko.rapeli@linaro.org> wrote: > So changes without justification are fine then? I'm not against this change, > I would just like to understand why it's needed or proposed right now. A reason > could be just to reduce custom patches in oe-core. That would tell me that no > actual usecase is currently broken, apart from ukify.py and secureboot for which > I've sent a patch separately and which was seen in meta-arm side testing. Yes, reducing the amount of custom code is sufficient justification in itself. If you need it explicitly spelled out, there you go. My favourite patches are those that remove code. We need more of these, and I always welcome them. Alex
Hi, On Mon, Feb 03, 2025 at 12:32:10PM +0100, Alexander Kanavin wrote: > On Mon, 3 Feb 2025 at 12:27, Mikko Rapeli <mikko.rapeli@linaro.org> wrote: > > So changes without justification are fine then? I'm not against this change, > > I would just like to understand why it's needed or proposed right now. A reason > > could be just to reduce custom patches in oe-core. That would tell me that no > > actual usecase is currently broken, apart from ukify.py and secureboot for which > > I've sent a patch separately and which was seen in meta-arm side testing. > > Yes, reducing the amount of custom code is sufficient justification in > itself. If you need it explicitly spelled out, there you go. I would like these to be explicitly written in commit messages. It is hard to guess the reasons even when I'm actively following mailing lists and irc, and even harder much later when all I have is git log and a bisect pointing to specific commit or its cherry-pick on a stable branch. Cheers, -Mikko
diff --git a/meta/recipes-core/systemd/systemd-systemctl-native.bb b/meta/recipes-core/systemd/systemd-systemctl-native.bb index ffa024caef..57bb1ab830 100644 --- a/meta/recipes-core/systemd/systemd-systemctl-native.bb +++ b/meta/recipes-core/systemd/systemd-systemctl-native.bb @@ -1,17 +1,13 @@ -SUMMARY = "Wrapper for enabling systemd services" +SUMMARY = "Systemctl executable from systemd" -LICENSE = "MIT" -LIC_FILES_CHKSUM = "file://${COREBASE}/meta/COPYING.MIT;md5=3da9cfbcb788c80a0384361b4de20420" +require systemd.inc +DEPENDS = "gperf-native libcap-native util-linux-native python3-jinja2-native" -inherit native +inherit pkgconfig meson native -SRC_URI = "file://systemctl" +SRC_URI = "file://0001-meson-add-install-tag-for-systemctl.patch" -S = "${WORKDIR}/sources" -UNPACKDIR = "${S}" - -do_install() { - install -d ${D}${bindir} - install -m 0755 ${S}/systemctl ${D}${bindir} -} +MESON_TARGET = "systemctl:executable" +MESON_INSTALL_TAGS = "systemctl" +EXTRA_OEMESON:append = " -Dlink-systemctl-shared=false" diff --git a/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch b/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch new file mode 100644 index 0000000000..a4b3e5a47e --- /dev/null +++ b/meta/recipes-core/systemd/systemd-systemctl/0001-meson-add-install-tag-for-systemctl.patch @@ -0,0 +1,24 @@ +From fe2eb1dde51648b0e5825766e2f27a859de8521c Mon Sep 17 00:00:00 2001 +From: Vyacheslav Yurkov <uvv.mail@gmail.com> +Date: Sun, 2 Feb 2025 10:13:38 +0100 +Subject: [PATCH] meson: add install tag for systemctl + +Upstream-Status: Submitted + +Signed-off-by: Vyacheslav Yurkov <uvv.mail@gmail.com> +--- + src/systemctl/meson.build | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/src/systemctl/meson.build b/src/systemctl/meson.build +index 88f73bf502..46218dc8ef 100644 +--- a/src/systemctl/meson.build ++++ b/src/systemctl/meson.build +@@ -61,6 +61,7 @@ executables += [ + libzstd_cflags, + threads, + ], ++ 'install_tag' : 'systemctl', + }, + fuzz_template + { + 'sources' : [ diff --git a/meta/recipes-core/systemd/systemd-systemctl/systemctl b/meta/recipes-core/systemd/systemd-systemctl/systemctl deleted file mode 100755 index 81c246a5b2..0000000000 --- a/meta/recipes-core/systemd/systemd-systemctl/systemctl +++ /dev/null @@ -1,366 +0,0 @@ -#!/usr/bin/env python3 -"""systemctl: subset of systemctl used for image construction - -Mask/preset systemd units -""" - -import argparse -import fnmatch -import os -import re -import sys - -from collections import namedtuple -from itertools import chain -from pathlib import Path - -version = 1.0 - -ROOT = Path("/") -SYSCONFDIR = Path("etc") -BASE_LIBDIR = Path("lib") -LIBDIR = Path("usr", "lib") - -locations = list() - - -class SystemdFile(): - """Class representing a single systemd configuration file""" - - _clearable_keys = ['WantedBy'] - - def __init__(self, root, path, instance_unit_name, unit_type): - self.sections = dict() - self._parse(root, path) - dirname = os.path.basename(path.name) + ".d" - for location in locations: - files = (root / location / unit_type / dirname).glob("*.conf") - if instance_unit_name: - inst_dirname = instance_unit_name + ".d" - files = chain(files, (root / location / unit_type / inst_dirname).glob("*.conf")) - for path2 in sorted(files): - self._parse(root, path2) - - def _parse(self, root, path): - """Parse a systemd syntax configuration file - - Args: - path: A pathlib.Path object pointing to the file - - """ - skip_re = re.compile(r"^\s*([#;]|$)") - section_re = re.compile(r"^\s*\[(?P<section>.*)\]") - kv_re = re.compile(r"^\s*(?P<key>[^\s]+)\s*=\s*(?P<value>.*)") - section = None - - if path.is_symlink(): - try: - path.resolve() - except FileNotFoundError: - # broken symlink, try relative to root - path = root / Path(os.readlink(str(path))).relative_to(ROOT) - - with path.open() as f: - for line in f: - if skip_re.match(line): - continue - - line = line.strip() - m = section_re.match(line) - if m: - if m.group('section') not in self.sections: - section = dict() - self.sections[m.group('section')] = section - else: - section = self.sections[m.group('section')] - continue - - while line.endswith("\\"): - line += f.readline().rstrip("\n") - - m = kv_re.match(line) - k = m.group('key') - v = m.group('value') - if k not in section: - section[k] = list() - - # If we come across a "key=" line for a "clearable key", then - # forget all preceding assignments. This works because we are - # processing files in correct parse order. - if k in self._clearable_keys and not v: - del section[k] - continue - - section[k].extend(v.split()) - - def get(self, section, prop): - """Get a property from section - - Args: - section: Section to retrieve property from - prop: Property to retrieve - - Returns: - List representing all properties of type prop in section. - - Raises: - KeyError: if ``section`` or ``prop`` not found - """ - return self.sections[section][prop] - - -class Presets(): - """Class representing all systemd presets""" - def __init__(self, scope, root): - self.directives = list() - self._collect_presets(scope, root) - - def _parse_presets(self, presets): - """Parse presets out of a set of preset files""" - skip_re = re.compile(r"^\s*([#;]|$)") - directive_re = re.compile(r"^\s*(?P<action>enable|disable)\s+(?P<unit_name>(.+))") - - Directive = namedtuple("Directive", "action unit_name") - for preset in presets: - with preset.open() as f: - for line in f: - m = directive_re.match(line) - if m: - directive = Directive(action=m.group('action'), - unit_name=m.group('unit_name')) - self.directives.append(directive) - elif skip_re.match(line): - pass - else: - sys.exit("Unparsed preset line in {}".format(preset)) - - def _collect_presets(self, scope, root): - """Collect list of preset files""" - presets = dict() - for location in locations: - paths = (root / location / scope).glob("*.preset") - for path in paths: - # earlier names override later ones - if path.name not in presets: - presets[path.name] = path - - self._parse_presets([v for k, v in sorted(presets.items())]) - - def state(self, unit_name): - """Return state of preset for unit_name - - Args: - presets: set of presets - unit_name: name of the unit - - Returns: - None: no matching preset - `enable`: unit_name is enabled - `disable`: unit_name is disabled - """ - for directive in self.directives: - if fnmatch.fnmatch(unit_name, directive.unit_name): - return directive.action - - return None - - -def add_link(path, target): - try: - path.parent.mkdir(parents=True) - except FileExistsError: - pass - if not path.is_symlink(): - print("ln -s {} {}".format(target, path)) - path.symlink_to(target) - - -class SystemdUnitNotFoundError(Exception): - def __init__(self, path, unit): - self.path = path - self.unit = unit - - -class SystemdUnit(): - def __init__(self, root, unit, unit_type): - self.root = root - self.unit = unit - self.unit_type = unit_type - self.config = None - - def _path_for_unit(self, unit): - for location in locations: - path = self.root / location / self.unit_type / unit - if path.exists() or path.is_symlink(): - return path - - raise SystemdUnitNotFoundError(self.root, unit) - - def _process_deps(self, config, service, location, prop, dirstem, instance): - systemdir = self.root / SYSCONFDIR / "systemd" / self.unit_type - - target = ROOT / location.relative_to(self.root) - try: - for dependent in config.get('Install', prop): - # expand any %i to instance (ignoring escape sequence %%) - dependent = re.sub("([^%](%%)*)%i", "\\g<1>{}".format(instance), dependent) - wants = systemdir / "{}.{}".format(dependent, dirstem) / service - add_link(wants, target) - - except KeyError: - pass - - def enable(self, units_enabled=[]): - # if we're enabling an instance, first extract the actual instance - # then figure out what the template unit is - template = re.match(r"[^@]+@(?P<instance>[^\.]*)\.", self.unit) - instance_unit_name = None - if template: - instance = template.group('instance') - if instance != "": - instance_unit_name = self.unit - unit = re.sub(r"@[^\.]*\.", "@.", self.unit, 1) - else: - instance = None - unit = self.unit - - path = self._path_for_unit(unit) - - if path.is_symlink(): - # ignore aliases - return - - config = SystemdFile(self.root, path, instance_unit_name, self.unit_type) - if instance == "": - try: - default_instance = config.get('Install', 'DefaultInstance')[0] - except KeyError: - # no default instance, so nothing to enable - return - - service = self.unit.replace("@.", - "@{}.".format(default_instance)) - else: - service = self.unit - - self._process_deps(config, service, path, 'WantedBy', 'wants', instance) - self._process_deps(config, service, path, 'RequiredBy', 'requires', instance) - - try: - for also in config.get('Install', 'Also'): - try: - units_enabled.append(unit) - if also not in units_enabled: - SystemdUnit(self.root, also, self.unit_type).enable(units_enabled) - except SystemdUnitNotFoundError as e: - sys.exit("Error: Systemctl also enable issue with %s (%s)" % (service, e.unit)) - - except KeyError: - pass - - systemdir = self.root / SYSCONFDIR / "systemd" / self.unit_type - target = ROOT / path.relative_to(self.root) - try: - for dest in config.get('Install', 'Alias'): - alias = systemdir / dest - add_link(alias, target) - - except KeyError: - pass - - def mask(self): - systemdir = self.root / SYSCONFDIR / "systemd" / self.unit_type - add_link(systemdir / self.unit, "/dev/null") - - -def collect_services(root, unit_type): - """Collect list of service files""" - services = set() - for location in locations: - paths = (root / location / unit_type).glob("*") - for path in paths: - if path.is_dir(): - continue - services.add(path.name) - - return services - - -def preset_all(root, unit_type): - presets = Presets('{}-preset'.format(unit_type), root) - services = collect_services(root, unit_type) - - for service in services: - state = presets.state(service) - - if state == "enable" or state is None: - try: - SystemdUnit(root, service, unit_type).enable() - except SystemdUnitNotFoundError: - sys.exit("Error: Systemctl preset_all issue in %s" % service) - - # If we populate the systemd links we also create /etc/machine-id, which - # allows systemd to boot with the filesystem read-only before generating - # a real value and then committing it back. - # - # For the stateless configuration, where /etc is generated at runtime - # (for example on a tmpfs), this script shouldn't run at all and we - # allow systemd to completely populate /etc. - (root / SYSCONFDIR / "machine-id").touch() - - -def main(): - if sys.version_info < (3, 4, 0): - sys.exit("Python 3.4 or greater is required") - - parser = argparse.ArgumentParser() - parser.add_argument('command', nargs='?', choices=['enable', 'mask', - 'preset-all']) - parser.add_argument('service', nargs=argparse.REMAINDER) - parser.add_argument('--root') - parser.add_argument('--preset-mode', - choices=['full', 'enable-only', 'disable-only'], - default='full') - parser.add_argument('--global', dest="opt_global", action="store_true", default=False) - - args = parser.parse_args() - - root = Path(args.root) if args.root else ROOT - - locations.append(SYSCONFDIR / "systemd") - # Handle the usrmerge case by ignoring /lib when it's a symlink - if not (root / BASE_LIBDIR).is_symlink(): - locations.append(BASE_LIBDIR / "systemd") - locations.append(LIBDIR / "systemd") - - command = args.command - if not command: - parser.print_help() - return 0 - - unit_type = "user" if args.opt_global else "system" - - if command == "mask": - for service in args.service: - try: - SystemdUnit(root, service, unit_type).mask() - except SystemdUnitNotFoundError as e: - sys.exit("Error: Systemctl main mask issue in %s (%s)" % (service, e.unit)) - elif command == "enable": - for service in args.service: - try: - SystemdUnit(root, service, unit_type).enable() - except SystemdUnitNotFoundError as e: - sys.exit("Error: Systemctl main enable issue in %s (%s)" % (service, e.unit)) - elif command == "preset-all": - if len(args.service) != 0: - sys.exit("Too many arguments.") - if args.preset_mode != "enable-only": - sys.exit("Only enable-only is supported as preset-mode.") - preset_all(root, unit_type) - else: - raise RuntimeError() - - -if __name__ == '__main__': - main()