[PATCH] Add recursive CRAN importer

[PATCH] Add recursive CRAN importer

[Ricardo Wurmus]

attached is a patch series that gives us a recursive CRAN importer.  I still
haven't documented it in the manual, because I'm not sure if the
implementation is acceptable.

Simply put, the procedure that produces a package expression now returns
multiple values.  The first is the package expression, the second is a list of
dependencies (with their upstream names).  This allows us to go through that
list and check if any unpackaged inputs are among them.

The recursive importer stops when a package cannot be imported (e.g. when a
bioconductor package has unpackaged CRAN dependencies), or when all packages
either already existed at runtime or have been packaged.

To make this work well I had to create a list of packages that should not be
imported (because they are part of the default R installation), and I had to
make sure that R packages always get a predictable Guix package name.

Comments are very welcome!

~~ Ricardo

[PATCH 1/7] import cran: Remove more invalid characters from package names.

[Ricardo Wurmus]

* guix/import/cran.scm (guix-name): Replace period and underscore with
  dash; always prepend package names with "r-".
---
 guix/import/cran.scm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index c1a9e8e..8b368af 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -151,9 +151,9 @@ empty list when the FIELD cannot be found."
   "Return the `package' s-expression for an R package published on REPOSITORY
 from the alist META, which was derived from the R package's DESCRIPTION file."
   (define (guix-name name)
-    (if (string-prefix? "r-" name)
-        (string-downcase name)
-        (string-append "r-" (string-downcase name))))
+    (string-append "r-" (string-downcase
+                         (regexp-substitute/global #f "(_|\\.)" name
+                                                   'pre "-" 'post))))
 
   (let* ((base-url   (case repository
                        ((cran)         %cran-url)
-- 
2.7.3

[PATCH 2/7] import cran: Move guix-name to top-level.

[Ricardo Wurmus]

* guix/import/cran.scm (guix-name): Move to top-level.
---
 guix/import/cran.scm | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 8b368af..0be5346 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -147,14 +147,16 @@ empty list when the FIELD cannot be found."
                         (string-any char-set:whitespace item)))
                   (map string-trim-both items))))))
 
+
+(define (guix-name name)
+  "Return a Guix package name for a given R package name."
+  (string-append "r-" (string-downcase
+                       (regexp-substitute/global #f "(_|\\.)" name
+                                                 'pre "-" 'post))))
+
 (define (description->package repository meta)
   "Return the `package' s-expression for an R package published on REPOSITORY
 from the alist META, which was derived from the R package's DESCRIPTION file."
-  (define (guix-name name)
-    (string-append "r-" (string-downcase
-                         (regexp-substitute/global #f "(_|\\.)" name
-                                                   'pre "-" 'post))))
-
   (let* ((base-url   (case repository
                        ((cran)         %cran-url)
                        ((bioconductor) %bioconductor-url)))
-- 
2.7.3

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

[Ricardo Wurmus]

* 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(-)

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'
-- 
2.7.3

[PATCH 4/7] import cran: Ignore default R packages.

[Ricardo Wurmus]

* guix/import/cran.scm (default-r-packages): New variable.
(description->package): Drop default R packages from list of inputs.
---
 guix/import/cran.scm | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index ff9dbd3..522df3e 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -148,6 +148,16 @@ empty list when the FIELD cannot be found."
                         (string-any char-set:whitespace item)))
                   (map string-trim-both items))))))
 
+(define default-r-packages
+  (list "stats"
+        "methods"
+        "utils"
+        "graphics"
+        "grDevices"
+        "parallel"
+        "grid"
+        "tools"
+        "matrix"))
 
 (define (guix-name name)
   "Return a Guix package name for a given R package name."
@@ -178,11 +188,13 @@ 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  (lset-union equal?
-                                 (listify meta "Imports")
-                                 (listify meta "LinkingTo")
-                                 (delete "R"
-                                         (listify meta "Depends")))))
+         (propagate  (filter (lambda (name)
+                               (not (member name default-r-packages)))
+                             (lset-union equal?
+                                         (listify meta "Imports")
+                                         (listify meta "LinkingTo")
+                                         (delete "R"
+                                                 (listify meta "Depends"))))))
     (values
      `(package
         (name ,(guix-name name))
-- 
2.7.3

[PATCH 5/7] import cran: Add recursive importer.

[Ricardo Wurmus]

* guix/import/cran.scm (recursive-import): New variable.
---
 guix/import/cran.scm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index 522df3e..907441e 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -33,8 +33,10 @@
   #:use-module ((guix build-system r) #:select (cran-uri bioconductor-uri))
   #:use-module (guix upstream)
   #:use-module (guix packages)
+  #:use-module (gnu packages)
   #:export (cran->guix-package
             bioconductor->guix-package
+            recursive-import
             %cran-updater
             %bioconductor-updater))
 
@@ -236,6 +238,25 @@ s-expression corresponding to that package, or #f on failure."
 ;; * Then follow
 ;;   https://r-forge.r-project.org/R/?group_id=929 with the given group_id.
 
+(define* (recursive-import package-name #:optional (repo 'cran))
+  (define (iter name imported)
+    ;; FIXME: this might fail, so catch errors
+    (receive (package dependencies)
+        (cran->guix-package name repo)
+      (if package
+          (let* ((new-entry    (cons name (list package)))
+                 (imported     (cons new-entry imported))
+                 (dependencies (filter (lambda (dependency)
+                                         (and (not (assoc dependency imported))
+                                              (null? (find-packages-by-name (guix-name dependency)))))
+                                       dependencies)))
+            (fold iter imported dependencies))
+          (begin
+            (format #t "error: failed to import package ~a from archive ~a.\n" name repo)
+            imported))))
+  (iter package-name '()))
+
+
 \f
 ;;;
 ;;; Updater.
-- 
2.7.3

[PATCH 6/7] import cran: Add "recursive" option.

[Ricardo Wurmus]

* guix/scripts/import/cran.scm: (%options): Add "recursive" option.
(guix-import-cran): Handle "recursive" option.
---
 guix/scripts/import/cran.scm | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/guix/scripts/import/cran.scm b/guix/scripts/import/cran.scm
index ace1123..4cae9da 100644
--- a/guix/scripts/import/cran.scm
+++ b/guix/scripts/import/cran.scm
@@ -63,6 +63,9 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
                  (lambda (opt name arg result)
                    (alist-cons 'repo (string->symbol arg)
                                (alist-delete 'repo result))))
+         (option '(#\r "recursive") #f #f
+                 (lambda (opt name arg result)
+                   (alist-cons 'recursive #t result)))
          %standard-import-options))
 
 \f
@@ -88,12 +91,23 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
                            (reverse opts))))
     (match args
       ((package-name)
-       (let ((sexp (cran->guix-package package-name
-                                       (or (assoc-ref opts 'repo) 'cran))))
-         (unless sexp
-           (leave (_ "failed to download description for package '~a'~%")
-                  package-name))
-         sexp))
+       (if (assoc-ref opts 'recursive)
+           ;; Recursive import
+           (map (match-lambda
+                  ((and (label . (('package ('name name) . rest)))
+                        (label . (pkg)))
+                   `(define-public ,(string->symbol name)
+                      ,pkg))
+                  (_ #f))
+                (recursive-import package-name
+                                  (or (assoc-ref opts 'repo) 'cran)))
+           ;; Single import
+           (let ((sexp (cran->guix-package package-name
+                                           (or (assoc-ref opts 'repo) 'cran))))
+             (unless sexp
+               (leave (_ "failed to download description for package '~a'~%")
+                      package-name))
+             sexp)))
       (()
        (leave (_ "too few arguments~%")))
       ((many ...)
-- 
2.7.3

[PATCH 7/7] guix import: Print list of expressions.

[Ricardo Wurmus]

* guix/scripts/import.scm (guix-import): Print list of expressions.
---
 guix/scripts/import.scm | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/guix/scripts/import.scm b/guix/scripts/import.scm
index e54744f..5cda3dd 100644
--- a/guix/scripts/import.scm
+++ b/guix/scripts/import.scm
@@ -107,10 +107,14 @@ Run IMPORTER with ARGS.\n"))
      (show-version-and-exit "guix import"))
     ((importer args ...)
      (if (member importer importers)
-         (match (apply (resolve-importer importer) args)
-           ((and expr ('package _ ...))
-            (pretty-print expr (newline-rewriting-port
-                                (current-output-port))))
-           (x
-            (leave (_ "'~a' import failed~%") importer)))
+         (let ((print (lambda (expr)
+                        (pretty-print expr (newline-rewriting-port
+                                            (current-output-port))))))
+           (match (apply (resolve-importer importer) args)
+             ((and expr ('package _ ...))
+              (print expr))
+             ((? list? expressions)
+              (for-each print expressions))
+             (x
+              (leave (_ "'~a' import failed~%") importer))))
          (leave (_ "~a: invalid importer~%") importer)))))
-- 
2.7.3

Re: [PATCH 1/7] import cran: Remove more invalid characters from package names.

[Ludovic Courtès]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/import/cran.scm (guix-name): Replace period and underscore with
>   dash; always prepend package names with "r-".

[...]

> +    (string-append "r-" (string-downcase
> +                         (regexp-substitute/global #f "(_|\\.)" name
> +                                                   'pre "-" 'post))))

I have a preference fro ‘string-map’, which I find more readable than
‘regexp-substitute/global’ in simple cases.  Thoughts?

Otherwise LGTM!

Thanks,
Ludo’.

Re: [PATCH 2/7] import cran: Move guix-name to top-level.

[Ludovic Courtès]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/import/cran.scm (guix-name): Move to top-level.

[...]

> +(define (guix-name name)
> +  "Return a Guix package name for a given R package name."
> +  (string-append "r-" (string-downcase
> +                       (regexp-substitute/global #f "(_|\\.)" name
> +                                                 'pre "-" 'post))))

Same comment as before, but otherwise OK!

Ludo’.

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

[Ludovic Courtès]

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

Re: [PATCH 4/7] import cran: Ignore default R packages.

[Ludovic Courtès]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/import/cran.scm (default-r-packages): New variable.
> (description->package): Drop default R packages from list of inputs.

OK!

Ludo'.

Re: [PATCH 6/7] import cran: Add "recursive" option.

[Ludovic Courtès]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/scripts/import/cran.scm: (%options): Add "recursive" option.
> (guix-import-cran): Handle "recursive" option.
> ---
>  guix/scripts/import/cran.scm | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/guix/scripts/import/cran.scm b/guix/scripts/import/cran.scm
> index ace1123..4cae9da 100644
> --- a/guix/scripts/import/cran.scm
> +++ b/guix/scripts/import/cran.scm
> @@ -63,6 +63,9 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
>                   (lambda (opt name arg result)
>                     (alist-cons 'repo (string->symbol arg)
>                                 (alist-delete 'repo result))))
> +         (option '(#\r "recursive") #f #f
> +                 (lambda (opt name arg result)
> +                   (alist-cons 'recursive #t result)))
>           %standard-import-options))
>  
>  \f
> @@ -88,12 +91,23 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
>                             (reverse opts))))
>      (match args
>        ((package-name)
> -       (let ((sexp (cran->guix-package package-name
> -                                       (or (assoc-ref opts 'repo) 'cran))))
> -         (unless sexp
> -           (leave (_ "failed to download description for package '~a'~%")
> -                  package-name))
> -         sexp))
> +       (if (assoc-ref opts 'recursive)
> +           ;; Recursive import
> +           (map (match-lambda
> +                  ((and (label . (('package ('name name) . rest)))
> +                        (label . (pkg)))
> +                   `(define-public ,(string->symbol name)
> +                      ,pkg))

Perhaps it could check with ‘find-packages-by-name’ whether NAME already
exists?

> +                  (_ #f))
> +                (recursive-import package-name
> +                                  (or (assoc-ref opts 'repo) 'cran)))
> +           ;; Single import
> +           (let ((sexp (cran->guix-package package-name
> +                                           (or (assoc-ref opts 'repo) 'cran))))
> +             (unless sexp
> +               (leave (_ "failed to download description for package '~a'~%")
> +                      package-name))
> +             sexp)))

Do you think this could be moved to (guix scripts import)?  We would
have to change other importers to return an empty list/stream of
dependencies for now.

Thanks,
Ludo’.

Re: [PATCH 7/7] guix import: Print list of expressions.

[Ludovic Courtès]

Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:

> * guix/scripts/import.scm (guix-import): Print list of expressions.

Depending on your answer to #6, maybe we’d bring more of the recursive
functionality to this file.  :-)

Anyway, this is a step in the right direction, thanks for working on it!

Ludo’.

Re: [PATCH 1/7] import cran: Remove more invalid characters from package names.

[Ricardo Wurmus]

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

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>
>> * guix/import/cran.scm (guix-name): Replace period and underscore with
>>   dash; always prepend package names with "r-".
>
> [...]
>
>> +    (string-append "r-" (string-downcase
>> +                         (regexp-substitute/global #f "(_|\\.)" name
>> +                                                   'pre "-" 'post))))
>
> I have a preference fro ‘string-map’, which I find more readable than
> ‘regexp-substitute/global’ in simple cases.  Thoughts?
>
> Otherwise LGTM!

Using “string-map” is indeed nicer!  I’m not a fan of
“regexp-substitute/global” (and regular expressions in general), but it
didn’t occur to me to use “string-map”.  Thanks for the hint.

~~ Ricardo

Re: [PATCH 6/7] import cran: Add "recursive" option.

[Ricardo Wurmus]

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

> Ricardo Wurmus <ricardo.wurmus@mdc-berlin.de> skribis:
>
>> * guix/scripts/import/cran.scm: (%options): Add "recursive" option.
>> (guix-import-cran): Handle "recursive" option.
>> ---
>>  guix/scripts/import/cran.scm | 26 ++++++++++++++++++++------
>>  1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/guix/scripts/import/cran.scm b/guix/scripts/import/cran.scm
>> index ace1123..4cae9da 100644
>> --- a/guix/scripts/import/cran.scm
>> +++ b/guix/scripts/import/cran.scm
>> @@ -63,6 +63,9 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
>>                   (lambda (opt name arg result)
>>                     (alist-cons 'repo (string->symbol arg)
>>                                 (alist-delete 'repo result))))
>> +         (option '(#\r "recursive") #f #f
>> +                 (lambda (opt name arg result)
>> +                   (alist-cons 'recursive #t result)))
>>           %standard-import-options))
>>  
>>  \f
>> @@ -88,12 +91,23 @@ Import and convert the CRAN package for PACKAGE-NAME.\n"))
>>                             (reverse opts))))
>>      (match args
>>        ((package-name)
>> -       (let ((sexp (cran->guix-package package-name
>> -                                       (or (assoc-ref opts 'repo) 'cran))))
>> -         (unless sexp
>> -           (leave (_ "failed to download description for package '~a'~%")
>> -                  package-name))
>> -         sexp))
>> +       (if (assoc-ref opts 'recursive)
>> +           ;; Recursive import
>> +           (map (match-lambda
>> +                  ((and (label . (('package ('name name) . rest)))
>> +                        (label . (pkg)))
>> +                   `(define-public ,(string->symbol name)
>> +                      ,pkg))
>
> Perhaps it could check with ‘find-packages-by-name’ whether NAME already
> exists?

The “recursive-import” method currently only does this for
dependencies.  A check would involve generating “(guix-name
package-name)”, but “guix-name” is defined in “guix/import/cran.scm”.
Should this check happen in the “recursive-import” procedure instead?

>> +                  (_ #f))
>> +                (recursive-import package-name
>> +                                  (or (assoc-ref opts 'repo) 'cran)))
>> +           ;; Single import
>> +           (let ((sexp (cran->guix-package package-name
>> +                                           (or (assoc-ref opts 'repo) 'cran))))
>> +             (unless sexp
>> +               (leave (_ "failed to download description for package '~a'~%")
>> +                      package-name))
>> +             sexp)))
>
> Do you think this could be moved to (guix scripts import)?  We would
> have to change other importers to return an empty list/stream of
> dependencies for now.

Yes, I’ll do that.

~~ Ricardo

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

[Ricardo Wurmus]

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

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

[Ricardo Wurmus]

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

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

[Ludovic Courtès]

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

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

[Ricardo Wurmus]

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

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

[Ludovic Courtès]

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