RE: [OE-core][PATCH 2/3] systemd.bbclass: add non-recursive service packaging

[Peter Kjellerstedt <peter.kjellerstedt@axis.com>]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Peter Marko
> Sent: den 8 februari 2023 08:13
> To: openembedded-core@lists.openembedded.org
> Cc: Peter Marko <peter.marko@siemens.com>
> Subject: [OE-core][PATCH 2/3] systemd.bbclass: add non-recursive service packaging
> 
> When service is split to separate package, it will take
> all services it depends on. It does not matter is the dependency

Change "is" to "if".

> is strong or week or if there is rdepends/rrecommends which would

Change "week" to "weak".

> be the proper way to pull it.
> 
> New variable SYSTEMD_PACKAGES_DONT_RECURSE allows to
> skip this recursion for packages which are extracted to a package.
> It is mostly useful for catch-all main package and splitting

Change "catch-all main package" to either "a catch-all main package" 
or "catch-all main packages".

> additional packages with PACKAGE_BEFORE_PN.
> 
> Signed-off-by: Peter Marko <peter.marko@siemens.com>
> ---
>  documentation/ref-manual/variables.rst | 10 ++++++++++
>  meta/classes-recipe/systemd.bbclass    | 15 ++++++++-------
>  2 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/documentation/ref-manual/variables.rst b/documentation/ref-manual/variables.rst
> index 725f5c54cc..910b99aed2 100644
> --- a/documentation/ref-manual/variables.rst
> +++ b/documentation/ref-manual/variables.rst
> @@ -8271,6 +8271,16 @@ system and gives an overview of their function and contents.
>        :term:`SYSTEMD_PACKAGES`. Overrides not included in :term:`SYSTEMD_PACKAGES`
>        will be silently ignored.
> 
> +   :term:`SYSTEMD_PACKAGES_DONT_RECURSE`

Negative variable names should be avoided when possible. I suggest 
changing this to "SYSTEMD_PACKAGES_RECURSE". See below for how the 
implementation should be changed.

> +      By default service files declared in :term:`SYSTEMD_SERVICE` are scanned
> +      and all related service files are added to parsed package recursively.

Change "parsed" to "the parsed".

> +
> +      It allows more readable and future-proof recipes, however it does not work well
> +      when services are split to separate packages. This variable prevents this behavior.

Change "prevents" to "can be used to prevent".

> +      Here is an example from systemd recipe::
> +
> +         SYSTEMD_PACKAGES_DONT_RECURSE:${PN}-networkd = "1"
> +
>     :term:`SYSVINIT_ENABLED_GETTYS`
>        When using
>        :ref:`SysVinit <dev-manual/new-recipe:enabling system services>`,
> diff --git a/meta/classes-recipe/systemd.bbclass b/meta/classes-recipe/systemd.bbclass
> index f9c92e6c2a..c8cee482fe 100644
> --- a/meta/classes-recipe/systemd.bbclass
> +++ b/meta/classes-recipe/systemd.bbclass
> @@ -124,19 +124,19 @@ python systemd_populate_packages() {
>          return appended
> 
>      # Add systemd files to FILES:*-systemd, parse for Also= and follow recursive
> -    def systemd_add_files_and_parse(pkg_systemd, path, service, keys):
> +    def systemd_add_files_and_parse(pkg_systemd, path, service, keys, recurse):
>          # avoid infinite recursion
> -        if systemd_append_file(pkg_systemd, oe.path.join(path, service)):
> +        if systemd_append_file(pkg_systemd, oe.path.join(path, service)) and recurse:
>              fullpath = oe.path.join(d.getVar("D"), path, service)
>              if service.find('.service') != -1:
>                  # for *.service add *@.service
>                  service_base = service.replace('.service', '')
> -                systemd_add_files_and_parse(pkg_systemd, path, service_base + '@.service', keys)
> +                systemd_add_files_and_parse(pkg_systemd, path, service_base + '@.service', keys, recurse)
>              if service.find('.socket') != -1:
>                  # for *.socket add *.service and *@.service
>                  service_base = service.replace('.socket', '')
> -                systemd_add_files_and_parse(pkg_systemd, path, service_base + '.service', keys)
> -                systemd_add_files_and_parse(pkg_systemd, path, service_base + '@.service', keys)
> +                systemd_add_files_and_parse(pkg_systemd, path, service_base + '.service', keys, recurse)
> +                systemd_add_files_and_parse(pkg_systemd, path, service_base + '@.service', keys, recurse)
>              for key in keys.split():
>                  # recurse all dependencies found in keys ('Also';'Conflicts';..) and add to files
>                  cmd = "grep %s %s | sed 's,%s=,,g' | tr ',' '\\n'" % (key, shlex.quote(fullpath), key)
> @@ -144,7 +144,7 @@ python systemd_populate_packages() {
>                  line = pipe.readline()
>                  while line:
>                      line = line.replace('\n', '')
> -                    systemd_add_files_and_parse(pkg_systemd, path, line, keys)
> +                    systemd_add_files_and_parse(pkg_systemd, path, line, keys, recurse)
>                      line = pipe.readline()
>                  pipe.close()
> 
> @@ -157,6 +157,7 @@ python systemd_populate_packages() {
>          keys = 'Also'
>          # scan for all in SYSTEMD_SERVICE[]
>          for pkg_systemd in systemd_packages.split():
> +            recurse = False if d.getVar('SYSTEMD_PACKAGES_DONT_RECURSE:' + pkg_systemd) else True

Change to:

            recurse = not bb.utils.to_boolean(d.getVar('SYSTEMD_PACKAGES_DONT_RECURSE:' + pkg_systemd))

Or preferably, to avoid the negative variable name, change to:

            recurse = bb.utils.to_boolean(d.getVar('SYSTEMD_PACKAGES_RECURSE:' + pkg_systemd), True)

You should probably also change "d.getVar('SYSTEMD_PACKAGES_RECURSE:' + pkg_systemd)" 
to "get_package_var(d, 'SYSTEMD_PACKAGES_RECURSE', pkg_systemd)", which 
would allow setting SYSTEMD_PACKAGES_RECURSE = "0" in the recipe to disable 
recursing for all packages. In that case, also adapt the documentation 
accordingly.

>              for service in get_package_var(d, 'SYSTEMD_SERVICE', pkg_systemd).split():
>                  path_found = ''
> 
> @@ -178,7 +179,7 @@ python systemd_populate_packages() {
>                              break
> 
>                  if path_found != '':
> -                    systemd_add_files_and_parse(pkg_systemd, path_found, service, keys)
> +                    systemd_add_files_and_parse(pkg_systemd, path_found, service, keys, recurse)
>                  else:
>                      bb.fatal("Didn't find service unit '{0}', specified in SYSTEMD_SERVICE:{1}. {2}".format(
>                          service, pkg_systemd, "Also looked for service unit '{0}'.".format(base) if base is not None else ""))
> --
> 2.30.2

//Peter