Re: [PATCH] gnu: refresh: Add --list-upstream-closure option.

[Ludovic Courtès]

Eric Bavier <address@hidden> skribis:

> When upgrading packages, I found it would be useful, in order to avoid
> breaking builds for hydra and everyone else, to know which packages to
> test building locally before pushing the upgrades.
>
> The attached patch provides this information in the form of a new option
> to the "guix refresh" command.  I thought that would be a nice for the
> functionality because it is already a "developer" command, and the
> use-case I had in mind revolved around upgrading packages.
>
> For the sake of brevity and human consumption, the option doesn't print
> *all* upstream packages, just the "top-level" upstream packages,
> i.e. those whose inputs encompass all other upstream packages.

This looks very useful!

> I'm not sure that the option name or all the terminology I used is
> appropriate, so any comments or suggestions are welcome.

The term “upstream” in the context of this command makes me think of
package maintainers which are considered “upstream” compared to the
distro.

What about “dependent” instead?

> +        (option '(#\l "list-upstream-closure") #f #f
> +                (lambda (opt name arg result)
> +                  (alist-cons 'list-upstream? #t result)))

“list-dependent”?

>    -s, --select=SUBSET    select all the packages in SUBSET, one of
>                           `core' or `non-core'"))
> +  (display (_ "
> +  -l, --list-upstream-closure
> +                         List top-level upstream packages that would need to
                            ^
lowercase

> +(define (upstream-packages packages)
> +  "Return a minimal list of top-level package specifications for packages 
> that
> +should be built in order to test changes made to the packages in
> +PACKAGE-SPECS.  Building the returned packages will ensure that *all* 
> packages
> +that depend, directly or indirectly, on those packages in PACKAGE-SPECS are
> +tested."
> +  (define (package-direct-inputs package)
> +    (append (package-native-inputs package)
> +            (package-inputs package)
> +            (package-propagated-inputs package)))
> +
> +  (let ((inverse-package-dependency-graph

‘inverse-dag’ or ‘dag’ is enough for a local var.  If needed a comment
can clarify what it is.

> +         (fold-packages
> +          (lambda (package forest)
> +            (for-each
> +             (lambda (d)
> +               ;; Insert a tree edge from each of package's inputs to 
> package.
> +               (hash-set! forest d
> +                          (cons package
> +                                (hash-ref forest d '()))))
> +             (map cadr (package-direct-inputs package)))
> +            forest)
> +          (make-hash-table))))

Could you use a vhash here instead of the hash table?

> +    (map package-full-name
> +         (fold-forest-leaves
> +          cons '()
> +          (lambda (node)
> +            (hash-ref inverse-package-dependency-graph node '()))
> +          packages))))

The function is documented as returning “package specifications” so I’d
remove ‘map’ from here and move it to the call site.

> +(define (fold-forest proc init next roots)
> +  "Call (PROC NODE RESULT) for each node in the forest that is reachable from
> +ROOTS, using INIT as the initial value of RESULT.  The order in which nodes
> +are traversed is not specified, however, each node is visited only once, 
> based
> +on an eq? check.  Children of a node to be visited are generated by
> +calling (NEXT NODE), the result of which should be a list of nodes that are
> +connected to NODE in the forest, or '() if NODE is a leaf node."
> +  (let loop ((result init)
> +             (seen vlist-null)
> +             (lst roots))
> +    (match lst
> +      (() result)
> +      ((head . tail)
> +       (if (not (vhash-assq head seen))
> +           (loop (proc head result)
> +                 (vhash-consq head #t seen)
> +                 (append tail (next head)))
> +           (loop result seen tail))))))

What about ‘fold-tree’ instead, and change ‘next’ to ‘children’?

> +(define (fold-forest-leaves proc init next roots)
> +  "Like fold-forest, but call (PROC NODE RESULT) only for leaf nodes."

‘fold-tree-leaves’.

Also could you write a few tests for these two?

I believe ‘upstream-packages’ (renamed to ‘dependent-packages’?) could
be moved to (guix packages) and have a few tests as well.  That’d be
great.

WDYT?

Thanks!

Ludo’.