GNU bug report logs - #35305
[WIP] LightDM service

Previous Next

Full log

Message #38 received at 35305 <at> debbugs.gnu.org (full text, mbox):

From: L  p R n  d n    <guix <at> lprndn.info>
To: Brice Waegeneire <brice <at> waegenei.re>
Cc: 35305 <at> debbugs.gnu.org
Subject: Re: [bug#35305] LightDM service
Date: Thu, 09 Apr 2020 18:02:15 +0200

[Message part 1 (text/plain, inline)]

Hello,

Brice Waegeneire <brice <at> waegenei.re> writes:

> Hello L p R n d n,
>
> I never did a review before but I would like to see this patch merged, so
> bear with me.

Thank you for the review! It's my first service for guix so we're
probably even here. ;)

> The indentation of lightdm's origin should probably be done in the commit
> 01 not 03.

Done.

>> `("XDG_DATA_DIRS" ":" prefix (,(string-append (assoc-ref inputs
>> "hicolor-icon-theme")
>>                                               "/share")
>>                               ,(string-append (assoc-ref inputs "glib")
>>                                               "/share")
>>                               ,(string-append (assoc-ref inputs
>> "shared-mime-info")
>>                                               "/share")
>>                               ,(string-append (assoc-ref inputs "gtk+")
>>                                               "/share")
>>                               ,(string-append (assoc-ref inputs "exo")
>>                                               "/share")
>>                               ,(string-append (assoc-ref outputs "out")
>>                                               "/share")
>>                               "/run/current-system/profile/share"))
> This part can use a map procedure.

Done. + cleaned some things that weren't necessary.

> It would be nice if “lightdm-service-type” support “set-xorg-configuration”
> like the other login manager now does by using “handle-xorg-configuration”
> see 50be0da7bfd5c108697679effeb2a893d2f37598 for how it's done in GDM, SLIM
> and co.
>
>> +         (comment "LighDM user")
>                           ^ a “t” is missing here

Huh.. Done (I think...) and done!

>> +(define (lightdm-shepherd-service config)
>> +  "Return a <lightdm-service> for LightDM with CONFIG."
>> +
>> +  (define lightdm-command
>> +    #~(list (string-append #$(lightdm-configuration-lightdm config)
>> "/sbin/lightdm")))
> […]
>> + (fork+exec-command
>> + (list #$(file-append
>> + (lightdm-configuration-lightdm config)
>> + "/sbin/lightdm"))
>
> “lightdm-command” isn't used, I get the hint it ought to be the argument of
> “fork+exec-command.”

Done.

>
>> +(define (lightdm-etc-service config)
>> +  (list `("xdg/lightdm/lightdm.conf.d/lightdm.conf"
>> +          ,(lightdm-configuration-file config))
>> +        `(,(string-append "xdg/lightdm/"
>> +                          (computed-file-name
>> +                           (lightdm-configuration-greeter-configuration
>> config)))
>> +          ,(lightdm-configuration-greeter-configuration config))))
>
> I've been told, in Guix, it's better to avoid putting configuration in
> “/etc” since it cause edge case during rollback and such, specifying the
> configuration by passing the “--config” argument to “lightdm” would be more
> appropriate.

Need some advices here as even if "--config" works for LightDM, greeters
also search their config in /etc. They're all different so they might or
might not provide a convenient way to do it... :/
In the meantime, kept the etc-service-extension + prevented errors in
case a file wasn't provided.

>> +        (define %user
>> +          (getpw "lightdm"))
>> +        (let ((directory "/var/lib/lightdm-data"))
>> +          (mkdir-p directory)
>> +          (chown directory (passwd:uid %user) (passwd:gid %user))))))
>
> “%user” could go in the “let”. BTW can't lightdm use its user home
> directory instead of “/var/lib/lightdm-data” or the reverse; IOW does it
> need to have to own two directories in “/var/lib”?

Reworked everything a little bit to match what is done for gdm.
I think we can use a CFLAG to change "/var/lib/lightdm-data" to
"/var/lib/lightdm/lightdm-data" for example. However, I think lightdm
sometime cleans or delete stuff in "/var/lib/lightdm" so it might
explain why there are two directories. I don't know what
"/var/lib/lightdm-data" is used for but LightDM does complain if it
doesn't exist. Advices needed here too.

> Several lines in “gnu/services/lightdm.scm” exceed the maximal line length.
>

Tried to keep them below 80. Is it ok?

>
> Thank you very much for this patch series. I'm impatient seeing it in Guix
> proper.
>
> - Brice


+ Corrected some typos in the documentation and added an extra-config
field to lightd and lightdm-gtk-greeter's configuration.

Hope it's better now, new patches are attached below.

Have a nice day,

L  p R n  d n

[0001-gnu-lightdm-Update-1.30.0.patch (text/x-patch, attachment)]

[0002-gnu-lightdm-Add-vala-bindings.patch (text/x-patch, attachment)]

[0003-gnu-lightdm-Disable-python-tests-only.patch (text/x-patch, attachment)]

[0004-gnu-lightdm-gtk-greeter-Update-to-2.0.7.patch (text/x-patch, attachment)]

[0005-gnu-lightdm-gtk-greeter-Fix-at-spi-runtime-dependenc.patch (text/x-patch, attachment)]

[0006-gnu-lightdm-gtk-greeter-Fix-.desktop-file.patch (text/x-patch, attachment)]

[0007-gnu-lightdm-gtk-greeter-Wrap-binary.patch (text/x-patch, attachment)]

[0008-gnu-lightdm-Build-accountsservice-files.patch (text/x-patch, attachment)]

[0009-gnu-lightdm-gtk-greeter-Fix-some-warnings.patch (text/x-patch, attachment)]

[0010-services-Add-lightdm-service-type.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.