Hello,

Brice Waegeneire <brice@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