diff mbox series

[bug#65343,v2] home: services: Add 'x11-display' service.

Message ID 87wmuvygs9.fsf@gnu.org
State New
Headers show
Series [bug#65343,v2] home: services: Add 'x11-display' service. | expand

Commit Message

Ludovic Courtès Nov. 5, 2023, 10:30 p.m. UTC 
Hi,

Oleg Pykhalov <go.wigust@gmail.com> skribis:

> During testing in a virtual machine I found an issue, which could be
> reproduced with the steps bellow. It's not an emergency issue which
> blocks the patch from merging, because we still have a workaround in our
> pocket (stop and start ‘shepherd’ manually on a specific ‘DISPLAY’).
>
> - start ‘shepherd’ manually on ‘127.0.0.1:5’ by typing ‘shepherd’ in a
>   GUI terminal;
> - stop ‘x11-display’ with ‘herd stop x11-display’;
> - start ‘x11-display’ with ‘herd start x11-display :1’;
> - start ‘redshift’ with ‘herd start redshift’.
>
> The ‘redshift’ service starts a ‘redshift’ process.  The
> /proc/REDSHIFT_PID/environ file has a ‘DISPLAY’ variable setted to
> ‘127.0.0.1:5’.

Indeed, good catch!  There was a bug: it’s not enough to call ‘setenv’
because ‘make-forkexec-constructor’ & co. have an explicit
#:environment-variables parameter, which defaults to the environment of
‘shepherd’ when it was started.  (On top of that, setting the
‘default-environment-variables’ SRFI-39 parameter from the ‘start’
method of ‘x11-display’ wouldn’t work because each fiber has its own
dynamic environment…)

I had to make the change below.  Result pushed as commit
08d94fe20eca47b69678b3eced8749dd02c700a4.

Thank you for reviewing and testing!

Ludo’.
diff mbox series

Patch

diff --git a/gnu/home/services/desktop.scm b/gnu/home/services/desktop.scm
index 45a319c0f8..91465bf168 100644
--- a/gnu/home/services/desktop.scm
+++ b/gnu/home/services/desktop.scm
@@ -92,6 +92,11 @@  (define (x11-shepherd-service delay)
 
               (let ((display (or display (find-display #$delay))))
                 (when display
+                  ;; Note: 'make-forkexec-constructor' calls take their
+                  ;; default #:environment-variables value before this service
+                  ;; is started and are thus unaffected by the 'setenv' call
+                  ;; below.  Users of this service have to explicitly query
+                  ;; its value.
                   (setenv "DISPLAY" display))
                 display)))
          (stop #~(lambda (_)
@@ -244,9 +249,20 @@  (define (redshift-shepherd-service config)
          ;; available, and fails to start otherwise.
          (requirement '(x11-display))
 
-         (start #~(make-forkexec-constructor
-                   (list #$(file-append (home-redshift-configuration-redshift config) "/bin/redshift")
-                         "-c" #$config-file)))
+         (modules '((srfi srfi-1)
+                    (srfi srfi-26)))
+         (start #~(lambda _
+                    (fork+exec-command
+                     (list #$(file-append
+                              (home-redshift-configuration-redshift config)
+                              "/bin/redshift")
+                           "-c" #$config-file)
+
+                     ;; Inherit the 'DISPLAY' variable set by 'x11-display'.
+                     #:environment-variables
+                     (cons (string-append "DISPLAY=" (getenv "DISPLAY"))
+                           (remove (cut string-prefix? "DISPLAY=" <>)
+                                   (default-environment-variables))))))
          (stop #~(make-kill-destructor))
          (actions (list (shepherd-configuration-action config-file))))))