[bug#77266,v2] gnu: Merge xorg configurations when extending.
Commit Message
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.
* doc/guix.texi (X Window): Document xorg-configuration composition.
Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba
---
doc/guix.texi | 34 ++++++++++++++++++++++++++++++++--
gnu/services/xorg.scm | 33 +++++++++++++++++++++++++++++----
2 files changed, 61 insertions(+), 6 deletions(-)
Comments
Any other feedback on this? Does the manual wording look good?
-- Ian
Hello Ian,
Ian Eure <ian@retrospec.tv> writes:
> 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.
> * doc/guix.texi (X Window): Document xorg-configuration composition.
>
> Change-Id: I20e9db911eef5d4efe98fdf382f3084e4defc1ba
> +@lisp
> +(define %xorg-intel-service
> + (simple-service
> + 'xorg-intel
> + gdm-service-type
> + (xorg-configuration
> + (modules (list xf86-video-intel))
> + (drivers '("intel")))))
> +
> +(define %xorg-keyboard-service
> + (simple-service
> + 'xorg-keyboard
> + gdm-service-type
> + (xorg-configuration (keyboard-layout keyboard-layouut))))
Could you fix the indentation and add short comments above the
definitions?
> +(operating-system
> + (services
> + (cons*
> + (service gdm-service-type)
> + %xorg-intel-service
Rather:
(append (list (service gdm-service-type)
%xorg-intel-service
…)
@dots{})
> +(define (merge-xorg-configurations a b)
> + (let ((configs (list b a))) ; Prefer later configurations.
> + (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)))
Something I had overlooked: this assumes that elements of this list can
be compared with ‘equal?’. Typically, these are <file-append> records
that refer to <package> records, and those cannot be usefully compared
with ‘equal?’ (it’s effectively equivalent to ‘eq?’ in the case of
<package> records, but more expensive).
So, what about removing ‘delete-duplicates’ from here, and instead…
> (define (xorg-configuration->file config)
> "Compute an Xorg configuration file corresponding to CONFIG, an
… adding ‘delete-duplicates’ calls in here, on the build side.
WDYT?
Thanks,
Ludo’.
Ludovic Courtès <ludo@chbouib.org> writes:
> Hello Ian,
>
> Ian Eure <ian@retrospec.tv> writes:
>
>> +(define %xorg-keyboard-service
>> + (simple-service
>> + 'xorg-keyboard
>> + gdm-service-type
>> + (xorg-configuration (keyboard-layout keyboard-layouut))))
>
> Could you fix the indentation and add short comments above the
> definitions?
Definitely.
>> +(operating-system
>> + (services
>> + (cons*
>> + (service gdm-service-type)
>> + %xorg-intel-service
>
> Rather:
>
> (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.
> Something I had overlooked: this assumes that elements of this
> list can
> be compared with ‘equal?’. Typically, these are <file-append>
> records
> that refer to <package> records, and those cannot be usefully
> compared
> with ‘equal?’ (it’s effectively equivalent to ‘eq?’ in the case
> of
> <package> records, but more expensive).
>
> So, what about removing ‘delete-duplicates’ from here, and
> instead…
>
>> (define (xorg-configuration->file config)
>> "Compute an Xorg configuration file corresponding to CONFIG,
>> an
>
> … adding ‘delete-duplicates’ calls in here, on the build side.
Good catch, makes sense to me. Will send a v3.
Thanks,
-- Ian
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.
Ludo’.
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. 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.
Best regards,
Rutherther
@@ -138,6 +138,7 @@ Copyright @copyright{} 2024 45mg@*
Copyright @copyright{} 2025 Sören Tempel@*
Copyright @copyright{} 2025 Rostislav Svoboda@*
Copyright @copyright{} 2025 Zacchaeus@*
+Copyright @copyright{} 2025 Ian Eure@*
Permission is granted to copy, distribute and/or modify this document
under the terms of the GNU Free Documentation License, Version 1.3 or
@@ -24865,8 +24866,37 @@ Tell the log-in manager (of type @var{login-manager-service-type}) to use
@var{config}, an @code{<xorg-configuration>} record.
Since the Xorg configuration is embedded in the log-in manager's
-configuration---e.g., @code{gdm-configuration}---this procedure provides a
-shorthand to set the Xorg configuration.
+configuration---e.g., @code{gdm-configuration}---this procedure provides
+a shorthand to set the Xorg configuration.
+
+Note that @code{set-xorg-configuration} can only be used once, and must
+provide a complete configuration. If you know the service-type of the
+log-in manager, you can compose a configuration from smaller pieces:
+
+@lisp
+(define %xorg-intel-service
+ (simple-service
+ 'xorg-intel
+ gdm-service-type
+ (xorg-configuration
+ (modules (list xf86-video-intel))
+ (drivers '("intel")))))
+
+(define %xorg-keyboard-service
+ (simple-service
+ 'xorg-keyboard
+ gdm-service-type
+ (xorg-configuration (keyboard-layout keyboard-layouut))))
+
+(operating-system
+ (services
+ (cons*
+ (service gdm-service-type)
+ %xorg-intel-service
+ %xorg-keyboard-service
+ %base-services))
+ ...)
+@end lisp
@end deffn
@deffn {Procedure} xorg-start-command [config]
@@ -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 b a))) ; Prefer later configurations.
+ (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