GNOME Bugzilla – Bug 788707
Enable elogind support to substitute systemd-login
Last modified: 2017-11-01 09:14:23 UTC
Created attachment 361176 [details] [review] Enable elogind-support in gvfs-1.32.1 Hi everybody, for a while now we are testing using gvfs with elogind (https://github.com/elogind/elogind) instead of systemd-login. ( See: https://bugs.gentoo.org/599482 ) This is useful for users wanting to have a different init running their systems than systemd, but do want to use features depending on systemd-login as the session tracker. The following patch only adds the needed pieces to the build system. For a while now, elogind installs its headers in a way that software does no longer need to distinguish between system/sd-login.h and elogind/sd-login.h. As elogind actually *is* systemd-login, only extracted to be a standalone package, nothing else has to be changed. Please take a look at the attached patch. Thanks in advance Sven
Review of attachment 361176 [details] [review]: Thanks for your contribution, why not... However, can you please provide the patch using the "git format-patch"? https://wiki.gnome.org/Git/Developers#Contributing_patches ::: configure.ac @@ +317,3 @@ +dnl ********************************** + +AC_ARG_ENABLE([libelogind], [AS_HELP_STRING([--enable-libsystemd-login],[Use libelogind instead of libsystemd-login])]) "--enable-libsystemd-login" is a typo, however, it is enabled by default, so we should mention --disable-libelogind instead...
(In reply to Ondrej Holy from comment #1) > However, can you please provide the patch using the "git format-patch"? > https://wiki.gnome.org/Git/Developers#Contributing_patches Will do. It means I have to clone the git repo first, I have patched based on the tarball when merging gvfs on gentoo. But if a git format-patch is more convenient, why not. > [AS_HELP_STRING([--enable-libsystemd-login],[Use libelogind instead of > libsystemd-login])]) > > "--enable-libsystemd-login" is a typo, however, it is enabled by default, so > we should mention --disable-libelogind instead... Yes, that is an embarrassing typo. Dammit... However, I just wanted to argue, that elogind should be "opt-in". But gvfs uses systemd-login by default, and if elogind is installed, systemd-login is not, so the default should only get moved to libelogind to keep consistent with the defaults, right?
Created attachment 361238 [details] [review] Enable elogind-support in gvfs-1.32.1 (git format-patch version) I have fixed the typo and have created a git formatted patch. As far as I could test it, everything is working fine. The only real difference to the former patch is, that if systemd-login is detected and not disabled, configure won't go looking for elogind. (Although it makes absolutely no sense to use elogind on a system on which systemd runs the show... Better be safe than to risk weird bug reports...)
Review of attachment 361238 [details] [review]: Thanks for update, looks ok now. Just one more question, isn't also systemd-tmpfiles provided by elogind, or similar project? Please see USE_LIBSYSTEMD_LOGIN in client/Makefile.am.
Created attachment 361626 [details] [review] Enable elogind support in meson meson build system support is in the works in bug 786149[0]. Just in case this patch and the meson build port are accepted, this patch also adds support for `libelogind` in the meson build port. [0] https://bugzilla.gnome.org/show_bug.cgi?id=786149
Still I would like to know an answer for my Comment 4. Because I see some tmpfiles occurrences when grepping code of elogind...
Review of attachment 361626 [details] [review]: Looks ok, thanks!
(In reply to Ondrej Holy from comment #4) > Review of attachment 361238 [details] [review] [review]: > > Thanks for update, looks ok now. Just one more question, isn't also > systemd-tmpfiles provided by elogind, or similar project? Please see > USE_LIBSYSTEMD_LOGIN in client/Makefile.am. Sorry for the late answer. I've got a new job, and the first two weeks are important and exhausting. No, elogind does not incorporate systemd-tmpfiles. Maybe https://github.com/openrc/opentmpfiles would be an option?
Thanks for the reply. Ok, then it would be nice to add support also for this stuff, however, we are about removing autotools soon (bug 786149), so it would be nice to prepare the patch for meson... :-) We are also about removing of "auto" options by the meson port, so I am thinking about to add only one option (e.g. --enable-logind) instead of (--enable-libsystemd-login and --enable-elogind) and fail only if none of them is available... And also we should add a separate option for tmpfilesd and do not reuse the one for logind, --enable-tmpfilesd? Inigo?
Created attachment 362252 [details] [review] Enable elogind support in meson I have removed both options and created a new one called `enable-logind`, which tries `logind` in the following order: 1) Checks if `libsystemd` exists. 2) Checks if `libsystemd-login` with version '>= 44' exists. 3) Checks if `libelogind` with version '>= 229' exists. If any of them is found, a guard called `HAVE_LOGIND` is created. > And also we should add a separate option for tmpfilesd and do not reuse the > one for logind, --enable-tmpfilesd? I don't get this. I've seen the following in the `client/Makefile.am` file: > # FIXME: hardcoded path (systemd doesn't use lib64) > tmpfilesddir = $(prefix)/lib/tmpfiles.d > tmpfilesd_DATA = gvfsd-fuse-tmpfiles.conf How does `opentmptfiles` affect to this? Does it envolve anything else?
Review of attachment 362252 [details] [review]: That's exactly what I have in my mind, thanks. But we have to do something with enable_libsystemd_login in client/meson.build also. I would use --enable-tmpfilesd and check for systemd, or opentmpfiles and add "if enable_tmpfilesd" in client/meson.build consequently... ::: meson.build @@ +377,3 @@ + logind_dep = dependency('libsystemd-login', version: '>= 44', required: false) + if not logind_dep.found() + logind_dep = dependency('libelogind', version: '>= 229') nitpick: Just this maybe has one slightly confusing side-effect, that it fails with an error that libelogind was not found in case of a system without logind support. But it should rather say something like that none of libsystemd-logind and libelogind was not found. But it is probably not possible to check for multiple dependencies at once with meson. Maybe assert() would help here, but it is also not optimal. So probably ok :-)
Created attachment 362336 [details] [review] build: Enable elogind support in meson Rebased on top of patches from Bug 786149.
Comment on attachment 361238 [details] [review] Enable elogind-support in gvfs-1.32.1 (git format-patch version) Marking this as obsolete because we are going to switch to meson.
Created attachment 362337 [details] [review] build: Enable elogind support in meson Fixed wrong if and wrong ifdef...
Created attachment 362338 [details] [review] meson: Add tmpfilesd support in meson This adds --enable-tmpfilesd and it should also work with standalone opentmpfiles, however, the opentmpfiles detection was not tested.
Inigo, can you please take a look at those?
Review of attachment 362338 [details] [review]: Hmmm, as I am looking at it more deeply... our tmpfiles "integration" is not actually a feature, but workaround for some warnings from systemd-tmpfiles process. So maybe it is not worth to add explicit --enable-tmpfiles option and we should rather silently install that file if systemd is detected (as it is now)... See comment in: https://git.gnome.org/browse/gvfs/tree/client/gvfsd-fuse-tmpfiles.conf
(In reply to Ondrej Holy from comment #11) > ::: meson.build > @@ +377,3 @@ > + logind_dep = dependency('libsystemd-login', version: '>= 44', required: > false) > + if not logind_dep.found() > + logind_dep = dependency('libelogind', version: '>= 229') > > nitpick: Just this maybe has one slightly confusing side-effect, that it > fails with an error that libelogind was not found in case of a system > without logind support. But it should rather say something like that none of > libsystemd-logind and libelogind was not found. But it is probably not > possible to check for multiple dependencies at once with meson. Maybe > assert() would help here, but it is also not optimal. So probably ok :-) It makes a lot of sense what you say. At the moment checking for multiple dependencies at the same time is not possible, but the assertion would be the way to go. For example: enable_logind = get_option('enable-logind') if logind logind_dep = dependency('libsystemd', required: false) if not logind_dep.found() logind_dep = dependency('libsystemd-login', version: '>= 44', required: false) if not logind_dep.found() logind_dep = dependency('libelogind', version: '>= 229', required: false) endif endif assert(login_dep.found(), 'logind required but systemd or elogind have not been found.') endif (In reply to Ondrej Holy from comment #16) > Inigo, can you please take a look at those? Although the patches looks good to me, I'm sorry to say that I'm a bit lost regarding `tmpfilesd` support, as I'm not fully aware of all the effects it may have.
Comment on attachment 362338 [details] [review] meson: Add tmpfilesd support in meson This patch is not needed see https://bugzilla.gnome.org/show_bug.cgi?id=786149#c60.
Created attachment 362721 [details] [review] build: Enable elogind support in meson Rebased to master and added assert. Inigo? (Btw the jhbuild patch needs update for this...)
LGTM :) Regarding jhbuild, I've updated the patch locally. However I'm not sure how jhbuild works for this specific case (gentoo/openrc), so I've asked for it on the correspondant bug[0] [0] https://bugzilla.gnome.org/show_bug.cgi?id=789736#c4
Attachment 362721 [details] pushed as 1e9d338 - build: Enable elogind support in meson