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
