From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ricardo Wurmus Subject: Re: [PATCH] R build system and CRAN importer (updated) Date: Mon, 24 Aug 2015 16:24:38 +0200 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:33921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZTsg9-0005y0-9A for guix-devel@gnu.org; Mon, 24 Aug 2015 10:24:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZTsg4-0005v2-9M for guix-devel@gnu.org; Mon, 24 Aug 2015 10:24:53 -0400 Received: from sinope.bbbm.mdc-berlin.de ([141.80.25.23]:37444) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZTsg3-0005sx-QZ for guix-devel@gnu.org; Mon, 24 Aug 2015 10:24:48 -0400 In-Reply-To: List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: "Thompson, David" Cc: guix-devel 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=E2=80=99s 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=E2=80=99m not sure this mapping is the same for CRAN as it is for other= s. For example, =E2=80=98string->license=E2=80=99 for the CPAN importer uses ver= y different strings. > +(define (maybe-inputs package-inputs) > + "Given a list of PACKAGE-INPUTS, tries to generate the 'inputs' fiel= d 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=E2=80=99= 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 =E2=80=98cadr=E2=80=99 is that the SXML returned = by =E2=80=98cran-fetch=E2=80=99 is wrapped in =E2=80=98(*TOP* ...)=E2=80=99. I changed the match express= ion in =E2=80=98sxml-match-let*=E2=80=99 such that =E2=80=98(*TOP* ...)=E2=80=99= is explicitly matched. I agree that this is clearer than using =E2=80=98cadr=E2=80=99. > + (let* ((name (match:prefix (string-match ": " name-and-synops= is))) > + (synopsis (match:suffix (string-match ": " name-and-synops= is))) > + (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 sourc= e-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 vers= ion))) > > 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=E2=80=99m guilty of not using =E2=80=98match-lambda=E2=80=99 of= ten 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-li= brary/")) > + (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 =E2=80=98car=E2=80=99 is okay. =E2=80=98scandir=E2=80=99 re= turns a list of matching file names and I only expect one anyway, so I take the first with =E2=80=98car= =E2=80=99. =E2=80=98first=E2=80=99 would also be okay here. I=E2=80=99ll add a comment to explain why this is required. The reason i= s 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 =E2=80=98tools::testInstalledPackage=E2=80=99, which runs the tests for a= package given its name and the path to the =E2=80=9Clibrary=E2=80=9D (a location for a = collection of R packages) containing it. In the case of Guix, there is only one R package in any collection (=3D =E2=80=9Clibrary=E2=80=9D), so we can just take the name of the only dire= ctory 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 =3D \"" libdir "\")") > + (list "--no-save" "--slave"))) > > Nitpick: Use a quoted list: '("--no-save" "--slave") Fixed. Thanks again! I=E2=80=99ll send a new patch once I=E2=80=99ve written a = couple of tests for all this. Cheers, Ricardo