GNU bug report logs - #77266
[PATCH] gnu: Merge xorg configurations when extending.

Previous Next

Full log

View this message in rfc822 format

From: Ian Eure <ian <at> retrospec.tv>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 77266 <at> debbugs.gnu.org
Subject: [bug#77266] [PATCH v4 2/2] gnu: set-xorg-configuration: Be smarter finding the display-manager.
Date: Mon, 12 May 2025 06:54:05 -0700

Hi Ludo’,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Ian Eure <ian <at> retrospec.tv> writes:
>
>> * gnu/services/desktop.scm (desktop-services-for-system): 
>> Export.
>> * gnu/services/desktop.scm (set-xorg-configuration): Look for a
>> display-manager service in (desktop-services-for-system).
>> * gnu/services/desktop.scm (set-xorg-configuration): Remove 
>> TODO comment.
>>
>> Change-Id: Iceda2d13d16435517cd92bcb49e6937ed096f392
>> ---
>>  gnu/services/desktop.scm |  1 +
>>  gnu/services/xorg.scm    | 25 ++++++++++++++++++-------
>>  2 files changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/gnu/services/desktop.scm 
>> b/gnu/services/desktop.scm
>> index fe034cfa8f..f7fb3c5bf0 100644
>> --- a/gnu/services/desktop.scm
>> +++ b/gnu/services/desktop.scm
>> @@ -199,6 +199,7 @@ (define-module (gnu services desktop)
>>              seatd-configuration
>>              seatd-service-type
>>  
>> +            desktop-services-for-system
>
> This is unnecessary since ‘%desktop-services’ behaves in the 
> same way.
>
>> +            xorg-configuration-service-type
>
> I would not export it (the name is confusing and it’s not used 
> outside).

I agree, I’ll change this.


>> +(define (xorg-configuration-service? s)
>> +  "Returns #t if @var{s} is a service carrying xorg 
>> configuration."
>> +  (let ((val (service-value s)))
>> +    (and (record? val)
>> +         (memq 'xorg-configuration
>> +                 (record-type-fields (record-type-descriptor 
>> val))))))
>
> Please don’t use introspection on records: it may sound 
> appealing, but
> it would make refactoring next-to-impossible since it becomes 
> very hard
> to know how records are used.
>
> Instead just do:
>
>   (and (or (sddm-configuration? value) …)
>        value)

Hardcoding the configuration types here creates a maintenance 
burden, where adding a DM service requires updating a list of DM 
services, which is likely to be a source of bugs.  And since the 
`xorg-configuration' field is added programmatically by 
`handle-xorg-configuration', it seems reasonable to rely on here, 
as it’s unlikely to change in a refactor.  WDYT?

Is there another way to programmatically determine whether a given 
service carries an xorg-configuration without introspection?


>> +(define (xorg-configuration-service-type services)
>> +  "Return the service type of the first service in 
>> @var{services}
>> +which carries xorg configuration. "
>> +  (let ((lmst (find xorg-configuration-service? services)))
>> +    (when lmst (service-kind lmst))))
>
> Rather:
>
>   (let ((xorg (find …)))
>     (and=> xorg service-kind))
>
> … otherwise the procedure can return *unspecified*, leading to a 
> type
> error down the road.

Will do.

> But hmm, sorry for overlooking it but I’m not convinced by this 
> patch.
> I would tend to keep just patch 1/2.

I’m okay with dropping this patch from the series, but would 
prefer to include it.  The existing logic in 
`set-xorg-configuration' is crude and has a TODO to improve it. 
While this patch doesn’t solve 100% of the problem (this is 
impossible without signigiantly reworking every DM), it does 
improve the situation and provide some helpful facilities for the 
future.

Thanks,
 -- Ian

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.