Re: [Buildroot] [PATCH v6 1/4] package/dbus-broker: new package

[Arnout Vandecappelle <arnout@mind.be>]

  Norbert, Yann,

  As usual, apologies for taking so long to review this. It's because it's a bit 
complicated. I actually tried to look at it just after our hackaton in January 
but it was too complicated to quickly get my head around.

On 09/01/2022 23:16, Norbert Lange wrote:
> dbus-broker is an alternate implementation of a dbus dameon. It can be
> used as a drop-in replacement for the system bus daemon, as well as the
> session bus daemon.
> 
> dbus-broker is (basically, and as far as we're concerned in Buildroot)
> split in two components:
> 
>    - the actual message bus daemon, that relays messages across clients
> 
>    - a launcher, which is responsible for setting various aspects of the
>      bus, like setting the policy et al. and opening the socket(s) the
>      message bus daemon will have to listen on...
> 
> The launcher can only be used in a systemd setup (it makes heavy use of
> systemd facilities), while the message bus is generic. However, the
> message bus daemon is useless without a launcher. There does not exist a
> non-systemd launcher, which makes dbus-broker actually a systemd-only
> package; this can be revisited when/if a non-systemd launcher appears.
> 
> Note, however, that libdbus is not provided by dbus-borker. People who
> want to use dbus-broker as the bus daemon, and need libdbus, will have
> to enable both, see below.
> 
> There are two cases:
> 
>   1. original dbus disabled
> 
>      Here, we install the config files and systemd socket activation
>      units; dbus-broker provides the system and sessions bus daemons.
> 
>      In this case, libdbus is not available.
> 
>   2. original dbus enabled
> 
>      In this case, we do not install the config files and systemd socket
>      activation units, or define a user: they all are provided by the
>      original dbus, and we piggy-back on those.
> 
>      In this situation, the default system and sessions message bus are
>      the original dbus, and libdbus is available; dbus-broker is not
>      enabled.
> 
>      So far, we believe it would be over-engineered to provide a way
>      to allow choosing the bus daemon in the configuraiton. However,
>      users may opt-in to use dbus-broker in a few ways:
>        - at build-time: by calling systemctl enable/disable from a
>          post-build script (preferred), or by providing drop-in units
>          or presets in an overlay (less preferred) or custom skeleton
>          (as a last resort),
>        - at runtime (on a RW filesystem): by calling systemctl
>          enable/disable

  I don't know... To me it seems logical that if both are enabled, that you use 
dbus-broker for the bus and only use original dbus for libdbus.

  So to me the proper approach seems to change dbus itself so the daemon is 
optional, i.e. only libdbus is installed if the daemon is not enabled, similar 
to e.g. libcurl. BR2_PACKAGE_DBUS_DAEMON should default to y because that's what 
most people want. We'd have to go through existing packages to check if they 
depend on libdbus or on the daemon - mostly it will be the first. If there is a 
package that really needs the daemon (I don't actually think so), then we might 
need to introduce a virtual package for the daemon. Regardless, dbus-broker 
should depend on !BR2_PACKAGE_DBUS_DAEMON.


> Note about the user: the path to the system bus socket is a so-called
> "well-known location": it is expected to be there, by spec. Moving it
> elsewhere is going to break existing programs. So, the user running the
> system bus daemon must be able to create that socket.
> 
> As we may have two packages providing a system bus daemon, they have to
> be both able to create the socket, and thus must both be able to write
> in the directory containing the socket. And since they can be switched
> at runtime, they must be running as the same user.
> 
> We can't just reference the original dbus user, so we duplicate the
> entry. What is important, is that the user be named 'dbus', as that's
> what we use in both cases.

  If it's a virtual package, then the user could be created by the virtual 
package itself.


> Finally, the licensing terms are pretty trivial for dbus-broker itself,
> but it makes use of third-party code that it inherits as git submodules
> (that are bundled in the release archive). 

  Ugh, bundling...

> Thus the licensing is a bit
> convoluted... The third-party codes claim to be licensed as "Apache-2.0
> and LGP-2.1+" in their AUTHORS files, but at the same time claim
> "**Apache-2.0** OR **LGPL-2.1-or-later**" in their README files. The
> individual source files (that are used) do not seem to have any
> licensing header to clarify the situation. So we represent the situation
> with "Apache-2.0 and/or LGPL-2.1+".

  That's rather f**d than convoluted :-)

> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> [yann.morin.1998@free.fr:
>    - don't select systemd; depend on it instead
>    - only install config files and systemd units without original dbus
>    - install a user to run the message bus as
>    - fix licensing info
>    - entirely reword and extend the commit log
>    - add myself to DEVELOPERS as well
> ]
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>

[snip]
> diff --git a/package/dbus-broker/Config.in b/package/dbus-broker/Config.in
> new file mode 100644
> index 0000000000..c7206232da
> --- /dev/null
> +++ b/package/dbus-broker/Config.in
> @@ -0,0 +1,23 @@
> +config BR2_PACKAGE_DBUS_BROKER
> +	bool "dbus-broker"
> +	depends on BR2_USE_MMU
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_17
> +	depends on BR2_PACKAGE_SYSTEMD
> +	select BR2_PACKAGE_EXPAT
> +	help
> +	  Linux D-Bus Message Broker.
> +
> +	  The dbus-broker project is an implementation of a message bus
> +	  as defined by the D-Bus specification. Its aim is to provide
> +	  high performance and reliability, while keeping compatibility
> +	  to the D-Bus reference implementation.
> +
> +	  It is exclusively written for Linux systems, and makes use of
> +	  many modern features provided by recent linux kernel releases.
> +
> +	  https://github.com/bus1/dbus-broker/wiki
> +
> +comment "dbusbroker needs systemd and a toolchain w/ threads"
> +	depends on BR2_USE_MMU
> +	depends on !BR2_TOOLCHAIN_HAS_THREADS || !BR2_PACKAGE_SYSTEMD

  It's a nitpick, but: I think since dbus-broker is so tightly integrated with 
systemd, there's no point really to show the comment if systemd is not enabled.

> diff --git a/package/dbus-broker/dbus-broker.hash b/package/dbus-broker/dbus-broker.hash
> new file mode 100644
> index 0000000000..26ebab7ac1
> --- /dev/null
> +++ b/package/dbus-broker/dbus-broker.hash
> @@ -0,0 +1,3 @@
> +# Locally calculated
> +sha256  4eca425db52b7ab1027153e93fea9b3f11759db9e93ffbf88759b73ddfb8026a  dbus-broker-29.tar.xz
> +sha256  3cda3630283eda0eab825abe5ac84d191248c6b3fe1c232a118124959b96c6a4  LICENSE
> diff --git a/package/dbus-broker/dbus-broker.mk b/package/dbus-broker/dbus-broker.mk
> new file mode 100644
> index 0000000000..547b79eb84
> --- /dev/null
> +++ b/package/dbus-broker/dbus-broker.mk
> @@ -0,0 +1,71 @@
> +################################################################################
> +#
> +# dbus-broker
> +#
> +################################################################################
> +
> +DBUS_BROKER_VERSION = 29

  By now we're at 31 already.

  The new version uses meson's wrapgit feature instead of submodules, but 
fortunately the release tarball still include them.

> +DBUS_BROKER_SOURCE = dbus-broker-$(DBUS_BROKER_VERSION).tar.xz
> +DBUS_BROKER_SITE = https://github.com/bus1/dbus-broker/releases/download/v$(DBUS_BROKER_VERSION)
> +
> +# For the third-party code, the licensing legla-info is inconsistent between
> +# the AUTHORS and README, so keep both
> +DBUS_BROKER_LICENSE = \
> +	Apache-2.0, \
> +	Apache-2.0 and/or LGPL-2.1+ (c-dvar, c-ini, c-list, c-rbtree, c-shquote, c-stdaux, c-utf8)
> +DBUS_BROKER_LICENSE_FILES = \
> +	LICENSE \
> +	subprojects/c-dvar/AUTHORS subprojects/c-dvar/README.md \
> +	subprojects/c-ini/AUTHORS subprojects/c-ini/README.md \
> +	subprojects/c-list/AUTHORS subprojects/c-list/README.md \
> +	subprojects/c-rbtree/AUTHORS subprojects/c-rbtree/README.md \
> +	subprojects/c-shquote/AUTHORS subprojects/c-shquote/README.md \
> +	subprojects/c-stdaux/AUTHORS subprojects/c-stdaux/README.md \
> +	subprojects/c-utf8/AUTHORS subprojects/c-utf8/README.md
> +
> +DBUS_BROKER_DEPENDENCIES = expat systemd
> +DBUS_BROKER_CONF_OPTS = -Dlauncher=true
> +
> +ifeq ($(BR2_PACKAGE_AUDIT),y)
> +DBUS_BROKER_DEPENDENCIES += audit
> +DBUS_BROKER_CONF_OPTS += -Daudit=true
> +else
> +DBUS_BROKER_CONF_OPTS += -Daudit=false
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LIBSELINUX),y)
> +DBUS_BROKER_DEPENDENCIES += libselinux
> +DBUS_BROKER_CONF_OPTS += -Dselinux=true
> +else
> +DBUS_BROKER_CONF_OPTS += -Dselinux=false
> +endif
> +
> +# We must be using the same user as the original dbus, so we can share
> +# the home directory and create a socket there. As a consequence, the
> +# username and groupname must be dbus:dbus, and they both need to have
> +# the same home.
> +define DBUS_BROKER_USERS
> +	dbus -1 dbus -1 * /run/dbus - dbus DBus messagebus user
> +endef
> +
> +# Only install units for system bus daemon socket if original dbus is not present
> +# Only install config and service files if original dbus is not present
> +#
> +# Note: BR2_COREUTILS_HOST_DEPENDENCY to be able to use ln --relative
> +ifeq ($(BR2_PACKAGE_DBUS),)
> +DBUS_BROKER_DEPENDENCIES += $(BR2_COREUTILS_HOST_DEPENDENCY)
> +
> +define DBUS_BROKER_INSTALL_INIT_SYSTEMD
> +	$(INSTALL) -D -m 0644 $(DBUS_BROKER_PKGDIR)/session.conf \
> +		$(TARGET_DIR)/usr/share/dbus-1/session.conf
> +	$(INSTALL) -D -m 0644 $(DBUS_BROKER_PKGDIR)/system.conf \
> +		$(TARGET_DIR)/usr/share/dbus-1/system.conf
> +	$(INSTALL) -D -m 0644 $(DBUS_BROKER_PKGDIR)/dbus.socket \
> +		$(TARGET_DIR)/usr/lib/systemd/system/dbus.socket

  I'm surprised that these files either isn't needed for original dbus, or that 
it's included in original dbus but not in dbus-broker...

> +	$(HOST_MAKE_ENV) ln -sf --relative \
> +		$(TARGET_DIR)/usr/lib/systemd/system/dbus.socket \
> +		$(TARGET_DIR)/usr/lib/systemd/system/sockets.target.wants/dbus.socket

  Is it really useful too include the coreutils dependency and HOST_MAKE_ENV, 
just to avoid having to put

	ln -sf ../dbus-socket \
		$(TARGET_DIR)/usr/lib/systemd/system/sockets.target.wants/dbus.socket


  I'm going to work more on and hopefully merge this series later today. I have 
some local changes already (spelling mistakes), so please don't resend just yet.

  Regards,
  Arnout


> +endef
> +endif # !BR2_PACKAGE_DBUS
> +
> +$(eval $(meson-package))

[snip]
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot