[3/7] import cran: description->package: Also return package dependencies.

Message ID 1464018008-1767-4-git-send-email-ricardo.wurmus@mdc-berlin.de
State New
Headers

Commit Message

Ricardo Wurmus May 23, 2016, 3:40 p.m. UTC
  * guix/import/cran.scm (description->package): Return package
  dependencies in addition to generated package expression.
---
 guix/import/cran.scm | 58 ++++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 27 deletions(-)
  

Comments

Ludovic Courtès May 30, 2016, 8:51 a.m. UTC | #1
Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/import/cran.scm (description->package): Return package
>   dependencies in addition to generated package expression.

What would you think of making it return a SRFI-41 stream of packages
instead?  Or maybe two values: the package asked for, and the stream
containing its dependencies.  That would hide the package lookup
machinery.

Ludo’.
  
Ricardo Wurmus June 14, 2016, 3:49 p.m. UTC | #2
Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>
>> * guix/import/cran.scm (description->package): Return package
>>   dependencies in addition to generated package expression.
>
> What would you think of making it return a SRFI-41 stream of packages
> instead?  Or maybe two values: the package asked for, and the stream
> containing its dependencies.  That would hide the package lookup
> machinery.

That’s a very good idea!  I’ll play around with this and submit a new
patch.

~~ Ricardo
  
Ricardo Wurmus Aug. 5, 2016, 4:03 p.m. UTC | #3
Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>>
>>> * guix/import/cran.scm (description->package): Return package
>>>   dependencies in addition to generated package expression.
>>
>> What would you think of making it return a SRFI-41 stream of packages
>> instead?  Or maybe two values: the package asked for, and the stream
>> containing its dependencies.  That would hide the package lookup
>> machinery.
>
> That’s a very good idea!  I’ll play around with this and submit a new
> patch.

I’ve been playing with this for a while and although it looks prettier
to a user it has some disadvantages to use streams here.  The first is
that it becomes more difficult to avoid duplicate work.  The second is
that we can no longer easily generate an order of package expressions
from leaf nodes to the root.

Consider this case where “plotly” should be imported.  The new inputs
look like this:

 plotly  : (ggplot2 digest)
 ggplot2 : (digest mass)
 digest  : ()
 mass    : ()

The generated stream may look like this:

  (plotly ggplot2 digest mass digest)

This happens because we only know about the next set of inputs upon
evaluation of the stream elements.  Of course there are ways to fix
this, but they involve extending or wrapping “cran->guix-package” to
make sure that we keep track of top-level dependencies as we traverse
the list of dependencies recursively.

Maybe I’m doing this wrong?


I rewrote description->package to essentially do this:

    (stream-cons the-package-expression
     (stream-concat
      (let ((unpackaged (unpackaged-dependencies
                         propagate guix-name)))
        (stream-map (cran->guix-package name repository)
                    (list->stream unpackaged)))))

I.e. the first element of the stream is the imported package expression;
the tail is a concatenation of streams of packages with their
dependencies just like this one.  Here’s the stream before
concatenation:

  (plotly (ggplot2 (digest ()) (mass ()))
          (digest ()))

To render this in the correct order we’d have to turn this tree inside
out.  Maybe this isn’t so well-suited to a representation as a stream
after all.  To turn it inside out we have to evaluate the whole tree
anyway.

What do you think?  Is it worth expressing this as a stream?  If so, do
you have any recommendations on how to approach this?

Or would it be okay to let “description->package” return two values: a
package expression and a list of unpackaged package names, which can
then be processed by a separate procedure?  (That’s what I submitted
earlier.)

~~ Ricardo
  
Ludovic Courtès Aug. 29, 2016, 3:20 p.m. UTC | #4
Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> writes:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>>>
>>>> * guix/import/cran.scm (description->package): Return package
>>>>   dependencies in addition to generated package expression.
>>>
>>> What would you think of making it return a SRFI-41 stream of packages
>>> instead?  Or maybe two values: the package asked for, and the stream
>>> containing its dependencies.  That would hide the package lookup
>>> machinery.
>>
>> That’s a very good idea!  I’ll play around with this and submit a new
>> patch.
>
> I’ve been playing with this for a while and although it looks prettier
> to a user it has some disadvantages to use streams here.  The first is
> that it becomes more difficult to avoid duplicate work.  The second is
> that we can no longer easily generate an order of package expressions
> from leaf nodes to the root.
>
> Consider this case where “plotly” should be imported.  The new inputs
> look like this:
>
>  plotly  : (ggplot2 digest)
>  ggplot2 : (digest mass)
>  digest  : ()
>  mass    : ()
>
> The generated stream may look like this:
>
>   (plotly ggplot2 digest mass digest)
>
> This happens because we only know about the next set of inputs upon
> evaluation of the stream elements.  Of course there are ways to fix
> this, but they involve extending or wrapping “cran->guix-package” to
> make sure that we keep track of top-level dependencies as we traverse
> the list of dependencies recursively.
>
> Maybe I’m doing this wrong?
>
>
> I rewrote description->package to essentially do this:
>
>     (stream-cons the-package-expression
>      (stream-concat
>       (let ((unpackaged (unpackaged-dependencies
>                          propagate guix-name)))
>         (stream-map (cran->guix-package name repository)
>                     (list->stream unpackaged)))))
>
> I.e. the first element of the stream is the imported package expression;
> the tail is a concatenation of streams of packages with their
> dependencies just like this one.  Here’s the stream before
> concatenation:
>
>   (plotly (ggplot2 (digest ()) (mass ()))
>           (digest ()))
>
> To render this in the correct order we’d have to turn this tree inside
> out.  Maybe this isn’t so well-suited to a representation as a stream
> after all.  To turn it inside out we have to evaluate the whole tree
> anyway.
>
> What do you think?  Is it worth expressing this as a stream?  If so, do
> you have any recommendations on how to approach this?
>
> Or would it be okay to let “description->package” return two values: a
> package expression and a list of unpackaged package names, which can
> then be processed by a separate procedure?  (That’s what I submitted
> earlier.)

I think you should go ahead with what you proposed if the other option
sounds like a headache.  Sorry for blocking you for this long!

In theory it would be possible to do something like:

  (define (description->package repo meta)
    (stream-cons `(package …)
                 (stream-unfold (lambda (state)
                                  (description->package
                                   repo
                                   (first-dependency state)))
                                (lambda (state)
                                  (done? state))
                                (lambda (state)
                                  (next-dependency state))
                                (make-state propagate (setq)))))

… where the state is roughly a pair containing the list of next
dependencies and the set of already visited dependencies (to avoid
duplicates).

But again, this is probably easier said than done, so no problem with
keeping the initial option.

Thanks!

Ludo’.
  
Ricardo Wurmus Aug. 31, 2016, 10:39 a.m. UTC | #5
Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>
>> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> writes:
>>
>>> Ludovic Courtès <ludo@gnu.org> writes:
>>>
>>>> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>>>>
>>>>> * guix/import/cran.scm (description->package): Return package
>>>>>   dependencies in addition to generated package expression.
>>>>
>>>> What would you think of making it return a SRFI-41 stream of packages
>>>> instead?  Or maybe two values: the package asked for, and the stream
>>>> containing its dependencies.  That would hide the package lookup
>>>> machinery.
>>>
>>> That’s a very good idea!  I’ll play around with this and submit a new
>>> patch.
>>
>> I’ve been playing with this for a while and although it looks prettier
>> to a user it has some disadvantages to use streams here.  The first is
>> that it becomes more difficult to avoid duplicate work.  The second is
>> that we can no longer easily generate an order of package expressions
>> from leaf nodes to the root.
>>
>> Consider this case where “plotly” should be imported.  The new inputs
>> look like this:
>>
>>  plotly  : (ggplot2 digest)
>>  ggplot2 : (digest mass)
>>  digest  : ()
>>  mass    : ()
>>
>> The generated stream may look like this:
>>
>>   (plotly ggplot2 digest mass digest)
>>
>> This happens because we only know about the next set of inputs upon
>> evaluation of the stream elements.  Of course there are ways to fix
>> this, but they involve extending or wrapping “cran->guix-package” to
>> make sure that we keep track of top-level dependencies as we traverse
>> the list of dependencies recursively.
>>
>> Maybe I’m doing this wrong?
>>
>>
>> I rewrote description->package to essentially do this:
>>
>>     (stream-cons the-package-expression
>>      (stream-concat
>>       (let ((unpackaged (unpackaged-dependencies
>>                          propagate guix-name)))
>>         (stream-map (cran->guix-package name repository)
>>                     (list->stream unpackaged)))))
>>
>> I.e. the first element of the stream is the imported package expression;
>> the tail is a concatenation of streams of packages with their
>> dependencies just like this one.  Here’s the stream before
>> concatenation:
>>
>>   (plotly (ggplot2 (digest ()) (mass ()))
>>           (digest ()))
>>
>> To render this in the correct order we’d have to turn this tree inside
>> out.  Maybe this isn’t so well-suited to a representation as a stream
>> after all.  To turn it inside out we have to evaluate the whole tree
>> anyway.
>>
>> What do you think?  Is it worth expressing this as a stream?  If so, do
>> you have any recommendations on how to approach this?
>>
>> Or would it be okay to let “description->package” return two values: a
>> package expression and a list of unpackaged package names, which can
>> then be processed by a separate procedure?  (That’s what I submitted
>> earlier.)
>
> I think you should go ahead with what you proposed if the other option
> sounds like a headache.  Sorry for blocking you for this long!

No problem.  I would really like to use streams, so it’s worth the
wait.  Can’t avoid the headache :)

> In theory it would be possible to do something like:
>
>   (define (description->package repo meta)
>     (stream-cons `(package …)
>                  (stream-unfold (lambda (state)
>                                   (description->package
>                                    repo
>                                    (first-dependency state)))
>                                 (lambda (state)
>                                   (done? state))
>                                 (lambda (state)
>                                   (next-dependency state))
>                                 (make-state propagate (setq)))))
>
> … where the state is roughly a pair containing the list of next
> dependencies and the set of already visited dependencies (to avoid
> duplicates).

That’s a good hint.  “stream-unfold” makes my head spin, to be honest.

> But again, this is probably easier said than done, so no problem with
> keeping the initial option.

I’ll read the documentation for “stream-unfold” again and try once more.
If I really cannot make it I’ll go ahead with the initial
implementation and submit a new patch set.

~~ Ricardo
  
Ludovic Courtès Sept. 1, 2016, 11:50 a.m. UTC | #6
Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> In theory it would be possible to do something like:
>>
>>   (define (description->package repo meta)
>>     (stream-cons `(package …)
>>                  (stream-unfold (lambda (state)
>>                                   (description->package
>>                                    repo
>>                                    (first-dependency state)))
>>                                 (lambda (state)
>>                                   (done? state))
>>                                 (lambda (state)
>>                                   (next-dependency state))
>>                                 (make-state propagate (setq)))))
>>
>> … where the state is roughly a pair containing the list of next
>> dependencies and the set of already visited dependencies (to avoid
>> duplicates).
>
> That’s a good hint.  “stream-unfold” makes my head spin, to be honest.

I had that feeling when I first met ‘unfold’, but my head has kept
spinning since then so I’m fine.  ;-)

Here’s an example that should probably be added to the Guile manual:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,use(srfi srfi-1)
scheme@(guile-user)> (unfold (lambda (x) (> x 10))
			     (lambda (x) (* 2 x))
			     1+
			     0)
$2 = (0 2 4 6 8 10 12 14 16 18 20)
--8<---------------cut here---------------end--------------->8---

Ludo’.
  

Patch

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 0be5346..ff9dbd3 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -23,6 +23,7 @@ 
   #:use-module ((ice-9 rdelim) #:select (read-string))
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
+  #:use-module (ice-9 receive)
   #:use-module (guix http-client)
   #:use-module (guix hash)
   #:use-module (guix store)
@@ -177,33 +178,36 @@  from the alist META, which was derived from the R package's DESCRIPTION file."
                        (_ #f)))
          (tarball    (with-store store (download-to-store store source-url)))
          (sysdepends (map string-downcase (listify meta "SystemRequirements")))
-         (propagate  (map guix-name (lset-union equal?
-                                                (listify meta "Imports")
-                                                (listify meta "LinkingTo")
-                                                (delete "R"
-                                                        (listify meta "Depends"))))))
-    `(package
-       (name ,(guix-name name))
-       (version ,version)
-       (source (origin
-                 (method url-fetch)
-                 (uri (,(procedure-name uri-helper) ,name version))
-                 (sha256
-                  (base32
-                   ,(bytevector->nix-base32-string (file-sha256 tarball))))))
-       ,@(if (not (equal? (string-append "r-" name)
-                          (guix-name name)))
-             `((properties ,`(,'quasiquote ((,'upstream-name . ,name)))))
-             '())
-       (build-system r-build-system)
-       ,@(maybe-inputs sysdepends)
-       ,@(maybe-inputs propagate 'propagated-inputs)
-       (home-page ,(if (string-null? home-page)
-                       (string-append base-url name)
-                       home-page))
-       (synopsis ,synopsis)
-       (description ,(beautify-description (assoc-ref meta "Description")))
-       (license ,license))))
+         (propagate  (lset-union equal?
+                                 (listify meta "Imports")
+                                 (listify meta "LinkingTo")
+                                 (delete "R"
+                                         (listify meta "Depends")))))
+    (values
+     `(package
+        (name ,(guix-name name))
+        (version ,version)
+        (source (origin
+                  (method url-fetch)
+                  (uri (,(procedure-name uri-helper) ,name version))
+                  (sha256
+                   (base32
+                    ,(bytevector->nix-base32-string (file-sha256 tarball))))))
+        ,@(if (not (equal? (string-append "r-" name)
+                           (guix-name name)))
+              `((properties ,`(,'quasiquote ((,'upstream-name . ,name)))))
+              '())
+        (build-system r-build-system)
+        ,@(maybe-inputs sysdepends)
+        ,@(maybe-inputs (map guix-name propagate) 'propagated-inputs)
+        (home-page ,(if (string-null? home-page)
+                        (string-append base-url name)
+                        home-page))
+        (synopsis ,synopsis)
+        (description ,(beautify-description (or (assoc-ref meta "Description")
+                                                "")))
+        (license ,license))
+     propagate)))
 
 (define* (cran->guix-package package-name #:optional (repo 'cran))
   "Fetch the metadata for PACKAGE-NAME from REPO and return the `package'