Subject: Re: [PATCH] services: Let cups-service-type reuse base lp group.

From: Leo Prikler <[email-masked]>
Date: Sat, 16 Jan 2021 20:49:10 +0100

Hi,
Am Samstag, den 16.01.2021, 19:37 +0100 schrieb Ludovic Courtès:
> Hi,
> 
> Leo Prikler <[email-masked]> skribis:
> 
> > * gnu/services/cups.scm (%cups-accounts): Try to use the lp group
> > defined in
> > %base-groups.
> > * gnu/system/shadow.scm (account-activation): Delete duplicate
> > (eq?) users
> > and groups before transforming them to specs and asserting, that
> > names are
> > unique.
> 
> [...]
> 
> >  (define %cups-accounts
> > -  (list (user-group (name "lp") (system? #t))
> > +  (list (or
> > +         ;; The "lp" group should already exist; try to reuse it.
> > +         (find (lambda (group)
> > +                 (and (user-group? group)
> > +                      (string=? (user-group-name group) "lp")))
> > +               %base-groups)
> > +         (user-group (name "lp") (system? #t)))
> >          (user-group (name "lpadmin") (system? #t))
> >          (user-account
> >           (name "lp")
> 
> This bit LGTM, and I think it can be committed in a commit of its
> own.
Will do so once I get my working tree is less dirty.

> > diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
> > index 0538fb1a24..7c57222716 100644
> > --- a/gnu/system/shadow.scm
> > +++ b/gnu/system/shadow.scm
> > @@ -321,13 +321,13 @@ of user '~a' is undeclared")
> >  <user-group> objects.  Raise an error if a user account refers to
> > a undefined
> >  group."
> >    (define accounts
> > -    (filter user-account? accounts+groups))
> > +    (delete-duplicates (filter user-account? accounts+groups)
> > eq?))
> >  
> >    (define user-specs
> >      (map user-account->gexp accounts))
> >  
> >    (define groups
> > -    (filter user-group? accounts+groups))
> > +    (delete-duplicates (filter user-group? accounts+groups) eq?))
> 
> Why use ‘eq?’?  I’d use ‘equal?’, but note that <user-account>
> records
> cannot necessarily be compared with ‘equal?’ because of the thunked
> ‘home-directory’ field (‘equal?’ is meaningless for procedures).
My personal reasoning (and perhaps a rather strong opinion) is, that it
is an error to add duplicate users even if they happen to be equal?. 
eq? is only provided as a way out for the specific case of services,
that need to do so for safety reasons – e.g. cups to not allow
overriding of the lp group if it has been removed from the OS groups
for whichever reason.

Regards,
Leo