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

Previous Next

Full log

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

From: Brice Waegeneire <brice <at> waegenei.re>
To: guix <at> lprndn.info
Cc: 35305 <at> debbugs.gnu.org
Subject: Re: [bug#35305] LightDM service
Date: Tue, 07 Apr 2020 17:06:34 +0000

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.

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

> `("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.

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

> +(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.”


> +(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.

> +        (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”?

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


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

- Brice

Previous Next

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