diff mbox series

[bug#35305] LightDM service

Message ID 87o8qtzd71.fsf@lprndn.info
State Superseded
Headers show
Series [bug#35305] LightDM service | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job

Commit Message

L p R n d n May 12, 2020, 9:59 a.m. UTC
Hello,

Little from my side, I forgot to add the polkit extension of the lightdm
service so here is a patch.

Have a nice day,

L  p R n  d n

Comments

Ricardo Wurmus May 20, 2020, 8:51 p.m. UTC | #1
Hey,

I’m very sorry for the delay.

What took me so long is that I’m conflicted about how to move forward.
On one hand I really don’t want to delay this.  I think your patches are
a great and important addition to Guix.  On the other hand I feel that
the relationship between these new components isn’t quite right.

It still doesn’t feel quite right to me that there’s a
lightdm-service-type and an independent
lightdm-gtk-greeter-service-type.  I know that there can be any number
of greeters, but only one will be used with lightdm-service-type
dependent on the string value of greeter-session.  This leads to
potential misconfiguration as we don’t (and can’t) validate this string.

It also feels wrong to me to have a global directory as the only
location for greeter desktop files, which means that all greeters must
be installed globally.

What I envision is something like this: we only have a single
lightdm-service-type and it has a field “greeters”, which by default is
a list of just one item: a <greeter> record containing the
lightdm-gtk-greeter and its configuration.

Other greeters could be added; they would all be record values of type
<greeter> and come with their own extensions of the
ligthdm-service-type.

The lightdm-service-type would go through each of the greeters in the
list and apply their specified extensions to itself.

The reason why I haven’t implemented this yet is because a) I don’t want
to break what already works with your patches and b) I don’t know if
that’s ultimately a clearer implementation.

I feel that this would be a more intuitive configuration and result in
clearer relationships between the display manager and its swappable
components.  It would also allow for better defaults (so less
configuration needed) and avoid the problem of stringy types that are
easy to get wrong.

Maybe we are already really close to this solution, though: maybe the
proposed “greeter” field could simply accept service types like the
lightdm-gtk-greeter-service-type you already defined.

I’m going to play with this a little bit more, but if this doesn’t lead
anywhere I’ll merge the current version.

My apologies for delaying this!
L p R n d n May 21, 2020, 8:28 a.m. UTC | #2
Hello,

Ricardo Wurmus <rekado@elephly.net> writes:

> Hey,
>
> I’m very sorry for the delay.

Again, there's no hurry. ;)

> What took me so long is that I’m conflicted about how to move forward.
> On one hand I really don’t want to delay this.  I think your patches are
> a great and important addition to Guix.  On the other hand I feel that
> the relationship between these new components isn’t quite right.
>
> It still doesn’t feel quite right to me that there’s a
> lightdm-service-type and an independent
> lightdm-gtk-greeter-service-type.  I know that there can be any number
> of greeters, but only one will be used with lightdm-service-type
> dependent on the string value of greeter-session.  This leads to
> potential misconfiguration as we don’t (and can’t) validate this string.

Just to clarify, as per my understanding, there can be multiple
`greeter-session fields defined. It's not a global value but a per seat
one. Each seat should be able to use a different greeter, I
think. Personally, I have a very standard use whith only one seat so
there are no questions in that case. However there might be use-cases
where it's needed. I CC bricewge, they might be more knowledgeable on
this issue.

> It also feels wrong to me to have a global directory as the only
> location for greeter desktop files, which means that all greeters must
> be installed globally.

Isn't using packages as inputs of `greeter-session solving this issue?
We can collect them from seats and string-join them into the
`greeter-directory field.

> What I envision is something like this: we only have a single
> lightdm-service-type and it has a field “greeters”, which by default is
> a list of just one item: a <greeter> record containing the
> lightdm-gtk-greeter and its configuration.
>
> Other greeters could be added; they would all be record values of type
> <greeter> and come with their own extensions of the
> ligthdm-service-type.
>
> The lightdm-service-type would go through each of the greeters in the
> list and apply their specified extensions to itself.

If I understand correctly, the main difference would be that the
greeters would be defined from within the lightdm-service (as a list of records?), right?
The current implementation was chosen to avoid too much field nesting
but I don't mind.
Also, you can have a look at the previous implementation (see
mail from 19th of March). It lacks seats and some featurse but it looks a
little closer to what you're describing. It might give some ideas.

> The reason why I haven’t implemented this yet is because a) I don’t want
> to break what already works with your patches and b) I don’t know if
> that’s ultimately a clearer implementation.
>
> I feel that this would be a more intuitive configuration and result in
> clearer relationships between the display manager and its swappable
> components.  It would also allow for better defaults (so less
> configuration needed) and avoid the problem of stringy types that are
> easy to get wrong.
>
> Maybe we are already really close to this solution, though: maybe the
> proposed “greeter” field could simply accept service types like the
> lightdm-gtk-greeter-service-type you already defined.

We're close. :)
Just curious here, if we use a list of services (for example) as input of
the greeters field, how do we take care of it? Can we "merge" the
different services into the lightdm one? If it's possible, this might be
a good solution.

> I’m going to play with this a little bit more, but if this doesn’t lead
> anywhere I’ll merge the current version.
>
> My apologies for delaying this!

Everything is going a lot faster than it was a few months ago so it's
already fine.

Have a nice day,

L  p R n  d n
Ricardo Wurmus May 21, 2020, 9:23 a.m. UTC | #3
L p R n d n <guix@lprndn.info> writes:

> Hello,
>
> Ricardo Wurmus <rekado@elephly.net> writes:
>
>> Hey,
>>
>> I’m very sorry for the delay.
>
> Again, there's no hurry. ;)
>
>> What took me so long is that I’m conflicted about how to move forward.
>> On one hand I really don’t want to delay this.  I think your patches are
>> a great and important addition to Guix.  On the other hand I feel that
>> the relationship between these new components isn’t quite right.
>>
>> It still doesn’t feel quite right to me that there’s a
>> lightdm-service-type and an independent
>> lightdm-gtk-greeter-service-type.  I know that there can be any number
>> of greeters, but only one will be used with lightdm-service-type
>> dependent on the string value of greeter-session.  This leads to
>> potential misconfiguration as we don’t (and can’t) validate this string.
>
> Just to clarify, as per my understanding, there can be multiple
> `greeter-session fields defined. It's not a global value but a per seat
> one. Each seat should be able to use a different greeter, I
> think. Personally, I have a very standard use whith only one seat so
> there are no questions in that case. However there might be use-cases
> where it's needed. I CC bricewge, they might be more knowledgeable on
> this issue.

Right, I realized this after composing my message.  However, currently
the lightdm-gtk-greeter-service-type inherits all the seats and then
overrides the greeter-session value for each of them, which seems rather
rude.  So maybe it is wrong to let greeters do that at all.

I wondered why there’s a service type for the greeter at all, so I
looked at the service extensions it provides:

* lightdm-service-type: only used to override greeter-session in all
  defined seats, which seems like an anti-feature.  If other greeters
  do the same, then effectively there can only be one greeter for all
  seats, and that would be wrong.  So seat configuration really should
  remain in lightdm-service-type and not be an extension.

* etc-service-type: that’s to provide the greeter’s global configuration
  file.  Ideally, we would not need to use a global configuration file.
  It looks like lightdm-gtk-greeter respects the XDG_CONFIG_DIRS
  variable, so we should be able to generate its configuration file in
  an arbitrary location and then add it to XDG_CONFIG_DIRS in the
  environment of lightdm itself.

* profile-service-type: that’s to install lightdm-gtk-greeter and its
  assets into the system profile.  Again, that’s something we should aim
  to avoid.  It seems that we can avoid it with the use of environment
  variables in the lightdm shepherd service.

If we can avoid all three extensions then we don’t need a
lightdm-gtk-greeter-service-type at all.  If we don’t need a service we
can specify greeters as record type values with a name and configuration
file generator.
L p R n d n June 8, 2020, 3:35 p.m. UTC | #4
Hello,

I spent some time thinking about the lightdm service and what are our
possibilities so here are my thoughts and conclusions.

(Here, I'll only describe relations between seats, greeters and the lightdm
service as it's our main source of problems)

So if we get rid of the greeter's services, we can have:

1:
(service lightd-service-type
    (lightdm-configuration
        (greeters
            (list
                (lightdm-gtk-greeter-configuration
                  (seats
                      (list
                          (lightdm-seat-configuration ...))))))))

Here seats are defined by greeters. This way the user doesn't need to
fill `greeter-session field as we can do it automatically. But there's a lot of nesting
(and two lists.) + How do we define autologin?

2:
(service lightd-service-type
    (lightdm-configuration
        (seats
            (list
                (lightdm-seat-configuration
                    (greeter-session
                        (lightdm-gtk-greeter-configuration ...)))))))

Defining greeters inside seats allows in the `greeter-session field make
it a little simpler. However, we would get errors or conflicts if a user
define two different configurations of the same greeter for two
different seats. The thing is that we can only have one configuration
per greeter as it will always look for a hardcoded file in /etc/ (worst
case) or a file we hardcoded at build time (best case). :/

3:
(service lightd-service-type
    (lightdm-configuration
        (seats
            (list
                (lightdm-seat-configuration
                    (greeter-session 'lightdm-gtk-greeter))))
        (greeters
            (list
                (lightdm-gtk-greeter-configuration
                  )))))

We can have separate fields for greeters and seats. The user will have
to define `greeter-session by himself. And what happen if they define
multiple occurences of one greeter's configuration?


So my conclusion is that the current implementation using a different service for lightdm and its
greeters is probably not so bad as it prevents most of these
issues. Unless someone has an even better idea, indeed. :D

Ricardo Wurmus <rekado@elephly.net> writes:

> L p R n d n <guix@lprndn.info> writes:
>
>> Hello,
>>
>> Ricardo Wurmus <rekado@elephly.net> writes:
[...]
> Right, I realized this after composing my message.  However, currently
> the lightdm-gtk-greeter-service-type inherits all the seats and then
> overrides the greeter-session value for each of them, which seems rather
> rude.  So maybe it is wrong to let greeters do that at all.
>
> I wondered why there’s a service type for the greeter at all, so I
> looked at the service extensions it provides:
>
> * lightdm-service-type: only used to override greeter-session in all
>   defined seats, which seems like an anti-feature.  If other greeters
>   do the same, then effectively there can only be one greeter for all
>   seats, and that would be wrong.  So seat configuration really should
>   remain in lightdm-service-type and not be an extension.

As disussed on IRC, a greeter service only overwrites its own
`greeter-session field. But we could get rid of the `greeter-session
field altogether as it's probably not necessary if they're filled by the
greeters' service or record (depending of what we choose).

> * etc-service-type: that’s to provide the greeter’s global configuration
>   file.  Ideally, we would not need to use a global configuration file.
>   It looks like lightdm-gtk-greeter respects the XDG_CONFIG_DIRS
>   variable, so we should be able to generate its configuration file in
>   an arbitrary location and then add it to XDG_CONFIG_DIRS in the
>   environment of lightdm itself.

I didn't find any informations about lightdm-gtk-greeter using
XDG_CONFIG_DIRS. Could you provide further explaination as it could
impact what I wrote earlier.

> * profile-service-type: that’s to install lightdm-gtk-greeter and its
>   assets into the system profile.  Again, that’s something we should aim
>   to avoid.  It seems that we can avoid it with the use of environment
>   variables in the lightdm shepherd service.

Yeah ;)

> If we can avoid all three extensions then we don’t need a
> lightdm-gtk-greeter-service-type at all.  If we don’t need a service we
> can specify greeters as record type values with a name and configuration
> file generator.

Hope it's clear, I'm really just dumping my thoughts... :)
Please tell me what you think.

Have a nice day,

L  p R n  d n
Maxim Cournoyer Aug. 4, 2022, 5:09 a.m. UTC | #5
Hi,

L  p R n  d n    <guix@lprndn.info> writes:

> Hello,
>
> I spent some time thinking about the lightdm service and what are our
> possibilities so here are my thoughts and conclusions.
>
> (Here, I'll only describe relations between seats, greeters and the lightdm
> service as it's our main source of problems)
>
> So if we get rid of the greeter's services, we can have:
>
> 1:
> (service lightd-service-type
>     (lightdm-configuration
>         (greeters
>             (list
>                 (lightdm-gtk-greeter-configuration
>                   (seats
>                       (list
>                           (lightdm-seat-configuration ...))))))))
>
> Here seats are defined by greeters. This way the user doesn't need to
> fill `greeter-session field as we can do it automatically. But there's a lot of nesting
> (and two lists.) + How do we define autologin?
>
> 2:
> (service lightd-service-type
>     (lightdm-configuration
>         (seats
>             (list
>                 (lightdm-seat-configuration
>                     (greeter-session
>                         (lightdm-gtk-greeter-configuration ...)))))))
>
> Defining greeters inside seats allows in the `greeter-session field make
> it a little simpler. However, we would get errors or conflicts if a user
> define two different configurations of the same greeter for two
> different seats. The thing is that we can only have one configuration
> per greeter as it will always look for a hardcoded file in /etc/ (worst
> case) or a file we hardcoded at build time (best case). :/
>
> 3:
> (service lightd-service-type
>     (lightdm-configuration
>         (seats
>             (list
>                 (lightdm-seat-configuration
>                     (greeter-session 'lightdm-gtk-greeter))))
>         (greeters
>             (list
>                 (lightdm-gtk-greeter-configuration
>                   )))))
>
> We can have separate fields for greeters and seats. The user will have
> to define `greeter-session by himself. And what happen if they define
> multiple occurences of one greeter's configuration?

After reading more about lightdm (and there's not much to read about
it... [0]), I can better understand your points above, and I find #3
perhaps the most natural.  The negative points (leaving place for user
errors) can be mitigated by a validating the configuration (e.g. that
all the configuration types listed in greeters are different).

I'll try to adjust your code to use the above layout when I get a
chance.

Thanks,

Maxim

[0]  https://wiki.ubuntu.com/LightDM
diff mbox series

Patch

From e40317008723f27743f72f90008587ca2f779dde Mon Sep 17 00:00:00 2001
From: L  p R n  d n <guix@lprndn.info>
Date: Mon, 11 May 2020 20:15:55 +0200
Subject: [PATCH 10/10] services: lightdm: Extend polkit service.

* gnu/services/lightdm.scm (lightdm-service-type): Add the LightDM package to
the polkit service.
---
 gnu/services/lightdm.scm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gnu/services/lightdm.scm b/gnu/services/lightdm.scm
index fa5330aade..5edeecd195 100644
--- a/gnu/services/lightdm.scm
+++ b/gnu/services/lightdm.scm
@@ -308,6 +308,8 @@  remote-sessions-directory = " ,remote-sessions-directory "
                   (service-extension dbus-root-service-type
                                      (compose list
                                               lightdm-configuration-lightdm))
+                  (service-extension polkit-service-type
+                                     (compose list lightdm-configuration-lightdm))
                   (service-extension account-service-type
                                      (const %lightdm-accounts))
                   (service-extension etc-service-type
-- 
2.26.1