diff mbox series

[bug#77266] gnu: Merge xorg configurations when extending.

Message ID 20250326042354.14033-1-ian@retrospec.tv
State New
Headers
Series [bug#77266] gnu: Merge xorg configurations when extending. |

Commit Message

Ian Eure March 26, 2025, 4:23 a.m. UTC 
Configuration for xorg is embedded in the various display-manager
configuration records, and extension support is factored out into the
`handle-xorg-configuration' macro.  However, the extension mechanism replaces
the existing xorg-configuration with the supplied one, making it impossible to
compose configuration from multiple sources.  This patch adds a procedure to
merge two xorg-configuration records, and calls it within
handle-xorg-configuration, allowing the config to be built piecemeal.

* gnu/services/xorg.scm (merge-xorg-configurations): New variable.
(handle-xorg-configuration): Merge xorg configs.

Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba
---
 gnu/services/xorg.scm | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

Comments

Ludovic Courtès March 26, 2025, 5:16 p.m. UTC | #1 
Hi!

Ian Eure <ian@retrospec.tv> skribis:

> Configuration for xorg is embedded in the various display-manager
> configuration records, and extension support is factored out into the
> `handle-xorg-configuration' macro.  However, the extension mechanism replaces
> the existing xorg-configuration with the supplied one, making it impossible to
> compose configuration from multiple sources.  This patch adds a procedure to
> merge two xorg-configuration records, and calls it within
> handle-xorg-configuration, allowing the config to be built piecemeal.
>
> * gnu/services/xorg.scm (merge-xorg-configurations): New variable.
> (handle-xorg-configuration): Merge xorg configs.
>
> Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba

Maybe you can add a word in the relevant part of the manual to give an
idea of how configs are merged.

Apart from, it LGTM.  I can’t think of a scenario where it would break
existing config or where the current setup is preferable.

Thanks!

Ludo’.
Rutherther March 26, 2025, 5:31 p.m. UTC | #2 
Hello, just wondering, wouldn't it make sense to inherit from the
previous a or b in case someone added a field to xorg-configuration and
forgot to change this service?

Or is there some kind of a smart way how we can in Guix ensure stuff
like that is properly checked? I can see someone forgetting about stuff
like that...

Rutherther
Ian Eure March 26, 2025, 7:15 p.m. UTC | #3 
Hi Rutherther,

Rutherther <rutherther@ditigal.xyz> writes:

> Hello, just wondering, wouldn't it make sense to inherit from 
> the
> previous a or b in case someone added a field to 
> xorg-configuration and
> forgot to change this service?

I considered using inherit, but since every field has to be set to 
the combined values of both records, saw no value in doing so.  In 
the case you outline, both inheriting and not will produce an 
incorrect result -- inheriting will discard all but one 
user-specified’s value, while not inheriting discards all 
user-specified value and uses the default.  Both are wrong, but I 
think the latter is more likely to get noticed (therefore fixed).

Thanks,
  -- Ian
Ian Eure March 26, 2025, 7:15 p.m. UTC | #4 
Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Ian Eure <ian@retrospec.tv> skribis:
>
>> Configuration for xorg is embedded in the various 
>> display-manager
>> configuration records, and extension support is factored out 
>> into the
>> `handle-xorg-configuration' macro.  However, the extension 
>> mechanism replaces
>> the existing xorg-configuration with the supplied one, making 
>> it impossible to
>> compose configuration from multiple sources.  This patch adds a 
>> procedure to
>> merge two xorg-configuration records, and calls it within
>> handle-xorg-configuration, allowing the config to be built 
>> piecemeal.
>>
>> * gnu/services/xorg.scm (merge-xorg-configurations): New 
>> variable.
>> (handle-xorg-configuration): Merge xorg configs.
>>
>> Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba
>
> Maybe you can add a word in the relevant part of the manual to 
> give an
> idea of how configs are merged.

Definitely, do you want a look at the wording, or should I write 
that up and merge?

Will send a v2 later today.

Thanks,
  -- Ian
Ian Eure April 19, 2025, 7:07 p.m. UTC | #5 
Hi Rutherther,

Rutherther <rutherther@ditigal.xyz> writes:

> Hi,
>
> Ian Eure <ian@retrospec.tv> writes:
>
>> Any other feedback on this?  Does the manual wording look good?
>
> I am wondering about the "and must provide a complete 
> configuration."
> part in documentation. Is that really so after this patch? You 
> can still
> extend with other services, no? So it doesn't seem right to me 
> it would
> be necessary to use only set-xorg-configuration, it can be 
> combined with
> custom service that will append parts of the config.
> At least if I am not missing anything here.
>
> Additionally, I am wondering why do we have that limitation of 
> just one
> usage of set-xorg-configuration. I suppose the name 
> 'set-xorg-configuration'
> implies you set it, not append it, etc., but that's not really 
> true
> after changes from this patch. The limitation comes from the 
> name of the service
> that is created.

You can mix `set-xorg-configuration' and service extensions, but 
cannot call `set-xorg-configuration' more than once.  As your 
note, it has a fixed service name, and having more than one 
service with the same name causes an error.

> So why not allow a new key/optional argument to set name of
> the service, so that it can be used multiple times, and default 
> it to
> 'set-xorg-configuration? On the other hand this is probably not 
> so
> important, I personally don't really see any gain in this
> set-xorg-configuration when user's can just extend the 
> appropriate
> service instead, so it doesn't seem to me that big of a deal to 
> change it.

I agree with the latter half of your message: allowing a service 
name argument makes `set-xorg-configuration' basically the same as 
writing a `simple-service' definition.  I don’t think it’s worth 
doing.

I’ll clarify the docs and send a v3.

A thing I dislike about all this stuff is how the display managers 
carry the xorg configuration, vs. having an xorg service which the 
DMs depend on.  I suppose it’s the way it is because the DMs spawn 
the X server process, but it feels like it should be possible to 
disentangle things at least a bit more than they are now.  The 
current setup also seems to preclude usecases like running gdm 
locally, while using sddm as an XDMCP greeter for other systems.

Thanks,

  -- Ian
Ian Eure April 22, 2025, 12:15 a.m. UTC | #6 
Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> Ian Eure <ian@retrospec.tv> writes:
>
>> Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba
>> +   (append
>> +       (list (service gdm-service-type)
>> +          %xorg-intel-service
>> +          %xorg-keyboard-service)
>
> Nitpick: indentation is off.  :-)

Do you have / know of a setup for editing Guile inside the 
documentation?  I’ve been editing in a *scheme* scratch buffer in 
Emacs and pasting back into the docs, but this clearly doesn’t 
work very well.


> I think ‘delete-duplicates’ should be used for drivers, modules, 
> and
> fonts, but not for ‘extra-config’ (which is merely a string).
>
> Does that make sense?

Makes perfect sense, updated patch coming soon.

Thanks,
  -- Ian
Ludovic Courtès April 22, 2025, 1:18 p.m. UTC | #7 
Ian Eure <ian@retrospec.tv> writes:

> Do you have / know of a setup for editing Guile inside the
> documentation?  I’ve been editing in a *scheme* scratch buffer in
> Emacs and pasting back into the docs, but this clearly doesn’t work
> very well.

That’s what I do as well.  Low-tech, but it’s also useful to test the
example beforehand.

Ludo’.
Ian Eure April 24, 2025, 11:58 p.m. UTC | #8 
Hi Ludo’,

Ludovic Courtès <ludo@chbouib.org> writes:

> Hi,
>
> Ian Eure <ian@retrospec.tv> writes:
>
>>>   (append (list (service gdm-service-type)
>>>                 %xorg-intel-service
>>>                 …)
>>>           @dots{})
>>
>> Can make these changes, but I’m curious why (append (list ...) 
>> ...) is
>> preferred over cons*.  I default to the latter, because I like 
>> the
>> removal of the extra level of indent.
>
> In most of the examples in the documentation, we went for 
> ‘append’
> because it less obscure to a newcomer than ‘cons*’ (or ‘cons’, 
> even).
>
> That’s also what the installer generates, and ‘guix home import’ 
> I
> believe.

Makes sense.  I sent a v4 with these changes and some doc 
rewording.

Thanks,

  -- Ian
Ian Eure May 11, 2025, 4:41 p.m. UTC | #9 
Hi all,

Any other feedback on this, or does it look ready to push?

Thanks,
  -- Ian
Ian Eure May 25, 2025, 4:57 p.m. UTC | #10 
Migrated this to a Codeberg PR: 
https://codeberg.org/guix/guix/pulls/21
diff mbox series

Patch

diff --git a/gnu/services/xorg.scm b/gnu/services/xorg.scm
index bef05b9bb9..dbd1513cc8 100644
--- a/gnu/services/xorg.scm
+++ b/gnu/services/xorg.scm
@@ -209,6 +209,34 @@  (define-record-type* <xorg-configuration>
   (server-arguments xorg-configuration-server-arguments ;list of strings
                     (default %default-xorg-server-arguments)))
 
+(define (merge-xorg-configurations a b)
+  (let ((configs (list a b)))
+    (xorg-configuration
+     (modules
+      (delete-duplicates (append-map xorg-configuration-modules configs)))
+     (fonts
+      (delete-duplicates (append-map xorg-configuration-fonts configs)))
+     (drivers
+      (delete-duplicates (append-map xorg-configuration-drivers configs)))
+     (resolutions
+      (delete-duplicates (append-map xorg-configuration-resolutions configs)))
+     (extra-config (append-map xorg-configuration-extra-config configs))
+     ;; Prefer the more recently set layout.
+     (keyboard-layout (or (xorg-configuration-keyboard-layout b)
+                          (xorg-configuration-keyboard-layout a)
+                          #f))
+     (server
+      ;; Prefer the non-default server.
+      (if (eq? xorg-server (xorg-configuration-server a))
+          (xorg-configuration-server b)
+          (xorg-configuration-server a)))
+     (server-arguments
+      ;; Prefer the non-default arguments.
+      (if (eq? %default-xorg-server-arguments
+             (xorg-configuration-server-arguments a))
+          (xorg-configuration-server-arguments b)
+          (xorg-configuration-server-arguments a))))))
+
 (define (xorg-configuration->file config)
   "Compute an Xorg configuration file corresponding to CONFIG, an
 <xorg-configuration> record."
@@ -628,10 +656,7 @@  (define-syntax handle-xorg-configuration
     ((_ configuration-record service-type-definition)
      (service-type
        (inherit service-type-definition)
-       (compose (lambda (extensions)
-                  (match extensions
-                    (() #f)
-                    ((config . _) config))))
+       (compose (cut reduce merge-xorg-configurations #f <>))
        (extend (lambda (config xorg-configuration)
                  (if xorg-configuration
                      (configuration-record