[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#35305] LightDM service

From: Brice Waegeneire
Subject: [bug#35305] LightDM service
Date: Tue, 07 Apr 2020 17:06:34 +0000
User-agent: Roundcube Webmail/1.3.8

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")
                              ,(string-append (assoc-ref inputs "glib")
,(string-append (assoc-ref inputs "shared-mime-info")
                              ,(string-append (assoc-ref inputs "gtk+")
                              ,(string-append (assoc-ref inputs "exo")
                              ,(string-append (assoc-ref outputs "out")
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

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

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

- Brice

reply via email to

[Prev in Thread] Current Thread [Next in Thread]