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

From: Ludovic Courtès <[email-masked]>
Date: Tue, 15 Apr 2025 13:20:23 +0200

Hello Ian,

Ian Eure <[email-masked]> 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’.