After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788707 - Enable elogind support to substitute systemd-login
Enable elogind support to substitute systemd-login
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: build
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2017-10-09 11:20 UTC by Sven Eden
Modified: 2017-11-01 09:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Enable elogind-support in gvfs-1.32.1 (2.73 KB, patch)
2017-10-09 11:20 UTC, Sven Eden
none Details | Review
Enable elogind-support in gvfs-1.32.1 (git format-patch version) (4.24 KB, patch)
2017-10-10 10:24 UTC, Sven Eden
reviewed Details | Review
Enable elogind support in meson (3.42 KB, patch)
2017-10-15 18:00 UTC, Iñigo Martínez
none Details | Review
Enable elogind support in meson (3.74 KB, patch)
2017-10-25 11:38 UTC, Iñigo Martínez
none Details | Review
build: Enable elogind support in meson (2.88 KB, patch)
2017-10-26 11:21 UTC, Ondrej Holy
none Details | Review
build: Enable elogind support in meson (3.44 KB, patch)
2017-10-26 11:36 UTC, Ondrej Holy
none Details | Review
meson: Add tmpfilesd support in meson (2.39 KB, patch)
2017-10-26 11:36 UTC, Ondrej Holy
none Details | Review
build: Enable elogind support in meson (3.29 KB, patch)
2017-11-01 07:32 UTC, Ondrej Holy
committed Details | Review

Description Sven Eden 2017-10-09 11:20:36 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
Comment 1 Ondrej Holy 2017-10-09 14:10:47 UTC
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...
Comment 2 Sven Eden 2017-10-09 16:26:07 UTC
(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?
Comment 3 Sven Eden 2017-10-10 10:24:06 UTC
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...)
Comment 4 Ondrej Holy 2017-10-10 13:25:36 UTC
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.
Comment 5 Iñigo Martínez 2017-10-15 18:00:17 UTC
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
Comment 6 Ondrej Holy 2017-10-20 09:54:00 UTC
Still I would like to know an answer for my Comment 4. Because I see some tmpfiles occurrences when grepping code of elogind...
Comment 7 Ondrej Holy 2017-10-20 09:54:15 UTC
Review of attachment 361626 [details] [review]:

Looks ok, thanks!
Comment 8 Sven Eden 2017-10-22 21:08:35 UTC
(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?
Comment 9 Ondrej Holy 2017-10-24 12:34:45 UTC
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?
Comment 10 Iñigo Martínez 2017-10-25 11:38:51 UTC
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?
Comment 11 Ondrej Holy 2017-10-26 08:40:49 UTC
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 :-)
Comment 12 Ondrej Holy 2017-10-26 11:21:54 UTC
Created attachment 362336 [details] [review]
build: Enable elogind support in meson

Rebased on top of patches from Bug 786149.
Comment 13 Ondrej Holy 2017-10-26 11:28:45 UTC
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.
Comment 14 Ondrej Holy 2017-10-26 11:36:00 UTC
Created attachment 362337 [details] [review]
build: Enable elogind support in meson

Fixed wrong if and wrong ifdef...
Comment 15 Ondrej Holy 2017-10-26 11:36:22 UTC
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.
Comment 16 Ondrej Holy 2017-10-26 11:39:31 UTC
Inigo, can you please take a look at those?
Comment 17 Ondrej Holy 2017-10-26 11:56:43 UTC
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
Comment 18 Iñigo Martínez 2017-10-28 09:43:13 UTC
(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 19 Ondrej Holy 2017-10-31 17:09:17 UTC
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.
Comment 20 Ondrej Holy 2017-11-01 07:32:50 UTC
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...)
Comment 21 Iñigo Martínez 2017-11-01 08:49:54 UTC
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
Comment 22 Ondrej Holy 2017-11-01 09:14:19 UTC
Attachment 362721 [details] pushed as 1e9d338 - build: Enable elogind support in meson