Re: [PATCH] guix: scripts: Add --dependencies=PACKAGE commmand

[Ludovic Courtès]

Amirouche Boubekki <address@hidden> skribis:

> From fca3a1020af9d4abe71ab8ab4e2a52011688f272 Mon Sep 17 00:00:00 2001
> From: amz3 <address@hidden>
> Date: Sat, 20 Jun 2015 18:23:26 +0200
> Subject: [PATCH] guix: scripts: add --dependencies=PACKAGE command
>
> exemple usage:
>
>   guix package --dependencies=qsynth | dot -Tpng > qsynth-deps.png
>
> * guix/scripts/package.scm: add --dependencies command
> * guix/scripts/dependencies.scm: graph datastructure and helpers

Thanks for working on this, this is a useful tool!

Please take some time to read the “Contributing” section of the manual,
and in particular the part about commit logs and coding style.  You’ll
quickly notice several things that need to be adjusted.  ;-)

Here’s a quick review:

> +++ b/guix/scripts/dependencies.scm
> @@ -0,0 +1,312 @@
> +;; FIXME: copyright

Indeed.

> +;; FIXME: Taken from Guile, should be in (srfi srfi-99)
> +;;        adapted to make it possible to declare record type like `<abc>' 
> and keep
> +;;        field accessor bracket free. record name *must* have brackets or 
> everything
> +;;        is broken
> +;;
> +;; Usage:
> +;;
> +;;   (define-record-type <abc> field-one field-two)
> +;;   (define zzz (make-abc 1 2))
> +;;   (abc-field-one zzz) ;; => 1
> +;;
> +;; FIXME: maybe this is less useful than the immutable record of (srfi 
> srfi-9 gnu)
> +;;        Right I will use `set-field` and `set-fields`
> +(define-syntax define-record-type*

Please don’t add another ‘define-record-type’; the one from SRFI-9 will
be good enough here.

> +;;;
> +;;; Store
> +;;;
> +;;; Memory bound immutable association
> +;;;
> +
> +
> +;; XXX: It's assumed that keys are strings.
> +;;
> +;; This replace scheme assoc, because:
> +;; 1) there is no immutable `assoc-set` in scheme
> +;; 2) `acons` (and friends) can replace `assoc-set` but memory will grow 
> without bound
> +;; 3) `assoc-ref` (and friends) always return `#f` when no values is found

That’s not a sufficient argument: one could just

  (define (assoc-set alist key value)
    (alist-cons key value
                (alist-delete key alist)))

Regardless, it may be that vhashes would work better here.

> +;;;
> +;;; Graph
> +;;;
> +
> +(define-record-type* <graph> label nodes edges properties)

[...]

> +(define (graph-get-uid store)
> +  (define CHARS "AZERTYUIOPQSDFGHJKLMWXCVBN1029384756")
> +
> +  (define (random-id)
> +    (let loop ((count 4)
> +               (id ""))
> +      (if (eq? count 0)
> +          id
> +          (loop (1- count) (format #f "~a~a" id (string-ref CHARS (random 
> 36)))))))
> +
> +  (let loop ()
> +    (let ((id (random-id)))
> +      (if (null? (store-ref store id))
> +          id
> +          (loop)))))

What about:

  (define (graph-unique-identifier graph)
    "Return a unique identifier for GRAPH for use in the ‘dot’ output.”
    (number->string (object-address graph) 16))

?

> +        (if (package? (cadar packages))
> +            (let ((dependency (package-name (cadar packages))))
> +              (let*-values (((graph package-uid) (maybe-create-node graph 
> (package-name package)))
> +                            ((graph dependency-uid) (maybe-create-node graph 
> dependency)))
> +                (let* ((label (string-append (package-name (cadar packages)) 
> "--(depends)-->" dependency))
> +                       (graph (maybe-create-edge graph package-uid label 
> dependency-uid))
> +                       (graph (package-dependency-graph (cadar packages) 
> graph)))
> +                (loop (cdr packages) graph))))
> +            (loop (cdr packages) graph)))))

car, cdr, cadar etc. are banned.  Please use ‘match’ or some specific
accessor (it may be that a record would be more appropriate here?)

> +(define (graph-dot graph port)

‘graph->dot’

> +                   ;; (let-values (((name version)
> +                   ;;               (package-name->name+version arg)))
> +                     
> +                   ;;   ;; (display-dependencies (find-packages-by-name 
> name))
> +                   ;;   (values (cons #f result) #f))))

Leftover.

More importantly, I think this should be in a separate command, or maybe
as an option in ‘guix gc’ since ‘guix gc’ already has --references & co.

WDYT?

I think the graph part should be abstracted so that it can work on (1)
packages, (2) bags, and (3) derivations.  Ideally users would be able to
choose which of these they want to represent, from coarse-grain to
fine-grain.  (That part could be implemented as a later patch, but the
framework has to be prepared for that.)

Also, I’m not completely sure a separate graph representation (<node>,
<edge>, and <graph>) is needed.  Wouldn’t it be sufficient to work
directly on packages objects (or bags or derivations)?

Thanks,
Ludo’.