From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:57346) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jLrh6-0004vE-9X for guix-patches@gnu.org; Tue, 07 Apr 2020 13:07:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1jLrh5-0005yJ-Ag for guix-patches@gnu.org; Tue, 07 Apr 2020 13:07:24 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:39242) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1jLrh5-0005yD-73 for guix-patches@gnu.org; Tue, 07 Apr 2020 13:07:23 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jLrh3-0003we-SH for guix-patches@gnu.org; Tue, 07 Apr 2020 13:07:21 -0400 Subject: [bug#35305] LightDM service References: <87zhooso9g.fsf@lprndn.info> In-Reply-To: <87zhooso9g.fsf@lprndn.info> Resent-Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Tue, 07 Apr 2020 17:06:34 +0000 From: Brice Waegeneire Message-ID: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: guix@lprndn.info Cc: 35305@debbugs.gnu.org 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 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