[PATCH libvirt-dbus 1/2] meson: generate systemd unit file for libvirt-dbus

[Pavel Hrdina]

On Mon, Aug 31, 2020 at 09:45:15AM +0200, Katerina Koukiou wrote:
> Signed-off-by: Katerina Koukiou <kkoukiou at redhat.com>
> ---
>  data/system/libvirt-dbus.service.in        | 21 +++++++++++++++
>  data/system/meson.build                    | 30 +++++++++++++++++-----
>  data/system/org.libvirt-systemd.service.in |  5 ++++
>  libvirt-dbus.spec.in                       |  4 ++-
>  meson.build                                | 12 +++++++++
>  meson_options.txt                          |  1 +
>  6 files changed, 66 insertions(+), 7 deletions(-)
>  create mode 100644 data/system/libvirt-dbus.service.in
>  create mode 100644 data/system/org.libvirt-systemd.service.in
> 
> diff --git data/system/libvirt-dbus.service.in data/system/libvirt-dbus.service.in
> new file mode 100644
> index 0000000..862a366
> --- /dev/null
> +++ data/system/libvirt-dbus.service.in
> @@ -0,0 +1,21 @@
> +#  SPDX-License-Identifier: LGPL-2.1+
> +#
> +#  This file is part of systemd.
> +#
> +#  systemd is free software; you can redistribute it and/or modify it
> +#  under the terms of the GNU Lesser General Public License as published by
> +#  the Free Software Foundation; either version 2.1 of the License, or
> +#  (at your option) any later version.

This doesn't look correct as you are contributing to libvirt-dbus, not
systemd.

> +
> +[Unit]
> +Description=Libvirt DBus Service
> +
> +[Service]
> +BusName=org.libvirt
> +DynamicUser=yes
> +User=@SYSTEM_USER@
> +Group=@SYSTEM_USER@
> +ExecStart=@sbindir@/libvirt-dbus --system
> +
> +[Install]
> +Alias=org.libvirt.service
> diff --git data/system/meson.build data/system/meson.build
> index 74f1949..67657d4 100644
> --- data/system/meson.build
> +++ data/system/meson.build
> @@ -1,9 +1,18 @@
> -configure_file(
> -    configuration: conf,
> -    input: 'org.libvirt.service.in',
> -    output: 'org.libvirt.service',
> -    install_dir: dbus_system_services_dir,
> -)
> +if init_script == 'systemd'
> +    configure_file(
> +        configuration: conf,
> +        input: 'org.libvirt-systemd.service.in',
> +        output: 'org.libvirt.service',
> +        install_dir: dbus_system_services_dir,
> +    )
> +else
> +    configure_file(
> +        configuration: conf,
> +        input: 'org.libvirt.service.in',
> +        output: 'org.libvirt.service',
> +        install_dir: dbus_system_services_dir,
> +    )
> +endif

Here you can use this to reduce code duplication:

if init_script == 'systemd'
    dbus_service_in = 'org.libvirt-systemd.service.in'
else
    dbus_service_in = 'org.libvirt.service.in'
endif

configure_file(
    configuration: conf,
    input: dbus_service_in,
    output: 'org.libvirt.service',
    install_dir: dbus_system_services_dir,
)

>  configure_file(
>      configuration: conf,
>      input: 'org.libvirt.conf.in',
> @@ -16,3 +25,12 @@ configure_file(
>      output: 'libvirt-dbus.rules',
>      install_dir: polkit_rules_dir,
>  )
> +if init_script == 'systemd'
> +    systemd_system_unit_dir = systemd_dep.get_pkgconfig_variable('systemdsystemunitdir')
> +    configure_file(
> +        configuration: conf,
> +        input: 'libvirt-dbus.service.in',
> +        output: 'libvirt-dbus.service',
> +        install_dir: systemd_system_unit_dir,
> +    )
> +endif
> diff --git data/system/org.libvirt-systemd.service.in data/system/org.libvirt-systemd.service.in
> new file mode 100644
> index 0000000..ba260b2
> --- /dev/null
> +++ data/system/org.libvirt-systemd.service.in
> @@ -0,0 +1,5 @@
> +[D-BUS Service]
> +Name=org.libvirt
> +Exec=/bin/false
> +User=@SYSTEM_USER@
> +SystemdService=libvirt-dbus.service
> diff --git libvirt-dbus.spec.in libvirt-dbus.spec.in
> index 4e6ff85..8286609 100644
> --- libvirt-dbus.spec.in
> +++ libvirt-dbus.spec.in
> @@ -40,7 +40,8 @@ This package provides D-Bus API for libvirt
>  %autosetup
>  
>  %build
> -%meson
> +%meson \
> +           -Dinit_script=systemd

I would say 4 spaces are good enough as indentation.

>  %meson_build
>  
>  %install
> @@ -57,6 +58,7 @@ exit 0
>  %doc AUTHORS.rst NEWS.rst
>  %license COPYING
>  %{_sbindir}/libvirt-dbus
> +%{_unitdir}/libvirt-dbus.service
>  %{_datadir}/dbus-1/services/org.libvirt.service
>  %{_datadir}/dbus-1/system-services/org.libvirt.service
>  %{_datadir}/dbus-1/system.d/org.libvirt.conf
> diff --git meson.build meson.build
> index e765ed6..c34c07d 100644
> --- meson.build
> +++ meson.build
> @@ -12,6 +12,18 @@ project(
>  prefix = get_option('prefix')
>  datadir = prefix / get_option('datadir')
>  sbindir = prefix / get_option('sbindir')
> +if get_option('init_script') == 'check'
> +  if find_program('systemctl', required: false).found()
> +    init_script = 'systemd'
> +  else
> +    init_script = 'other'
> +  endif
> +else
> +  init_script = get_option('init_script')
> +endif

Inconsistent indentation, in libvirt-dbus we use 4 spaces.

> +if init_script == 'systemd'
> +    systemd_dep = dependency('systemd')
> +endif
>  
>  opt_dirs = [
>      'dbus_interfaces',
> diff --git meson_options.txt meson_options.txt
> index 36e8065..41d348f 100644
> --- meson_options.txt
> +++ meson_options.txt
> @@ -4,3 +4,4 @@ option('dbus_system_policies', type: 'string', value: 'dbus-1/system.d', descrip
>  option('dbus_interfaces', type: 'string', value: 'dbus-1/interfaces', description: 'D-Bus interfaces directory')
>  option('polkit_rules', type: 'string', value: 'polkit-1/rules.d', description: 'polkit rules directory')
>  option('system_user', type: 'string', value: 'libvirtdbus', description: 'username to run system instance as')
> +option('init_script', type: 'combo', choices: ['systemd', 'other', 'check'], value: 'check', description: 'Style of init script to install')
> -- 
> 2.26.2
> 


Overall looks good.

One note though, libvirt-dbus switched to gitlab and uses merge request
workflow. So would you please create a merge request here:

https://gitlab.com/libvirt/libvirt-dbus

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200831/96a16d4e/attachment-0001.sig>