Re: [PATCH] R build system and CRAN importer (updated)

[Ricardo Wurmus]

Hi David,

> Apologies if the mail client I'm using butchers the formatting... my
> Emacs mail setup isn't working quite right now so I'm using something
> else.  I hope you can still read my feedback well enough.

that’s okay.  Thank you for taking the time to review this patch set!

> +(define string->license
> +  (match-lambda
> +   ("AGPL-3" 'agpl3)
> +   ("Artistic-2.0" 'artistic2.0)
> +   ("Apache License 2.0" 'asl2.0)
> +   ("BSD_2_clause" 'bsd-2)
> +   ("BSD_3_clause" 'bsd-3)
> +   ("GPL-2" 'gpl2)
> +   ("GPL-3" 'GPL3)
> +   ("LGPL-2" 'lgpl2.0)
> +   ("LGPL-2.1" 'lgpl2.1)
> +   ("LGPL-3" 'lgpl3)
> +   ("MIT" 'x11)
> +   ((x) (string->license x))
> +   ((lst ...) `(list ,@(map string->license lst)))
> +   (_ #f)))
>
> With the addition of the Ruby gem importer, I have factorized
> string->license into (guix import utils).  Once this importer and the
> gem importer have reached master, would you like to merge this
> procedure with the factorized one?

I’m not sure this mapping is the same for CRAN as it is for others.  For
example, ‘string->license’ for the CPAN importer uses very different
strings.

> +(define (maybe-inputs package-inputs)
> +  "Given a list of PACKAGE-INPUTS, tries to generate the 'inputs' field of a
> +package definition."
> +  (match package-inputs
> +    (()
> +     '())
> +    ((package-inputs ...)
> +     `((inputs (,'quasiquote ,(format-inputs package-inputs)))))))
>
> Should these be propagated inputs?

Actually, yes, but only when the list consists of R packages.  I’ll
change it such that the field name can be customised.

> +  (sxml-match-let*
> +   (((xhtml:html
> +      ,head
> +      (xhtml:body
> +       (xhtml:h2 ,name-and-synopsis)
> +       (xhtml:p ,description)
> +       ,summary
> +       (xhtml:h4 "Downloads:") ,downloads
> +       . ,rest))
> +     (cadr sxml)))
>
> Can we avoid this cadr call and use 'match' instead?

The only reason for the ‘cadr’ is that the SXML returned by ‘cran-fetch’
is wrapped in ‘(*TOP* ...)’.  I changed the match expression in
‘sxml-match-let*’ such that ‘(*TOP* ...)’ is explicitly matched.  I
agree that this is clearer than using ‘cadr’.

> +   (let* ((name       (match:prefix (string-match ": " name-and-synopsis)))
> +          (synopsis   (match:suffix (string-match ": " name-and-synopsis)))
> +          (version    (nodes->text (table-datum summary "Version:")))
> +          (license    ((compose string->license nodes->text)
> +                       (table-datum summary "License:")))
> +          (home-page  (nodes->text ((sxpath '((xhtml:a 1)))
> +                                    (table-datum summary "URL:"))))
> +          (source-url (string-append "mirror://cran/"
> +                                     ;; Remove double dots, because we want 
> an
> +                                     ;; absolute path.
> +                                     (regexp-substitute/global
> +                                      #f "\\.\\./"
> +                                      (string-join
> +                                       ((sxpath '((xhtml:a 1) @ href *text*))
> +                                        (table-datum downloads "
> Package source: ")))
>
> Line is too long.

Oops.  Fixed.

> +                                      'pre 'post)))
> +          (tarball    (with-store store (download-to-store store 
> source-url)))
> +          (sysdepends (map match:substring
> +                           (list-matches
> +                            "[^ ]+"
> +                            ;; Strip off comma and parenthetical
> +                            ;; expressions.
> +                            (regexp-substitute/global
> +                             #f "(,|\\([^\\)]+\\))"
> +                             (nodes->text (table-datum summary
> "SystemRequirements:"))
>
> Line is too long.

Fixed.

> +                             'pre 'post))))
> +          (imports    (map guix-name
> +                           ((sxpath '(// xhtml:a *text*))
> +                            (table-datum summary "Imports:")))))
> +     `(package
> +        (name ,(guix-name name))
> +        (version ,version)
> +        (source (origin
> +                  (method url-fetch)
> +                  (uri (string-append ,@(factorize-uri source-url version)))
>
> Food for thought: For Ruby, I decided that rather than repeating the
> same 'string-append' form all over the place, I would have a procedure
> called 'rubygems-uri' in (guix build-system ruby) that accepts a
> 'name' and 'version' argument and returns the correct URI.  If the
> source tarballs are all hosted on CRAN, I think this would be a nice
> thing to add.  It can be done later, though.

Good idea!

> +(define (generate-site-path inputs)
> +  (string-join (map (lambda (input)
> +                      (string-append (cdr input) "/site-library"))
> +                    ;; Restrict to inputs beginning with "r-".
> +                    (filter (lambda (input)
> +                              (string-prefix? "r-" (car input)))
> +                            inputs))
>
> Use 'match-lambda' instead of car/cdr above.

Fixed.  I’m guilty of not using ‘match-lambda’ often enough :)

> +               ":"))
> +
> +(define* (check #:key test-target inputs outputs tests? #:allow-other-keys)
> +  "Run the test suite of a given R package."
> +  (let* ((libdir    (string-append (assoc-ref outputs "out") 
> "/site-library/"))
> +         (pkg-name  (car (scandir libdir (negate (cut member <> '("."
> ".."))))))
>
> Ludo prefers that we use 'match' instead of car/cdring.  A comment
> here would help me understand why the package name needs to be
> determined this way.

I think here ‘car’ is okay.  ‘scandir’ returns a list of matching file
names and I only expect one anyway, so I take the first with ‘car’.
‘first’ would also be okay here.

I’ll add a comment to explain why this is required.  The reason is that
R package names are case-sensitive and cannot be derived from the Guix
package name.  The R build system has no way to know the original name
of the R package, but the exact name is required as an argument to
‘tools::testInstalledPackage’, which runs the tests for a package given
its name and the path to the “library” (a location for a collection of R
packages) containing it.

In the case of Guix, there is only one R package in any collection (=
“library”), so we can just take the name of the only directory in the
collection path to figure out the original name of the R package.

> +        (begin
> +          (setenv "R_LIBS_SITE" site-path)
> +          (pipe-to-r (string-append "tools::testInstalledPackage(\""
> pkg-name "\", "
> +                                    "lib.loc = \"" libdir "\")")
> +                     (list "--no-save" "--slave")))
>
> Nitpick: Use a quoted list: '("--no-save" "--slave")

Fixed.

Thanks again!  I’ll send a new patch once I’ve written a couple of tests
for all this.

Cheers,
Ricardo