From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:470:142:3::10]:45181) by lists.gnu.org with esmtp (Exim 4.86_2) (envelope-from ) id 1hbFUC-00014G-H9 for guix-patches@gnu.org; Wed, 12 Jun 2019 22:29:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hbFU8-0000Sp-DK for guix-patches@gnu.org; Wed, 12 Jun 2019 22:29:06 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:49004) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hbFU6-0000R6-7T for guix-patches@gnu.org; Wed, 12 Jun 2019 22:29:03 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1hbFU6-0002v3-3D for guix-patches@gnu.org; Wed, 12 Jun 2019 22:29:02 -0400 Subject: [bug#36048] [PATCH] guix: import: hackage: handle hackage revisions Resent-Message-ID: From: Timothy Sample References: <20190601223636.74362-1-rob@vllmrt.net> Date: Wed, 12 Jun 2019 22:28:35 -0400 In-Reply-To: <20190601223636.74362-1-rob@vllmrt.net> (Robert Vollmert's message of "Sun, 2 Jun 2019 00:36:36 +0200") Message-ID: <87sgse43sc.fsf@ngyro.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Robert Vollmert Cc: 36048@debbugs.gnu.org Hi Robert, Thanks for the patch, and thanks for the work you=E2=80=99ve been doing in general on the Haskell packages and tools in Guix! Robert Vollmert writes: > Hackage packages can have metadata revision (cabal-file only) > that aren't reflected in the source archive. haskell-build-system > has support for this, but previously `guix import hackage` would > create a definition based on the new cabal file but building using > the old cabal file. > > Compare https://debbugs.gnu.org/cgi/bugreport.cgi?bug=3D35750. > > > * guix/import/cabal.scm: Parse `x-revision:` property. > * guix/import/hackage.scm: Compute hash of cabal file, and write > cabal-revision build system arguments. > * guix/tests/hackage.scm: Test import of cabal revision. > --- > guix/import/cabal.scm | 7 +++-- > guix/import/hackage.scm | 61 ++++++++++++++++++++++++++++++----------- > tests/hackage.scm | 46 +++++++++++++++++++++++++++++++ > 3 files changed, 96 insertions(+), 18 deletions(-) > > diff --git a/guix/import/cabal.scm b/guix/import/cabal.scm > index 1a87be0b00..7dfe771e41 100644 > --- a/guix/import/cabal.scm > +++ b/guix/import/cabal.scm > @@ -40,6 +40,7 @@ > cabal-package? > cabal-package-name > cabal-package-version > + cabal-package-revision > cabal-package-license > cabal-package-home-page > cabal-package-source-repository > @@ -638,13 +639,14 @@ If #f use the function 'port-filename' to obtain it= ." > ;; information of the Cabal file, but only the ones we currently are > ;; interested in. > (define-record-type > - (make-cabal-package name version license home-page source-repository > + (make-cabal-package name version revision license home-page source-rep= ository > synopsis description > executables lib test-suites > flags eval-environment custom-setup) > cabal-package? > (name cabal-package-name) > (version cabal-package-version) > + (revision cabal-package-revision) > (license cabal-package-license) > (home-page cabal-package-home-page) > (source-repository cabal-package-source-repository) > @@ -838,6 +840,7 @@ See the manual for limitations."))))))) > (define (cabal-evaluated-sexp->package evaluated-sexp) > (let* ((name (lookup-join evaluated-sexp "name")) > (version (lookup-join evaluated-sexp "version")) > + (revision (lookup-join evaluated-sexp "x-revision")) > (license (lookup-join evaluated-sexp "license")) > (home-page (lookup-join evaluated-sexp "homepage")) > (home-page-or-hackage > @@ -856,7 +859,7 @@ See the manual for limitations."))))))) > (custom-setup (match (make-cabal-section evaluated-sexp 'cust= om-setup) > ((x) x) > (_ #f)))) > - (make-cabal-package name version license home-page-or-hackage > + (make-cabal-package name version revision license home-page-or-hac= kage > source-repository synopsis description executa= bles lib > test-suites flags eval-environment custom-setu= p))) >=20=20 > diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm > index 366256b40d..cf8219143a 100644 > --- a/guix/import/hackage.scm > +++ b/guix/import/hackage.scm > @@ -117,9 +117,15 @@ version is returned." > (#f name) > (m (match:substring m 1))))))) >=20=20 > -(define (hackage-fetch name-version) > - "Return the Cabal file for the package NAME-VERSION, or #f on failure.= If > -the version part is omitted from the package name, then return the latest > +(define (read-cabal-and-hash port) This procedure should have a docstring. > + (let-values (((port get-hash) (open-sha256-input-port port))) > + (cons > + (read-cabal (canonical-newline-port port)) > + (bytevector->nix-base32-string (get-hash))))) The indentation here is wrong. In general, list elements all line up with each other, so everything would be under the =E2=80=98c=E2=80=99 of = =E2=80=9Ccons=E2=80=9D. There are exceptions to this, but they=E2=80=99re mostly for special block-like f= orms such as =E2=80=9Clet=E2=80=9D and =E2=80=9Cbegin=E2=80=9D. However, I thin= k Schemers tend to avoid putting a line break after the procedure when applying. That is, it would be more conventionally formatted as: (cons (read-cabal (canonical-newline-port port)) (bytevector->nix-base32-string (get-hash))) Also, I think returning multiple values would be more natural here (i.e., replace =E2=80=9Ccons=E2=80=9D with =E2=80=9Cvalues=E2=80=9D). > + > +(define (hackage-fetch-and-hash name-version) > + "Return the Cabal file and hash for the package NAME-VERSION, or #f on= failure. Here, it would be clearer to mention the shape of the return value as well. For the =E2=80=9Cvalues=E2=80=9D version, it would be something like= =E2=80=9CReturn two values: the Cabal file for the package NAME-VERSION and its hash....=E2=80= =9D I cribbed this wording from the Guile manual, but I worry that the referent of that last =E2=80=9Cits=E2=80=9D is not clear. It=E2=80=99s pro= bably good enough, but maybe you can do better. ;) It=E2=80=99s the =E2=80=9Ctwo values=E2= =80=9D part that=E2=80=99s important. Also, please try to keep the line lengths under 80 columns. > +If the version part is omitted from the package name, then return the la= test > version." > (guard (c ((and (http-get-error? c) > (=3D 404 (http-get-error-code c))) > @@ -127,10 +133,18 @@ version." > (let-values (((name version) (package-name->name+version name-versio= n))) > (let* ((url (hackage-cabal-url name version)) > (port (http-fetch url)) > - (result (read-cabal (canonical-newline-port port)))) > + (result (read-cabal-and-hash port))) > (close-port port) > result)))) >=20=20 > +(define (hackage-fetch name-version) > + "Return the Cabal file for the package NAME-VERSION, or #f on failure.= If > +the version part is omitted from the package name, then return the latest > +version." > + (match (hackage-fetch-and-hash name-version) > + ((cabal . hash) cabal) > + (_ #f))) > + This will have to become a =E2=80=9Clet-values=E2=80=9D form. > (define string->license > ;; List of valid values from > ;; https://www.haskell.org > @@ -198,15 +212,19 @@ package being processed and is used to filter refer= ences to itself." > (cons own-name ghc-standard-libraries= )))) > dependencies)) >=20=20 > -(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #= t)) > +(define* (hackage-module->sexp cabal cabal-hash #:key (include-test-depe= ndencies? #t)) Another long line. You could line up =E2=80=9C#:key=E2=80=9D under the fir= st =E2=80=9Ccabal=E2=80=9D. > "Return the `package' S-expression for a Cabal package. CABAL is the > -representation of a Cabal file as produced by 'read-cabal'." > +representation of a Cabal file as produced by 'read-cabal'. CABAL-HASH is There should be two spaces after a period ------------------^ > +the hash of the Cabal file." >=20=20 > (define name > (cabal-package-name cabal)) >=20=20 > (define version > (cabal-package-version cabal)) > + > + (define revision > + (cabal-package-revision cabal)) >=20=20=20=20 > (define source-url > (hackage-source-url name version)) > @@ -252,9 +270,17 @@ representation of a Cabal file as produced by 'read-= cabal'." > (list 'quasiquote inputs)))))) >=20=20=20=20 > (define (maybe-arguments) > - (if (not include-test-dependencies?) > - '((arguments `(#:tests? #f))) > - '())) > + (define testargs (if (not include-test-dependencies?) > + '(#:tests? #f) > + '())) > + (define revargs (if (not (string-null? revision)) > + `(#:cabal-revision (,revision ,cabal-hash)) > + '())) > + (define args (append testargs revargs)) > + (if (not (nil? args)) > + (let ((qargs `(,'quasiquote ,args))) > + `((arguments ,qargs))) > + '())) I think that this would be a little clearer using =E2=80=9Cmatch=E2=80=9D a= nd without the intermediary definitions: (match (append (if (not include-test-dependencies?) '(#:tests? #f) '()) (if (not (string-null? revision)) `(#:cabal-revision (,revision ,cabal-hash)) '())) (() '()) (args `((arguments (,'quasiquote ,args))))) >=20=20 > (let ((tarball (with-store store > (download-to-store store source-url)))) > @@ -294,13 +320,16 @@ symbol 'true' or 'false'. The value associated wit= h other keys has to conform > to the Cabal file format definition. The default value associated with = the > keys \"os\", \"arch\" and \"impl\" is \"linux\", \"x86_64\" and \"ghc\" > respectively." > - (let ((cabal-meta (if port > - (read-cabal (canonical-newline-port port)) > - (hackage-fetch package-name)))) > - (and=3D> cabal-meta (compose (cut hackage-module->sexp <> > - #:include-test-dependencies? > - include-test-dependencies?) > - (cut eval-cabal <> cabal-environment))))) > + (match > + (if port (read-cabal-and-hash port) > + (hackage-fetch-and-hash package-name)) > + ((cabal-meta . cabal-hash) > + (and=3D> cabal-meta (compose (cut hackage-module->sexp <> > + cabal-hash > + #:include-test-dependencies? > + include-test-dependencies?) > + (cut eval-cabal <> cabal-environment)))) > + (_ #f))) This will also need to be a =E2=80=9Clet-values=E2=80=9D form for the multi= ple values. Fortunately, it will look almost identical to the old =E2=80=9Clet=E2=80=9D= form, so that=E2=80=99s kinda nice (if you=E2=80=99re as easily amused as I am). :) > (define hackage->guix-package/m ;memoized variant > (memoize hackage->guix-package)) > diff --git a/tests/hackage.scm b/tests/hackage.scm > index 38a5825af7..fe4e0efb69 100644 > --- a/tests/hackage.scm > +++ b/tests/hackage.scm > @@ -274,6 +274,52 @@ executable cabal > (test-assert "hackage->guix-package test multiline desc (braced)" > (eval-test-with-cabal test-cabal-multiline-braced match-ghc-foo)) >=20=20 > +;; test hackage cabal revisions To be consistent with the other comments in the file, I would suggest: ;; Check Hackage Cabal revisions. (I know that some of the comments are missing a period at the end, but most of them have it, and it should be there.) > +(define test-cabal-revision > + "name: foo > +version: 1.0.0 > +x-revision: 2 > +homepage: http://test.org > +synopsis: synopsis > +description: description > +license: BSD3 > +executable cabal > + build-depends: > + HTTP >=3D 4000.2.5 && < 4000.3, > + mtl >=3D 2.0 && < 3 > +") > + > +(define-package-matcher match-ghc-foo-revision > + ('package > + ('name "ghc-foo") > + ('version "1.0.0") > + ('source > + ('origin > + ('method 'url-fetch) > + ('uri ('string-append > + "https://hackage.haskell.org/package/foo/foo-" > + 'version > + ".tar.gz")) > + ('sha256 > + ('base32 > + (? string? hash))))) > + ('build-system 'haskell-build-system) > + ('inputs > + ('quasiquote > + (("ghc-http" ('unquote 'ghc-http))))) > + ('arguments > + ('quasiquote > + ('#:cabal-revision > + ("2" "0xxd88fb659f0krljidbvvmkh9ppjnx83j0nqzx8whcg4n5qbyng")))) > + ('home-page "http://test.org") > + ('synopsis (? string?)) > + ('description (? string?)) > + ('license 'bsd-3))) > + > +(test-assert "hackage->guix-package test cabal revision" > + (eval-test-with-cabal test-cabal-revision match-ghc-foo-revision)) > + > + I think there=E2=80=99s an extra line break here. > (test-assert "read-cabal test 1" > (match (call-with-input-string test-read-cabal-1 read-cabal) > ((("name" ("test-me")) Overall, it looks pretty good. Bonus points for including a test! With the few tweaks I mentioned, I think it will be ready to go. Thanks again. -- Tim