Re: [PATCH] guix: refresh: Add --list-dependent option.

[Ludovic Courtès]

Eric Bavier <address@hidden> skribis:

> From 57aa3ac9bf0a58c0981fbf729dd12756dedd5831 Mon Sep 17 00:00:00 2001
> From: Eric Bavier <address@hidden>
> Date: Thu, 17 Jul 2014 12:42:01 -0500
> Subject: [PATCH] guix: refresh: Add --list-dependent option.
>
> * guix/utils.scm (fold-tree, fold-tree-leaves): New functions.
> * tests/utils.scm: Add tests for fold-tree and fold-tree-leaves.
> * guix/packages.scm (package-direct-inputs): New procedure.
> * gnu/packages.scm (vhash-refq, package-dependencies, 
> package-direct-dependents)
>   (package-transitive-dependents, package-covering-dependents): New 
> procedures.
> * guix/scripts/refresh.scm (%options, show-help, guix-refresh): Add
>   --list-dependent option.

Overall looks good to me.

[...]

> +            package-dependencies

I would keep this one private.

> +     (fold-packages
> +      (lambda (package dag)
> +        (fold
> +         (lambda (in d)
> +           ;; Insert a graph edge from each of package's inputs to package.
> +           (vhash-consq in
> +                        (cons package (vhash-refq d in '()))
> +                        (vhash-delq in d)))
> +         dag
> +         (map cadr (package-direct-inputs package))))
> +      vlist-null))))

Please replace ‘map cadr’ with:

  (match (package-direct-inputs package)
    (((labels packages . _) ...)
     packages))

> +(define (package-direct-dependents . packages)
> +  "Return a list of packages that directly depend on the packages in

“Return a list of packages of the distribution” (to make it clear that
it doesn’t work with any package.)

Also make ‘packages’ a normal parameter rather than a rest parameter.

> +(define (package-transitive-dependents . packages)

Likewise.

> +(define (package-covering-dependents . packages)

Likewise.

> +  (display (_ "
> +  -l, --list-dependent   list top-level dependent packages that would need to
> +                         be rebuilt as a result of upgrading PACKAGE...."))
                                                                          ^
Extra period here.

Please add it to guix.texi as well.

> +          (format (current-output-port)
> +                  (N_ "No dependents other than itself: ~3*~{~a~}~%"
> +                      (N_ "Building the following package ensures ~*~d \
> +dependent packages are rebuilt: ~{~a~^ ~}~%"
> +                          "Building the following ~d packages would ensure 
> ~d \
> +dependent packages are rebuilt: ~{~a~^ ~}~%"
> +                          (length rebuilds))
> +                      total-rebuilt)
> +                  (length rebuilds) total-rebuilt rebuilds
> +                  (map package-full-name packages))))

Add “would” in the first message.

The message look good to me.

Be aware though that there can be subtle situations that trigger
rebuilds, and are not addressed here (which is a fine approximation
IMO.)  For instance, implicit inputs are not taken into account.  And
there are things like reuse of sources, as in base.scm where gcc uses
(package-source gmp) and the likes.

Perhaps there should be a sentence of warning in the manual.

Lastly, could you make the util.scm changes a separate patch?

Then I think we’ll be all set.

Thanks!

Ludo’.