Comment on attachment 132163 [details] [review]
build: Introduce ${runstatedir} and use it for the pid  file

Review of attachment 132163 [details] [review]:
-----------------------------------------------------------------

The commit message is great, except that it doesn’t say why introducing ${runstatedir} is a good thing. Is it so that distros can configure the PID file to be in /run?

Additionally, there seem to be some references to var/run in bus/CMakeLists.txt which might need attention? (I used `git grep "/run"`.)

::: configure.ac
@@ +1562,5 @@
>  if ! test -z "$with_system_socket"; then
>     DBUS_SYSTEM_SOCKET=$with_system_socket
>  else
> +   # We don't use runstatedir for this (yet?), because /var/run has been the
> +   # interoperable system bus socket for 10+ years.

Maybe add a reference to a bug (this bug?) to go with the ‘yet?’.

@@ +1602,5 @@
>  else
> +   # We don't use runstatedir for this (yet?), because /var/run is the
> +   # path that was traditionally used by pam_console and pam_foreground.
> +   # TODO: pam_console and pam_foreground were superseded by ConsoleKit,
> +   # which was superseded by systemd-logind, so maybe we should remove this

Similarly.

Comment on attachment 132163 [details] [review]
build: Introduce ${runstatedir} and use it for the pid  file

Review of attachment 132163 [details] [review]:
-----------------------------------------------------------------

> Additionally, there seem to be some references to var/run in bus/CMakeLists.txt

Specifically:
    install(DIRECTORY DESTINATION var/run/dbus)
and
#	$(mkinstalldirs) $(DESTDIR)/$(localstatedir)/run/dbus

(In reply to Philip Withnall from comment #1)
> The commit message is great, except that it doesn’t say why introducing
> ${runstatedir} is a good thing. Is it so that distros can configure the PID
> file to be in /run?

A little bit, but it's mainly so that I can use ${runstatedir} in Bug #101354 (buses that start as root, such as the system bus, will create their container servers in ${runstatedir}/dbus/containers/ instead of XDG_RUNTIME_DIR). I should have said so, though.

> Additionally, there seem to be some references to var/run in
> bus/CMakeLists.txt which might need attention? (I used `git grep "/run"`.)

The CMake build system is a bit of a second-class citizen, particularly on Unix.  It's mainly there for MSVC. As long as it *works* (so that I can verify that the CMake build system isn't totally broken), that's enough; it doesn't have to be perfect. In particular, the Autotools build system is recommended for system-wide (distro) installations on Unix.

As such, there's a lot more hard-coding in the CMake build system, and defining ${runstatedir} to be always ${prefix}/var/run under CMake seems acceptable.

> Maybe add a reference to a bug (this bug?) to go with the ‘yet?’.

Reasonable.

> Similarly.

Also reasonable.

(In reply to Philip Withnall from comment #1)
> > +   # We don't use runstatedir for this (yet?), because /var/run has been the
> > +   # interoperable system bus socket for 10+ years.
> 
> Maybe add a reference to a bug (this bug?) to go with the ‘yet?’.

Bug #101628 is that bug.

> @@ +1602,5 @@
> >  else
> > +   # We don't use runstatedir for this (yet?), because /var/run is the
> > +   # path that was traditionally used by pam_console and pam_foreground.
> > +   # TODO: pam_console and pam_foreground were superseded by ConsoleKit,
> > +   # which was superseded by systemd-logind, so maybe we should remove this

This is Bug #101629.

Created attachment 132352 [details] [review]
build: Introduce ${runstatedir} and use it for the pid file

By default ${runstatedir} is the same as ${localstatedir}/run, but many
Linux distributions configure it to be /run and mount a tmpfs in that
location. All other factors being equal, it is preferable to use /run
where available because it is guaranteed to be local, whereas traversing
/var might involve automounting a networked filesystem (even though
/var/run itself is very likely to be a tmpfs).

/run or /var/run is currently only used in a few places in dbus, but
I plan to make more use of it during the development of
<https://bugs.freedesktop.org/show_bug.cgi?id=100344>.

The pid file is not part of the API between dbus and other software
(other than distribution init scripts for dbus itself), so we do not
need to keep it strictly compatible; so it is OK to move it.

We do not yet use /run for the system bus socket, because that is
part of the API between D-Bus clients and servers, and has always been
"officially" /var/run/dbus/system_bus_socket.
<https://bugs.freedesktop.org/show_bug.cgi?id=101628> tracks the
possibility of changing that.

Similarly, we do not replace /var/run/console with /run/console, because
that path is part of the API between dbus-daemon and the obsolete PAM
modules pam_console and pam_foreground that used /var/run/console.
<https://bugs.freedesktop.org/show_bug.cgi?id=101629> tracks the possible
future removal of that code path.

In the CMake build system, the equivalent of ${runstatedir} remains
hard-coded to the equivalent of ${localstatedir}/run for simplicity. For
the sort of system-wide installations that would consider redefining
${runstatedir} to /run, the Autotools build system is strongly
recommended: in particular this is what Linux distributions are expected
to use.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101569

Comment on attachment 132352 [details] [review]
build: Introduce ${runstatedir} and use it for the pid file

Review of attachment 132352 [details] [review]:
-----------------------------------------------------------------

r+

Thanks! This narrowly missed the boat for 1.11.14 (it was about time we had a release, the diff since 1.11.12 was getting rather large) but is now merged for 1.11.16.