Bug 101251 - [PATCH] Add support for elogind session tracker to AccountsService
Summary: [PATCH] Add support for elogind session tracker to AccountsService
Status: RESOLVED FIXED
Alias: None
Product: accountsservice
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-31 12:32 UTC by Sven Eden
Modified: 2017-06-06 14:22 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add support for elogind session tracker (2.42 KB, patch)
2017-05-31 12:32 UTC, Sven Eden
Details | Splinter Review
Updated Patch to enable elogind support (2.70 KB, patch)
2017-06-01 11:46 UTC, Sven Eden
Details | Splinter Review
Updated Patch to enable elogind support - configure.ac only (3.22 KB, patch)
2017-06-06 07:38 UTC, Sven Eden
Details | Splinter Review
[FIXED] Updated Patch to enable elogind support - configure.ac only (3.22 KB, patch)
2017-06-06 08:13 UTC, Sven Eden
Details | Splinter Review

Description Sven Eden 2017-05-31 12:32:20 UTC
Created attachment 131598 [details] [review]
Add support for elogind session tracker

The elogind project (https://github.com/elogind/elogind) provides systemd-logind as a stand-alone daemon.
To allow users of systems not run by the full systemd suite, for whatever reason there might be, to at least be able to not have to use ConsoleKit for session tracking, an integration of elogind usage into AccountsService would be very helpful.

As elogind provides the same interface as systemd-login, integration is easy.

Please see the attached patch.

The patch :
a) adds --enable-libelogind to configure.ac,
b) adds the required CFLAGS and LIBS and
c) adds an include <elogind/sd-login.h> to
    src/libaccountsservice/act-user-manager.c
    if --enable-libelogind was issued by the user.

No other changes where needed.
Comment 1 Ray Strode [halfline] 2017-05-31 14:54:46 UTC
Comment on attachment 131598 [details] [review]
Add support for elogind session tracker

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

I'm a little unhappy about this because I feel like systemd has already reached critical mass, and I don't want to repeat historical mistakes.
On the other hand, we still ostensibly have ConsoleKit support at this point, and we have done fixes to work on BSD systems, so I guess this 
can go in provided it's minimally invasive, tested, and actively going to get used by more than one person. 

But i don't understand, if elogind is trying to be a drop in replacement, why does it require any source changes ?
I mean why isn't the header file installed in 

/usr/include/elogind-1.0/systemd/sd-login.h

(or whatever)

and the pkg-config file updated so code works without changes?  Sure configure gook makes sense to me,
but code changes? That just seems wrong for something trying to be a drop in replacement.

::: configure.ac
@@ +317,5 @@
>  if test x$enable_systemd != xno; then
> +   if test "x$have_elogind" != "xno"; then
> +           AC_MSG_NOTICE([Systemd support requested, but elogind found])
> +           have_systemd=no
> +           enable_systemd=no

I don't think elogind should trump systemd if it accidentally gets installed in an everything install or something.  It's the niche, outsider, so it should be explicitly opt-in, imo.

@@ +42,3 @@
>  #include <gio/gunixinputstream.h>
>  
>  #ifdef WITH_SYSTEMD

if we can't get elogind fixed to be transparent, then I'd rather we do #ifdef HAVE_SYSTEMD_SD_LOGIN_H here instead of #ifdef WITH_SYSTEMD (but i'd rather we fix elogind)

@@ +42,5 @@
>  #include <gio/gunixinputstream.h>
>  
>  #ifdef WITH_SYSTEMD
>  #include <systemd/sd-login.h>
> +#else

let's not have an #else here.  It just adds more noise, and unless i'm missing something in my casual reading makes LOGIND_RUNNING not get defined for the normal, non-elogind case? (was this patch tested on a non-elogind system?)

@@ +43,5 @@
>  
>  #ifdef WITH_SYSTEMD
>  #include <systemd/sd-login.h>
> +#else
> +#ifdef WITH_ELOGIND

#ifdef HAVE_ELOGIND_SD_LOGIN_H

@@ +45,5 @@
>  #include <systemd/sd-login.h>
> +#else
> +#ifdef WITH_ELOGIND
> +#include <elogind/sd-login.h>
> +#define WITH_SYSTEMD 1 /* Do not clutter the sources */

This should be AC_DEFINE in configure.ac

@@ +51,1 @@
>  

should just say /* HAVE_ELOGIND_SD_LOGIN_H */ instead of a sentence to match the HAVE_PATHS_H style #endif comment higher up in the code.  Also, this comment isn't closed (was this patch tested? I don't really want to push something that's not getting used)
Comment 2 Sven Eden 2017-06-01 11:00:38 UTC
(In reply to Ray Strode [halfline] from comment #1)
> I'm a little unhappy about this because I feel like systemd has already
> reached critical mass, and I don't want to repeat historical mistakes.
> On the other hand, we still ostensibly have ConsoleKit support at this
> point, and we have done fixes to work on BSD systems, so I guess this 
> can go in provided it's minimally invasive, tested, and actively going to
> get used by more than one person. 

elogind is for users who
a) Do *not* want systemd to run the show but
b) want to use (some/any) software that depends on systemd-login and
c) give ConsoleKit2 a wide berth (maybe because of b))

That's it. Nothing more, nothing less. No anti-systemd vendetta, nothing. Just choice. Because Linux is about choice.

> But i don't understand, if elogind is trying to be a drop in replacement,
> why does it require any source changes ?
> I mean why isn't the header file installed in 
> 
> /usr/include/elogind-1.0/systemd/sd-login.h
> 
> (or whatever)

That would need source changes, too.
The headers are installed in /usr/include/elogind, so they are distinguishable from /usr/include/systemd, although they are basically the same.


> and the pkg-config file updated so code works without changes?  Sure
> configure gook makes sense to me,
> but code changes? That just seems wrong for something trying to be a drop in
> replacement.

Not a single code change. Only one include line. Nothing else.

> ::: configure.ac
> @@ +317,5 @@
> >  if test x$enable_systemd != xno; then
> > +   if test "x$have_elogind" != "xno"; then
> > +           AC_MSG_NOTICE([Systemd support requested, but elogind found])
> > +           have_systemd=no
> > +           enable_systemd=no
> 
> I don't think elogind should trump systemd if it accidentally gets installed
> in an everything install or something.  It's the niche, outsider, so it
> should be explicitly opt-in, imo.

Basically it does not make any sense to install elogind on any systemd powered system. If you let systemd run the show, then elogind makes a lot of headache, but no sense, as it *is* systemd-login without the systemd-attachment.

The above part makes sense from this point of view:
 - The user set ./configure --enable-systemd
 - But elogind is installed (and therefore the running session tracker)
 - Which basically means that syetemd isn't even PID 1, although it *might* be installed (somewhere)

However, you overlooked this:

+AC_ARG_ENABLE([elogind],
+              AS_HELP_STRING([--enable-elogind], [Use elogind]),
+              [enable_elogind=$enableval],
+              [enable_elogind=auto])
+
+if test "x$enable_elogind" != "xno"; then
+   PKG_CHECK_MODULES(ELOGIND, [libelogind >= 219],
+                              [have_elogind=yes],
+                              [have_elogind=no])
+else
+   have_elogind=no
+fi

So elogind actually *is* strictly opt-in.

> 
> @@ +42,3 @@
> >  #include <gio/gunixinputstream.h>
> >  
> >  #ifdef WITH_SYSTEMD
> 
> if we can't get elogind fixed to be transparent, then I'd rather we do
> #ifdef HAVE_SYSTEMD_SD_LOGIN_H here instead of #ifdef WITH_SYSTEMD (but i'd
> rather we fix elogind)


That's your source. Not mine. And elogind is perfectly transparent. Or I do not understand your point.


> 
> @@ +42,5 @@
> >  #include <gio/gunixinputstream.h>
> >  
> >  #ifdef WITH_SYSTEMD
> >  #include <systemd/sd-login.h>
> > +#else
> 
> let's not have an #else here.  It just adds more noise, and unless i'm
> missing something in my casual reading makes LOGIND_RUNNING not get defined
> for the normal, non-elogind case? (was this patch tested on a non-elogind
> system?)

Of course an #else, because the location of the needed include file depends on it.

> should just say /* HAVE_ELOGIND_SD_LOGIN_H */ instead of a sentence to match
> the HAVE_PATHS_H style #endif comment higher up in the code.  Also, this
> comment isn't closed (was this patch tested? I don't really want to push
> something that's not getting used)

Ooops... I'll fix that ASAP. The comment is implicitly closed two lines below.

However, this is you code. The patch adds five lines and changes absolutely *nothing*. Can it be less invasive? (just asking)

Just as a side node: https://bugs.gentoo.org/show_bug.cgi?id=599470

And yes, this patch is tested and runs perfectly fine on my Plasma Desktop (and dozen others) since December 2016. (Plus many other users.)

elogind is used by a lot of people already, not only users of Gentoo Linux, but also users who are using elogind to have a systemd-free gnome desktop, all users of GuixSD and some (adventurous) users of Void Linux.

Sorry for not making this clear. I thought a completely non-invasive patch that changes not a single line of code (just one include path) wouldn't need long explanations. My fault, really.
Comment 3 Sven Eden 2017-06-01 11:46:01 UTC
Created attachment 131641 [details] [review]
Updated Patch to enable elogind support

- Cleaned up configure.ac
- Added WITH_ELOGIND to config.h.in, so autoreconf is no longer needed.
- Extended the changes in src/libaccountsservice/act-user-manager.c so that they are better readable.
Comment 4 Ray Strode [halfline] 2017-06-01 15:31:02 UTC
> Just choice. Because Linux is about choice.
https://www.redhat.com/archives/rhl-devel-list/2008-January/msg00861.html

Anyway, like I said, if there are going to be users, I'll do it after the requested changes are made. who are the users going to be? guix? I'm not going to put it in there for the sake of choice alone, of course.  If it's solving a concrete problem, I'll sigh and cede and be pragmatic.  I'm not going to do it because "linux is about choice" or some irrational reason like that.

> > But i don't understand, if elogind is trying to be a drop in replacement,
> > why does it require any source changes ?
> > I mean why isn't the header file installed in 
> > 
> > /usr/include/elogind-1.0/systemd/sd-login.h
> > 
> > (or whatever)
> 
> That would need source changes, too.
> The headers are installed in /usr/include/elogind, so they are
> distinguishable from /usr/include/systemd, although they are basically the
> same.
No it wouldn't need source changes, if the headers were put in /usr/include/elogind-something/systemd/sd-login.h

and the .pc file adds -I/usr/include/elogind-something to the include path, then source code can remain completely unchanged.  Just configure goo needs a little tweaking.  That "put include files in a versioned directory" is pretty common practice. granted logind doesn't do, but logind isn't trying to be a drop in replacement for something else.

Andy, can elogind be changed to do that so it's a drop in replacement?

>  - But elogind is installed (and therefore the running session tracker)
is the part in parenthesis actually true? if someone with systemd installed accidentally installed elogind because they did an "everything" install, it elogind going to preempt logind ?

> However, you overlooked this:
> 
> +AC_ARG_ENABLE([elogind],
> +              AS_HELP_STRING([--enable-elogind], [Use elogind]),
> +              [enable_elogind=$enableval],
> +              [enable_elogind=auto])
> +
> +if test "x$enable_elogind" != "xno"; then
> +   PKG_CHECK_MODULES(ELOGIND, [libelogind >= 219],
> +                              [have_elogind=yes],
> +                              [have_elogind=no])
> +else
> +   have_elogind=no
> +fi
> 
> So elogind actually *is* strictly opt-in.
what? unless i'm misreading what you pasted, it says "If --enable-elogind is missing from the configure line, set enable_elogind=auto.  If enable_elogind isn't no, for instance, if enable_elogind==auto or enable_elogind==yes  and elogind happens to be installed use elogind."  How is that strictly opt-in? it looks opt-out to me.

> 
> > 
> > @@ +42,3 @@
> > >  #include <gio/gunixinputstream.h>
> > >  
> > >  #ifdef WITH_SYSTEMD
> > 
> > if we can't get elogind fixed to be transparent, then I'd rather we do
> > #ifdef HAVE_SYSTEMD_SD_LOGIN_H here instead of #ifdef WITH_SYSTEMD (but i'd
> > rather we fix elogind)
> 
> 
> That's your source. Not mine. And elogind is perfectly transparent. Or I do
> not understand your point.
I'm asking you to make changes to your patch before i'll integrate it.  I'm saying if WITH_SYSTEMD is used for both logind's then it no longer is a good feature test for "should include systemd logind's header file", and instead a new test should be introduced HAVE_FOO_H is a pretty common idiom in source code, so the test name I suggested was HAVE_SYSTEMD_SD_LOGIN_H.

> > let's not have an #else here.  It just adds more noise, and unless i'm
> > missing something in my casual reading makes LOGIND_RUNNING not get defined
> > for the normal, non-elogind case? (was this patch tested on a non-elogind
> > system?)
> 
> Of course an #else, because the location of the needed include file depends
> on it.
No, you can have two independent #if calls without an #else.  since they won't be set at the same time, it's equivalent.

> However, this is you code. The patch adds five lines and changes absolutely
> *nothing*. Can it be less invasive? (just asking)
Well, yea, it would be less invasive if it only touched configure.ac :-)

> elogind is used by a lot of people already, not only users of Gentoo Linux,
> but also users who are using elogind to have a systemd-free gnome desktop,
> all users of GuixSD and some (adventurous) users of Void Linux.
ok

> Sorry for not making this clear. I thought a completely non-invasive patch
> that changes not a single line of code (just one include path) wouldn't need
> long explanations. My fault, really.
so did you write this patch originally? or are you upstreaming something someone else wrote?
Comment 5 Ray Strode [halfline] 2017-06-01 15:57:27 UTC
Comment on attachment 131641 [details] [review]
Updated Patch to enable elogind support

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

thanks for the update.

::: config.h.in
@@ +307,5 @@
>  if test x$enable_systemd != xno; then
> +   if test "x$enable_elogind" != "xno"; then
> +           AC_MSG_NOTICE([Systemd support requested, but elogind found])
> +           have_systemd=no
> +           enable_systemd=no

still opt-out, i think it should be opt-in.

@@ +47,5 @@
> +
> +#ifdef WITH_ELOGIND
> +#include <elogind/sd-login.h>
> +/* Re-Use WITH_SYSTEMD as elogind substitutes systemd-login */
> +#define WITH_SYSTEMD 1

(still want this in configure.ac not here).  But hopefully we can get elogind fixed and not need these changes
Comment 6 Sven Eden 2017-06-02 07:55:40 UTC
(In reply to Ray Strode [halfline] from comment #4)
> Anyway, like I said, if there are going to be users, I'll do it after the
> requested changes are made. who are the users going to be?

The problem with projects on github is, that there is no way to tell how many users there (probably) are. It might be a few dozens, but could be hundreds or even thousands already. elogind is not new, (See https://lists.gnu.org/archive/html/guix-devel/2015-04/msg00352.html) ; I just messed up to report this patch here for almost 6 months. :-/

But https://www.google.com/search?q=elogind gives a good clue about its impact.

> If it's solving a concrete problem, I'll sigh and cede and be pragmatic. 

I guess this thread is showing that elogind does solve very concrete problems:
https://forums.gentoo.org/viewtopic-t-1022050.html


Some other problems solved by elogind that come to my mind:

- Plasma on wayland without systemd.

- Brightness settings via hotkeys do not work in Plasma (and others) if the modesetting driver is used, and ConsoleKit2 is the session manager, but with elogind.

- suspend to disk on my (32GiB RAM) Laptop takes about 20 Minutes with ConsoleKit2 and hibernate-script, but only 15 seconds with elogind and loginctl.
  (Okay, that one is highly subjective. ;-) )


> No it wouldn't need source changes, if the headers were put in
> /usr/include/elogind-something/systemd/sd-login.h
> 
> and the .pc file adds -I/usr/include/elogind-something to the include path,
> then source code can remain completely unchanged.  Just configure goo needs
> a little tweaking.

Well, the practice to put the includes into /usr/include/elogind is historical. In the beginning, it was not clear whether it might sense to install elogind on a systemd-powered system. I just haven't changed that since I took over the development.

However, having both systemd and elogind does not make any sense, because elogind is no "drop in replacement", it is a systemd-login substitution for systems that are *not* run by systemd. It is the same code.
See: https://github.com/elogind/elogind/graphs/contributors


> That "put include files in a versioned directory" is
> pretty common practice. granted logind doesn't do, but logind isn't trying
> to be a drop in replacement for something else.

I do not like versioned directories. They do not make any sense for software that isn't slotted.

*But* I think it would be very nice to really have the include path handled by pkg-config.

So I have just added "Change include install to /usr/include/elogind/systemd" to my TODO list for the v230 release. ;-)

> >  - But elogind is installed (and therefore the running session tracker)
> is the part in parenthesis actually true? if someone with systemd installed
> accidentally installed elogind because they did an "everything" install, it
> elogind going to preempt logind ?

I changed that already. When I fixed that patch I noticed that enable_elogind=auto is set. A very bad idea.

However, it is the distributions responsibility to not let users install systemd and elogind at the same time. My ebuilds for Gentoo do this already. 

On a system where systemd is only installed to have some library but using another PID1 (and a different session tracker), enable_systemd=auto is bad, too. ;-)

When I patch software for elogind support, I never *fix* upstream, but copy and adapt. That's where the =auto came from.

> I'm asking you to make changes to your patch before i'll integrate it.  I'm
> saying if WITH_SYSTEMD is used for both logind's then it no longer is a good
> feature test for "should include systemd logind's header file", and instead
> a new test should be introduced HAVE_FOO_H is a pretty common idiom in
> source code, so the test name I suggested was HAVE_SYSTEMD_SD_LOGIN_H.

Yes. This is your code. I never fix upstream.

However, the reuse of WITH_SYSTEMD is for the lines below. Otherwise all occurences of

#ifdef WITH_SYSTEMD
(... systemd-login specific code ...)
#endif

Must be substituted by

#if defined(WITH_SYSTEMD) || defined (WITH_ELOGIND)
(... systemd-login specific code ...)
#endif

Which is as invasive as you can get.

How about this:

- Change configure.ac and config.h.in to use
   A) HAVE_SYSTEMD_SD_LOGIN_H for systemd and
   B) HAVE_ELOGIND_SD_LOGIN_H for elognd
  to determine which file to include.
  (Note: This is no longer necessary when elogind-v230 changes the include
   path, so <systemd/sd-login.h> is valid for elogind, too.)
- Substitute WITH_SYSTEMD with WITH_SYSTEMD_LOGIN as that is more exact
  and perfectly valid for elogind, as it *is* systemd-login.


When elogind-v230 is released, however, the first part can be substituted with HAVE_SYSTEMD_LOGIN. That'll make things a bit easier, and it should be perfectly clear.


> No, you can have two independent #if calls without an #else.  since they
> won't be set at the same time, it's equivalent.

You are right, I fixed that.


> Well, yea, it would be less invasive if it only touched configure.ac :-)

Point taken. ;-)


> so did you write this patch originally? or are you upstreaming something
> someone else wrote?


I wrote it. But it was one of my first when I started my work on integrating elogind into Gentoo Linux. That's the reason it is off some kind of "works-for-me"-quality.

Unfortunately, bringing the support upstream is hard work.
Projects like SDDM or PolKit ignore them for months.
But others like NetworkManager (https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=e40d47fe) or udisks (https://github.com/storaged-project/udisks/commit/884b630) already support elogind using my patches.

And then there are projects like PowerDevil, which do it right and simply ask dbus for an org.freedesktop.login1 interface, and do not need any includes or configure options. They just work.


(In reply to Ray Strode [halfline] from comment #5)
> thanks for the update.
> 
> ::: config.h.in
> @@ +307,5 @@
> >  if test x$enable_systemd != xno; then
> > +   if test "x$enable_elogind" != "xno"; then
> > +           AC_MSG_NOTICE([Systemd support requested, but elogind found])
> > +           have_systemd=no
> > +           enable_systemd=no
> 
> still opt-out, i think it should be opt-in.

Well, no. You have to explicitly use --enable-elogind now on a system with systemd installed for that Notice to appear.

But I think the wording is bad. And it might be better to assume, that if enable_systemd was set to auto and detected, that --enable-elogind was a mistake, so error out instead of just giving a warning.

I'll fix that.

> (still want this in configure.ac not here).  But hopefully we can get
> elogind fixed and not need these changes

Unfortunatly the change will break existing packages.

So I am currently thinking about changing the install a bit more, so that the include (reported by pkg-config as well) goes into
  /usr/include/elogind/systemd
and adding a symlink
  /usr/include/elogind/sd-login.h -> systemd/sd-login.h
so existing packages still find their includes.

That pkg-config adds -I/usr/include/elogind to the CPPFLAGS shouldn't be a problem, as /usr/include is searched anyway.

What do you think?
Comment 7 Andy Wingo 2017-06-02 08:54:51 UTC
Hi!

Just a few comments.  Firstly I defer to Sven who has been maintaining elogind for the last year or so, and doing a great job at that; mine are words from a former maintainer, please take them with a grain of salt.

Guix uses elogind and has been happy with it.  When we build packages that use systemd interfaces, we have custom code to patch those packages as part of the build process, typically substituting "systemd" for "elogind" in some of the files in the package.

Initially I chose not to make mock "systemd" pkg-config files because having "systemd >= 219" means more than "elogind >= 219", and I didn't want to cause spurious bug reports to systemd users around the world caused by elogind/systemd confusion.

If I understand Ray right: could we make elogind easier to just drop in by exporting systemd/sd-login.h instead of elogind/sd-login.h?  Yes of course we could.  A system with elogind won't have systemd, so there's no real problem there.  But my question is, do you we want to open ourselves to systemd/elogind confusion bugs?  Compile errors where it's not clear whether an interface is coming from the supported systemd/sd-login.h or the contributed elogind/sd-login.h?  For me I would keep things as they are, but mine is an "emeritus" perspective; you all are the real maintainers.
Comment 8 Andy Wingo 2017-06-02 08:57:27 UTC
One more unsolicited comment :)  If I maintained a project that depended on either systemd-logind or consolekit[2], I would drop consolekit[2] support in favor of elogind.  That way you reduce your code from having two backends to having one, and you preserve the ability to compile on systems that for whatever reason don't run systemd.
Comment 9 Sven Eden 2017-06-02 09:31:51 UTC
(In reply to Andy Wingo from comment #8)
> One more unsolicited comment :)  If I maintained a project that depended on
> either systemd-logind or consolekit[2], I would drop consolekit[2] support
> in favor of elogind.  That way you reduce your code from having two backends
> to having one, and you preserve the ability to compile on systems that for
> whatever reason don't run systemd.

Adding elogind support is nothing more than deciding which file to include.

You can do more, of course, just take a look at how elogind support got integrated into NetworkManager-1.8. But *they* need support for suspend/resume. So the example is a bit off...

However, I have added an experimental dev_v230_header_move branch with https://github.com/elogind/elogind/commit/d1db417

The results so far:

--------
 $ ls -l usr/include/elogind/*
 usr/include/elogind/_sd-common.h -> systemd/_sd-common.h
 usr/include/elogind/sd-id128.h -> systemd/sd-id128.h
 usr/include/elogind/sd-login.h -> systemd/sd-login.h
 usr/include/elogind/sd-messages.h -> systemd/sd-messages.h

usr/include/elogind/systemd:
 _sd-common.h
 sd-id128.h
 sd-login.h
 sd-messages.h

 $ pkg-config --cflags ./usr/lib64/pkgconfig/libelogind.pc
-I/tmp/elogind_test/usr/include/elogind
--------

This would work, of course, and we can experiment with it.
But as Andy already wrote, any compiler error on any package supporting both, will present <systemd/foo> to the user.

If enough time passed since the last update, the user might not even be aware, that they look at an unfortunate bug in elogind...
Comment 10 Ray Strode [halfline] 2017-06-02 14:25:43 UTC
(In reply to Sven Eden from comment #6)
> I guess this thread is showing that elogind does solve very concrete
okay.

> *But* I think it would be very nice to really have the include path handled
> by pkg-config.
> 
> So I have just added "Change include install to
> /usr/include/elogind/systemd" to my TODO list for the v230 release. ;-)
cool.  Should make it easier to upstream integration patches in various places.

> - Change configure.ac and config.h.in to use
>    A) HAVE_SYSTEMD_SD_LOGIN_H for systemd and
>    B) HAVE_ELOGIND_SD_LOGIN_H for elognd
>   to determine which file to include.
>   (Note: This is no longer necessary when elogind-v230 changes the include
>    path, so <systemd/sd-login.h> is valid for elogind, too.)
I guess let's just make the PKG_CHECK_MODULES call >= 230 then.

> - Substitute WITH_SYSTEMD with WITH_SYSTEMD_LOGIN as that is more exact
>   and perfectly valid for elogind, as it *is* systemd-login.
Sure we can rename this.

> I wrote it. 
Okay, cool. I just wanted to make sure the authorship was right in the commit log.

> Well, no. You have to explicitly use --enable-elogind now on a system with
> systemd installed for that Notice to appear.
okay.

> But I think the wording is bad. And it might be better to assume, that if
> enable_systemd was set to auto and detected, that --enable-elogind was a
> mistake, so error out instead of just giving a warning.
> 
> I'll fix that.
even better.

> Unfortunatly the change will break existing packages.
> 
> So I am currently thinking about changing the install a bit more, so that
> the include (reported by pkg-config as well) goes into
>   /usr/include/elogind/systemd
> and adding a symlink
>   /usr/include/elogind/sd-login.h -> systemd/sd-login.h
> so existing packages still find their includes.
> 
> That pkg-config adds -I/usr/include/elogind to the CPPFLAGS shouldn't be a
> problem, as /usr/include is searched anyway.
> 
> What do you think?
sounds great, maybe with --enable-compat-symlink configure option that defaults
to "yes" and in a year could default to "no"
Comment 11 Ray Strode [halfline] 2017-06-02 14:31:22 UTC
(In reply to Andy Wingo from comment #7)
> Hi!
hey thanks for chiming in. hi btw ! haven't seen you in #gnome-hackers in a while.

> Just a few comments.  Firstly I defer to Sven who has been maintaining
> elogind for the last year or so, and doing a great job at that; mine are
> words from a former maintainer, please take them with a grain of salt.
ah okay, I didn't realize that.  I did a quick google search for elogind and saw you wrote it and thought you were the maintainer. sorry for dragging you into this when you're not the maintainer.

> Initially I chose not to make mock "systemd" pkg-config files because having
> "systemd >= 219" means more than "elogind >= 219", and I didn't want to
> cause spurious bug reports to systemd users around the world caused by
> elogind/systemd confusion.
Sure, I'm okay with the .pc file being called elogind.  I think that's better in fact.  But it's best, imo, if the gook is confined to configure.ac instead of the source files. every #ifdef makes an angel cry or something.

> If I understand Ray right: could we make elogind easier to just drop in by
> exporting systemd/sd-login.h instead of elogind/sd-login.h?  Yes of course
> we could.  A system with elogind won't have systemd, so there's no real
> problem there.  But my question is, do you we want to open ourselves to
> systemd/elogind confusion bugs?  Compile errors where it's not clear whether
> an interface is coming from the supported systemd/sd-login.h or the
> contributed elogind/sd-login.h?
Well, I think that's adequately addressed by making sure elogind is fully opt-in at configure time, which it is now.
Comment 12 Ray Strode [halfline] 2017-06-02 14:32:42 UTC
(In reply to Andy Wingo from comment #8)
> One more unsolicited comment :)  If I maintained a project that depended on
> either systemd-logind or consolekit[2], I would drop consolekit[2] support
> in favor of elogind
sounds good to me, except, apparently bsd doesn't work with elogind yet. See this gnome bug comment from yesterday:

https://bugzilla.gnome.org/show_bug.cgi?id=783313#c2
Comment 13 Sven Eden 2017-06-05 11:46:34 UTC
I have though about this for the last few days, and this really is a medal with two sides.

1) /usr/include/elogind/sd-login.h
  -> - downstream must explicitly opt in for the elogind header in their sources.
  -> + any compilation error already shows what is going on.
  -> - the distinction can not be put solely into configure.ac
  -> + no confusion possible.

2) /usr/include/elogind/systemd/sd-login.h
  -> + downstream does not need to change a singly line in their sources.
  -> - compilation errors will complain about <systemd/sd-login.h>, no distinction
       possible.
  -> + the distinction can be put solely in configure.ac
  -> - the result of the configuration stage alone can be confusing. (elogind
       chosen, but systemd look'n'feel in header selection)

I know I am exaggerating a bit. But there you are.

At the end of the day I am convinced that there is not a single true valid way, but both are perfectly fine ; depending on downstreams point of view.

So what I'd like to do is what I have already experimented with:

 - Install headers in $PREFIX/include/elogind/systemd
 - symlink headers in $PREFIX/include/elogind
 - Let Downstream chose their way at their own discretion.

To make this available as soon as possible, I will backport the patch for the install location change to v229. The reason for this is simple: I do not plan to release v230, as there is no systemd stable branch for that version. The next version with a branch in systemd/systemd-stable is v231. But I do not want to wait that long.

After the release of elogind-v229.4 I'll post a patch here to make use of this header move.
Comment 14 Sven Eden 2017-06-06 07:38:22 UTC
Created attachment 131732 [details] [review]
Updated Patch to enable elogind support - configure.ac only

Ray, this is the updated patch.

I have not renamed the defines, but (slightly) "fixed" the systemd detection block, so both blocks look identical. I think that enhances readability.
Comment 15 Sven Eden 2017-06-06 08:13:17 UTC
Created attachment 131733 [details] [review]
[FIXED] Updated Patch to enable elogind support - configure.ac only

Sorry, I have overlooked an idiotic typo. Fixed and tested now.
Comment 16 Ray Strode [halfline] 2017-06-06 14:22:46 UTC
okay i pushed this with minor changes:

https://cgit.freedesktop.org/accountsservice/commit/?id=9fdd1d95ec094a0df6d8d3dd9c8f04fa8499b845

please let me know if one of those changes broke something or a follow up commit is needed.

Thanks for the work on this.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.