Subject: Re: [bug#74542] [PATCH 06/11] guix build: Add ‘--dependents’.

From: Simon Tournier <[email-masked]>
Date: Wed, 27 Nov 2024 20:12:27 +0100

Hi,

On Tue, 26 Nov 2024 at 11:33, Ludovic Courtès <[email-masked]> wrote:

> +@item --dependents[=@var{depth}]
> +@itemx -T [@var{depth}]

Cosmetic: argh, ’-M’ is already taken under “guix build”! Because it
would be nice to have something similar as with “guix graph
--max-depth”.  Yes, both depths does not have the same meaning but
that’s still a ’max depth’ for both cases. ;-)

Well…

> +The list of dependents is computed in the same way as with @command{guix
> +refresh --list-dependent} (@pxref{Invoking guix refresh}).

… “guix refresh” already uses ’-T’ but for another meaning
(--list-transitive).  Therefore, I think something other than ’-T’
should be better.  Maybe ’-D’ since it cannot conflict with “guix
shell”..

> +  -T, --dependents[=N]   build dependents of the following package, up to
> +                         depth N"))

As said, ’-T’ might be confusing with “guix refresh -T” especially when
’dependents’ is more or less connected with “guix refresh -l”.

> +         (option '(#\T "dependents") #f #t
> +                 (lambda (opt name arg result)
> +                   (alist-cons 'dependents
> +                               (or (and=> arg string->number*) +inf.0)
> +                               result)))

Idem.

> +(define (options->things-to-build store opts)

[...]

> -    (map transform (options->things-to-build opts)))
> +    (map transform (options->things-to-build store opts)))

I thought ’store’ could have been hidden, but not.  Since it’s already
passed to ’options->derivations’. :-)

Well, aside the cosmetic, LGTM.

Cheers,
simon