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

[Eric Bavier <ericbavier@gmail.com>]

Ludovic Courtès writes:

>> 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 glad.

>
>> 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?

Yes, I like "dependent" better.

>> +  (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.

OK

>> +         (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?

Yes, that shouldn't be a problem.  Could I ask why the request?

>> +    (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.

I'm fine with moving the mapping to the call site.  I used "package
specification" here because that term is used elsewhere when talking
about a string like "zlib-1.2.7".  I'll change the documentation to be
more clear.

> 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’.

I realize now that the structure isn't necessarily a forest, so yes,
"fold-tree" would be more appropriate.

> Also could you write a few tests for these two?

Sure thing.

> 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?

Because of the use of 'fold-packages' from (gnu packages), putting
dependent-packages into (guix packages) would cause a circular
dependency.  I think I'd also want to make the interface more general if
it were to go somewhere more widely available.

I'll post an updated patch soon.

-- 
Eric Bavier

Please avoid sending me Word or PowerPoint attachments.
See http://www.gnu.org/philosophy/no-word-attachments.html