GNU bug report logs - #36048
[PATCH] guix: import: hackage: handle hackage revisions

Previous Next

Package: guix-patches;

Reported by: Robert Vollmert <rob <at> vllmrt.net>

Date: Sat, 1 Jun 2019 22:37:02 UTC

Severity: normal

Tags: patch

Done: Timothy Sample <samplet <at> ngyro.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36048 in the body.
You can then email your comments to 36048 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Sat, 01 Jun 2019 22:37:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Robert Vollmert <rob <at> vllmrt.net>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sat, 01 Jun 2019 22:37:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: guix-patches <at> gnu.org
Cc: Robert Vollmert <rob <at> vllmrt.net>
Subject: [PATCH] guix: import: hackage: handle hackage revisions
Date: Sun,  2 Jun 2019 00:36:36 +0200
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=35750.

* 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 <cabal-package>
-  (make-cabal-package name version license home-page source-repository
+  (make-cabal-package name version revision license home-page source-repository
                       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 'custom-setup)
                            ((x) x)
                            (_ #f))))
-      (make-cabal-package name version license home-page-or-hackage
+      (make-cabal-package name version revision license home-page-or-hackage
                           source-repository synopsis description executables lib
                           test-suites flags eval-environment custom-setup)))
 
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)))))))
 
-(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)
+  (let-values (((port get-hash) (open-sha256-input-port port)))
+    (cons
+      (read-cabal (canonical-newline-port port))
+      (bytevector->nix-base32-string (get-hash)))))
+
+(define (hackage-fetch-and-hash name-version)
+  "Return the Cabal file and hash for the package NAME-VERSION, or #f on failure.
+If the version part is omitted from the package name, then return the latest
 version."
   (guard (c ((and (http-get-error? c)
                   (= 404 (http-get-error-code c)))
@@ -127,10 +133,18 @@ version."
     (let-values (((name version) (package-name->name+version name-version)))
       (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))))
 
+(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)))
+
 (define string->license
   ;; List of valid values from
   ;; https://www.haskell.org
@@ -198,15 +212,19 @@ package being processed and is used to filter references to itself."
                                    (cons own-name ghc-standard-libraries))))
           dependencies))
 
-(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #t))
+(define* (hackage-module->sexp cabal cabal-hash #:key (include-test-dependencies? #t))
   "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
+the hash of the Cabal file."
 
   (define name
     (cabal-package-name cabal))
 
   (define version
     (cabal-package-version cabal))
+
+  (define revision
+    (cabal-package-revision cabal))
   
   (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))))))
   
   (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)))
+      '()))
 
   (let ((tarball (with-store store
                    (download-to-store store source-url))))
@@ -294,13 +320,16 @@ symbol 'true' or 'false'.  The value associated with 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=> 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=> cabal-meta (compose (cut hackage-module->sexp <>
+                                     cabal-hash
+                                     #:include-test-dependencies?
+                                     include-test-dependencies?)
+                                (cut eval-cabal <> cabal-environment))))
+    (_ #f)))
 
 (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))
 
+;; test hackage cabal revisions
+(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       >= 4000.2.5 && < 4000.3,
+    mtl        >= 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))
+
+
 (test-assert "read-cabal test 1"
   (match (call-with-input-string test-read-cabal-1 read-cabal)
     ((("name" ("test-me"))
-- 
2.20.1 (Apple Git-117)





Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 02:29:02 GMT) Full text and rfc822 format available.

Message #8 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Timothy Sample <samplet <at> ngyro.com>
To: Robert Vollmert <rob <at> vllmrt.net>
Cc: 36048 <at> debbugs.gnu.org
Subject: Re: [bug#36048] [PATCH] guix: import: hackage: handle hackage
 revisions
Date: Wed, 12 Jun 2019 22:28:35 -0400
Hi Robert,

Thanks for the patch, and thanks for the work you’ve been doing in
general on the Haskell packages and tools in Guix!

Robert Vollmert <rob <at> vllmrt.net> 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=35750.
>
>
> * 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 <cabal-package>
> -  (make-cabal-package name version license home-page source-repository
> +  (make-cabal-package name version revision license home-page source-repository
>                        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 'custom-setup)
>                             ((x) x)
>                             (_ #f))))
> -      (make-cabal-package name version license home-page-or-hackage
> +      (make-cabal-package name version revision license home-page-or-hackage
>                            source-repository synopsis description executables lib
>                            test-suites flags eval-environment custom-setup)))
>  
> 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)))))))
>  
> -(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 ‘c’ of “cons”.  There
are exceptions to this, but they’re mostly for special block-like forms
such as “let” and “begin”.  However, I think 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 “cons” with “values”).

> +
> +(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 “values” version, it would be something like “Return two
values: the Cabal file for the package NAME-VERSION and its hash....”  I
cribbed this wording from the Guile manual, but I worry that the
referent of that last “its” is not clear.  It’s probably good enough,
but maybe you can do better.  ;)  It’s the “two values” part that’s
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 latest
>  version."
>    (guard (c ((and (http-get-error? c)
>                    (= 404 (http-get-error-code c)))
> @@ -127,10 +133,18 @@ version."
>      (let-values (((name version) (package-name->name+version name-version)))
>        (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))))
>  
> +(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 “let-values” 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 references to itself."
>                                     (cons own-name ghc-standard-libraries))))
>            dependencies))
>  
> -(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #t))
> +(define* (hackage-module->sexp cabal cabal-hash #:key (include-test-dependencies? #t))

Another long line.  You could line up “#:key” under the first “cabal”.

>    "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."
>  
>    (define name
>      (cabal-package-name cabal))
>  
>    (define version
>      (cabal-package-version cabal))
> +
> +  (define revision
> +    (cabal-package-revision cabal))
>    
>    (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))))))
>    
>    (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 “match” and 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)))))

>  
>    (let ((tarball (with-store store
>                     (download-to-store store source-url))))
> @@ -294,13 +320,16 @@ symbol 'true' or 'false'.  The value associated with 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=> 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=> 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 “let-values” form for the multiple values.
Fortunately, it will look almost identical to the old “let” form, so
that’s kinda nice (if you’re 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))
>  
> +;; 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       >= 4000.2.5 && < 4000.3,
> +    mtl        >= 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’s 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




Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 16:04:02 GMT) Full text and rfc822 format available.

Message #11 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: 36048 <at> debbugs.gnu.org
Cc: Robert Vollmert <rob <at> vllmrt.net>
Subject: [PATCH] guix: import: hackage: handle hackage revisions
Date: Thu, 13 Jun 2019 18:02:29 +0200
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=35750.

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

Revised patch which addresses the formatting and documentation
concernse, but not yet the multiple return values.

 guix/import/cabal.scm   |  7 +++--
 guix/import/hackage.scm | 61 +++++++++++++++++++++++++++++------------
 tests/hackage.scm       | 45 ++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 19 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 <cabal-package>
-  (make-cabal-package name version license home-page source-repository
+  (make-cabal-package name version revision license home-page source-repository
                       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 'custom-setup)
                            ((x) x)
                            (_ #f))))
-      (make-cabal-package name version license home-page-or-hackage
+      (make-cabal-package name version revision license home-page-or-hackage
                           source-repository synopsis description executables lib
                           test-suites flags eval-environment custom-setup)))
 
diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 366256b40d..2853037797 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -117,20 +117,34 @@ version is returned."
           (#f name)
           (m (match:substring m 1)))))))
 
-(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."
+(define (read-cabal-and-hash port)
+  "Read a cabal file and its base32 hash from a port."
+  (let-values (((port get-hash) (open-sha256-input-port port)))
+    (cons (read-cabal (canonical-newline-port port))
+          (bytevector->nix-base32-string (get-hash)))))
+
+(define (hackage-fetch-and-hash name-version)
+  "Return the Cabal file and hash for the package NAME-VERSION, or #f on
+failure. If the version part is omitted from the package name, then return
+the latest version."
   (guard (c ((and (http-get-error? c)
                   (= 404 (http-get-error-code c)))
              #f))                       ;"expected" if package is unknown
     (let-values (((name version) (package-name->name+version name-version)))
       (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))))
 
+(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)))
+
 (define string->license
   ;; List of valid values from
   ;; https://www.haskell.org
@@ -198,15 +212,20 @@ package being processed and is used to filter references to itself."
                                    (cons own-name ghc-standard-libraries))))
           dependencies))
 
-(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #t))
+(define* (hackage-module->sexp cabal cabal-hash
+                               #:key (include-test-dependencies? #t))
   "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
+the hash of the Cabal file."
 
   (define name
     (cabal-package-name cabal))
 
   (define version
     (cabal-package-version cabal))
+
+  (define revision
+    (cabal-package-revision cabal))
   
   (define source-url
     (hackage-source-url name version))
@@ -252,9 +271,14 @@ representation of a Cabal file as produced by 'read-cabal'."
                    (list 'quasiquote inputs))))))
   
   (define (maybe-arguments)
-    (if (not include-test-dependencies?)
-        '((arguments `(#:tests? #f)))
-        '()))
+    (match (append (if (not include-test-dependencies?)
+                       '(#:tests? #f)
+                       '())
+                   (if (not (string-null? revision))
+                       `(#:cabal-revision (,revision ,cabal-hash))
+                       '()))
+      (() '())
+      (args `((arguments (,'quasiquote ,args))))))
 
   (let ((tarball (with-store store
                    (download-to-store store source-url))))
@@ -294,13 +318,16 @@ symbol 'true' or 'false'.  The value associated with 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=> 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=> cabal-meta (compose (cut hackage-module->sexp <>
+                                     cabal-hash
+                                     #:include-test-dependencies?
+                                     include-test-dependencies?)
+                                (cut eval-cabal <> cabal-environment))))
+    (_ #f)))
 
 (define hackage->guix-package/m                   ;memoized variant
   (memoize hackage->guix-package))
diff --git a/tests/hackage.scm b/tests/hackage.scm
index 38a5825af7..14176b2cf9 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -274,6 +274,51 @@ executable cabal
 (test-assert "hackage->guix-package test multiline desc (braced)"
   (eval-test-with-cabal test-cabal-multiline-braced match-ghc-foo))
 
+;; Check Hackage Cabal revisions.
+(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       >= 4000.2.5 && < 4000.3,
+    mtl        >= 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))
+
 (test-assert "read-cabal test 1"
   (match (call-with-input-string test-read-cabal-1 read-cabal)
     ((("name" ("test-me"))
-- 
2.20.1 (Apple Git-117)





Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 16:12:02 GMT) Full text and rfc822 format available.

Message #14 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: Timothy Sample <samplet <at> ngyro.com>
Cc: 36048 <at> debbugs.gnu.org
Subject: Re: [bug#36048] [PATCH] guix: import: hackage: handle hackage
 revisions
Date: Thu, 13 Jun 2019 18:11:00 +0200
Hi Timothy,

thanks for the detailed feedback, this is very helpful!

I’ve sent an updated patch addressing some of the concerns, but have
some questions regarding others. (I just realized that the documentation
updates probably anticipate multiple return values.)

> On 13. Jun 2019, at 04:28, Timothy Sample <samplet <at> ngyro.com> wrote:
>> +  (let-values (((port get-hash) (open-sha256-input-port port)))

>> +    (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 “cons” with “values”).

I tried building it that way to begin with, but I’m having issues
making it work (nicely, or maybe at all). I think it comes down to
later functions optionally failing with a single #f-value. Judging
by the lack of infrastructure, I imagine functions that return different
numbers of values are not idiomatic scheme. Should this be changed to
return two values (#f #f) on failure? Or to raise an exception and
handle it higher up when we want to ignore a failure?

Currently, implementing this with values/let-values results in me
doing more or less a combination of let-values and match, at which
point it seems that any potential benefits of using multiple values
as opposed to a pair/list are lost. (There’s no match-values form is
there?)

Cheers
Rob





Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 18:10:02 GMT) Full text and rfc822 format available.

Message #17 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Timothy Sample <samplet <at> ngyro.com>
To: Robert Vollmert <rob <at> vllmrt.net>
Cc: 36048 <at> debbugs.gnu.org
Subject: Re: [bug#36048] [PATCH] guix: import: hackage: handle hackage
 revisions
Date: Thu, 13 Jun 2019 14:09:10 -0400
[Message part 1 (text/plain, inline)]
Hi Robert,

Robert Vollmert <rob <at> vllmrt.net> writes:

> Hi Timothy,
>
> thanks for the detailed feedback, this is very helpful!

You’re welcome!

> I’ve sent an updated patch addressing some of the concerns, but have
> some questions regarding others. (I just realized that the documentation
> updates probably anticipate multiple return values.)

Yes.

>> On 13. Jun 2019, at 04:28, Timothy Sample <samplet <at> ngyro.com> wrote:
>>> +  (let-values (((port get-hash) (open-sha256-input-port port)))
>
>>> +    (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 “cons” with “values”).
>
> I tried building it that way to begin with, but I’m having issues
> making it work (nicely, or maybe at all). I think it comes down to
> later functions optionally failing with a single #f-value. Judging
> by the lack of infrastructure, I imagine functions that return different
> numbers of values are not idiomatic scheme. Should this be changed to
> return two values (#f #f) on failure? Or to raise an exception and
> handle it higher up when we want to ignore a failure?
>
> Currently, implementing this with values/let-values results in me
> doing more or less a combination of let-values and match, at which
> point it seems that any potential benefits of using multiple values
> as opposed to a pair/list are lost. (There’s no match-values form is
> there?)

I’m not sure I understand the problem.  Here’s what I had in mind:

[values.patch (text/x-patch, inline)]
diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 2853037797..9ba3c4b58e 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -120,8 +120,8 @@ version is returned."
 (define (read-cabal-and-hash port)
   "Read a cabal file and its base32 hash from a port."
   (let-values (((port get-hash) (open-sha256-input-port port)))
-    (cons (read-cabal (canonical-newline-port port))
-          (bytevector->nix-base32-string (get-hash)))))
+    (values (read-cabal (canonical-newline-port port))
+            (bytevector->nix-base32-string (get-hash)))))
 
 (define (hackage-fetch-and-hash name-version)
   "Return the Cabal file and hash for the package NAME-VERSION, or #f on
@@ -129,7 +129,7 @@ failure. If the version part is omitted from the package name, then return
 the latest version."
   (guard (c ((and (http-get-error? c)
                   (= 404 (http-get-error-code c)))
-             #f))                       ;"expected" if package is unknown
+             (values #f #f)))           ;"expected" if package is unknown
     (let-values (((name version) (package-name->name+version name-version)))
       (let* ((url (hackage-cabal-url name version))
              (port (http-fetch url))
@@ -141,9 +141,8 @@ the latest 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)))
+  (let-values (((cabal hash) (hackage-fetch-and-hash name-version)))
+    cabal))
 
 (define string->license
   ;; List of valid values from
@@ -318,16 +317,14 @@ symbol 'true' or 'false'.  The value associated with 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."
-  (match
-    (if port (read-cabal-and-hash port)
-             (hackage-fetch-and-hash package-name))
-    ((cabal-meta . cabal-hash)
-     (and=> cabal-meta (compose (cut hackage-module->sexp <>
-                                     cabal-hash
-                                     #:include-test-dependencies?
-                                     include-test-dependencies?)
-                                (cut eval-cabal <> cabal-environment))))
-    (_ #f)))
+  (let-values (((cabal-meta cabal-hash)
+                (if port
+                    (read-cabal-and-hash (canonical-newline-port port))
+                    (hackage-fetch-and-hash package-name))))
+    (and=> cabal-meta (compose (cut hackage-module->sexp <> cabal-hash
+                                    #:include-test-dependencies?
+                                    include-test-dependencies?)
+                               (cut eval-cabal <> cabal-environment)))))
 
 (define hackage->guix-package/m                   ;memoized variant
   (memoize hackage->guix-package))
[Message part 3 (text/plain, inline)]
As far as I see, this behaves the same as the cons-and-match version.
Did I miss something?

By the way, you make a good point about “match-values”.  That would be
handy.  In general, we love “match” in Guile and in Guix in particular,
but multiple values are part of the Scheme standard – there’s no reason
to avoid them.  They are perfect for situations like this in place of
wrapping the values up into a pair or list and then immediately
unwrapping them.


-- Tim

Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 19:40:02 GMT) Full text and rfc822 format available.

Message #20 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: 36048 <at> debbugs.gnu.org
Cc: Robert Vollmert <rob <at> vllmrt.net>
Subject: [PATCH] guix: import: hackage: handle hackage revisions
Date: Thu, 13 Jun 2019 21:39:14 +0200
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=35750.

* 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 | 69 ++++++++++++++++++++++++++++-------------
 tests/hackage.scm       | 45 +++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 23 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 <cabal-package>
-  (make-cabal-package name version license home-page source-repository
+  (make-cabal-package name version revision license home-page source-repository
                       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 'custom-setup)
                            ((x) x)
                            (_ #f))))
-      (make-cabal-package name version license home-page-or-hackage
+      (make-cabal-package name version revision license home-page-or-hackage
                           source-repository synopsis description executables lib
                           test-suites flags eval-environment custom-setup)))
 
diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 366256b40d..b739b61157 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -117,19 +117,34 @@ version is returned."
           (#f name)
           (m (match:substring m 1)))))))
 
+(define (read-cabal-and-hash port)
+  "Given an input PORT, read a cabal file and its base32 hash from it,
+and return both values."
+  (let-values (((port get-hash) (open-sha256-input-port port)))
+    (values (read-cabal (canonical-newline-port port))
+            (bytevector->nix-base32-string (get-hash)))))
+
+(define (hackage-fetch-and-hash name-version)
+  "Fetch the latest Cabal revision for the package NAME-VERSION, and return
+two values: the parsed Cabal file and its base32 hash. If the version part
+is omitted from the package name, then fetch the latest version. Return #f
+on failure."
+  (guard (c ((and (http-get-error? c)
+                  (= 404 (http-get-error-code c)))
+             (values #f #f)))           ;"expected" if package is unknown
+    (let*-values (((name version) (package-name->name+version name-version))
+                  ((url)          (hackage-cabal-url name version))
+                  ((port)         (http-fetch url))
+                  ((cabal hash)   (read-cabal-and-hash port)))
+      (close-port port)
+      (values cabal hash))))
+
 (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."
-  (guard (c ((and (http-get-error? c)
-                  (= 404 (http-get-error-code c)))
-             #f))                       ;"expected" if package is unknown
-    (let-values (((name version) (package-name->name+version name-version)))
-      (let* ((url (hackage-cabal-url name version))
-             (port (http-fetch url))
-             (result (read-cabal (canonical-newline-port port))))
-        (close-port port)
-        result))))
+  (let-values (((cabal hash) (hackage-fetch-and-hash name-version)))
+    cabal))
 
 (define string->license
   ;; List of valid values from
@@ -198,15 +213,20 @@ package being processed and is used to filter references to itself."
                                    (cons own-name ghc-standard-libraries))))
           dependencies))
 
-(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #t))
+(define* (hackage-module->sexp cabal cabal-hash
+                               #:key (include-test-dependencies? #t))
   "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
+the hash of the Cabal file."
 
   (define name
     (cabal-package-name cabal))
 
   (define version
     (cabal-package-version cabal))
+
+  (define revision
+    (cabal-package-revision cabal))
   
   (define source-url
     (hackage-source-url name version))
@@ -252,9 +272,14 @@ representation of a Cabal file as produced by 'read-cabal'."
                    (list 'quasiquote inputs))))))
   
   (define (maybe-arguments)
-    (if (not include-test-dependencies?)
-        '((arguments `(#:tests? #f)))
-        '()))
+    (match (append (if (not include-test-dependencies?)
+                       '(#:tests? #f)
+                       '())
+                   (if (not (string-null? revision))
+                       `(#:cabal-revision (,revision ,cabal-hash))
+                       '()))
+      (() '())
+      (args `((arguments (,'quasiquote ,args))))))
 
   (let ((tarball (with-store store
                    (download-to-store store source-url))))
@@ -294,13 +319,15 @@ symbol 'true' or 'false'.  The value associated with 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=> cabal-meta (compose (cut hackage-module->sexp <>
-                                    #:include-test-dependencies?
-                                    include-test-dependencies?)
-                               (cut eval-cabal <> cabal-environment)))))
+  (let-values (((cabal hash)
+                (if port
+                    (read-cabal-and-hash port)
+                    (hackage-fetch-and-hash package-name))))
+    (and=> cabal (compose (cut hackage-module->sexp <>
+                               hash
+                               #:include-test-dependencies?
+                               include-test-dependencies?)
+                          (cut eval-cabal <> cabal-environment)))))
 
 (define hackage->guix-package/m                   ;memoized variant
   (memoize hackage->guix-package))
diff --git a/tests/hackage.scm b/tests/hackage.scm
index 38a5825af7..14176b2cf9 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -274,6 +274,51 @@ executable cabal
 (test-assert "hackage->guix-package test multiline desc (braced)"
   (eval-test-with-cabal test-cabal-multiline-braced match-ghc-foo))
 
+;; Check Hackage Cabal revisions.
+(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       >= 4000.2.5 && < 4000.3,
+    mtl        >= 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))
+
 (test-assert "read-cabal test 1"
   (match (call-with-input-string test-read-cabal-1 read-cabal)
     ((("name" ("test-me"))
-- 
2.20.1 (Apple Git-117)





Information forwarded to guix-patches <at> gnu.org:
bug#36048; Package guix-patches. (Thu, 13 Jun 2019 19:41:03 GMT) Full text and rfc822 format available.

Message #23 received at 36048 <at> debbugs.gnu.org (full text, mbox):

From: Robert Vollmert <rob <at> vllmrt.net>
To: Timothy Sample <samplet <at> ngyro.com>
Cc: 36048 <at> debbugs.gnu.org
Subject: Re: [bug#36048] [PATCH] guix: import: hackage: handle hackage
 revisions
Date: Thu, 13 Jun 2019 21:40:15 +0200

> On 13. Jun 2019, at 20:09, Timothy Sample <samplet <at> ngyro.com> wrote:
> 
> Hi Robert,
> 
> Robert Vollmert <rob <at> vllmrt.net> writes:
> 
>> Hi Timothy,
>> 
>> thanks for the detailed feedback, this is very helpful!
> 
> You’re welcome!
> 
>> I’ve sent an updated patch addressing some of the concerns, but have
>> some questions regarding others. (I just realized that the documentation
>> updates probably anticipate multiple return values.)
> 
> Yes.
> 
>>> On 13. Jun 2019, at 04:28, Timothy Sample <samplet <at> ngyro.com> wrote:
>>>> +  (let-values (((port get-hash) (open-sha256-input-port port)))
>> 
>>>> +    (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 “cons” with “values”).
>> 
>> I tried building it that way to begin with, but I’m having issues
>> making it work (nicely, or maybe at all). I think it comes down to
>> later functions optionally failing with a single #f-value. Judging
>> by the lack of infrastructure, I imagine functions that return different
>> numbers of values are not idiomatic scheme. Should this be changed to
>> return two values (#f #f) on failure? Or to raise an exception and
>> handle it higher up when we want to ignore a failure?
>> 
>> Currently, implementing this with values/let-values results in me
>> doing more or less a combination of let-values and match, at which
>> point it seems that any potential benefits of using multiple values
>> as opposed to a pair/list are lost. (There’s no match-values form is
>> there?)
> 
> I’m not sure I understand the problem.  Here’s what I had in mind:
> 
> diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
> index 2853037797..9ba3c4b58e 100644
> --- a/guix/import/hackage.scm
> +++ b/guix/import/hackage.scm
> @@ -120,8 +120,8 @@ version is returned."
> (define (read-cabal-and-hash port)
>   "Read a cabal file and its base32 hash from a port."
>   (let-values (((port get-hash) (open-sha256-input-port port)))
> -    (cons (read-cabal (canonical-newline-port port))
> -          (bytevector->nix-base32-string (get-hash)))))
> +    (values (read-cabal (canonical-newline-port port))
> +            (bytevector->nix-base32-string (get-hash)))))
> 
> (define (hackage-fetch-and-hash name-version)
>   "Return the Cabal file and hash for the package NAME-VERSION, or #f on
> @@ -129,7 +129,7 @@ failure. If the version part is omitted from the package name, then return
> the latest version."
>   (guard (c ((and (http-get-error? c)
>                   (= 404 (http-get-error-code c)))
> -             #f))                       ;"expected" if package is unknown
> +             (values #f #f)))           ;"expected" if package is unknown

This is the point: I tried to keep returning a single #f to signal failure.

I sent an updated patch, let me know if I missed something. Thanks!

Robert






Reply sent to Timothy Sample <samplet <at> ngyro.com>:
You have taken responsibility. (Fri, 14 Jun 2019 02:29:02 GMT) Full text and rfc822 format available.

Notification sent to Robert Vollmert <rob <at> vllmrt.net>:
bug acknowledged by developer. (Fri, 14 Jun 2019 02:29:02 GMT) Full text and rfc822 format available.

Message #28 received at 36048-done <at> debbugs.gnu.org (full text, mbox):

From: Timothy Sample <samplet <at> ngyro.com>
To: Robert Vollmert <rob <at> vllmrt.net>
Cc: 36048-done <at> debbugs.gnu.org
Subject: Re: [bug#36048] [PATCH] guix: import: hackage: handle hackage
 revisions
Date: Thu, 13 Jun 2019 22:28:20 -0400
Hi Robert,

Robert Vollmert <rob <at> vllmrt.net> 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=35750.
>
> * 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 | 69 ++++++++++++++++++++++++++++-------------
>  tests/hackage.scm       | 45 +++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+), 23 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 <cabal-package>
> -  (make-cabal-package name version license home-page source-repository
> +  (make-cabal-package name version revision license home-page source-repository
>                        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 'custom-setup)
>                             ((x) x)
>                             (_ #f))))
> -      (make-cabal-package name version license home-page-or-hackage
> +      (make-cabal-package name version revision license home-page-or-hackage
>                            source-repository synopsis description executables lib
>                            test-suites flags eval-environment custom-setup)))
>  
> diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
> index 366256b40d..b739b61157 100644
> --- a/guix/import/hackage.scm
> +++ b/guix/import/hackage.scm
> @@ -117,19 +117,34 @@ version is returned."
>            (#f name)
>            (m (match:substring m 1)))))))
>  
> +(define (read-cabal-and-hash port)
> +  "Given an input PORT, read a cabal file and its base32 hash from it,
> +and return both values."
> +  (let-values (((port get-hash) (open-sha256-input-port port)))
> +    (values (read-cabal (canonical-newline-port port))
> +            (bytevector->nix-base32-string (get-hash)))))
> +
> +(define (hackage-fetch-and-hash name-version)
> +  "Fetch the latest Cabal revision for the package NAME-VERSION, and return
> +two values: the parsed Cabal file and its base32 hash. If the version part
> +is omitted from the package name, then fetch the latest version. Return #f
> +on failure."
> +  (guard (c ((and (http-get-error? c)
> +                  (= 404 (http-get-error-code c)))
> +             (values #f #f)))           ;"expected" if package is unknown
> +    (let*-values (((name version) (package-name->name+version name-version))
> +                  ((url)          (hackage-cabal-url name version))
> +                  ((port)         (http-fetch url))
> +                  ((cabal hash)   (read-cabal-and-hash port)))
> +      (close-port port)
> +      (values cabal hash))))
> +
>  (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."
> -  (guard (c ((and (http-get-error? c)
> -                  (= 404 (http-get-error-code c)))
> -             #f))                       ;"expected" if package is unknown
> -    (let-values (((name version) (package-name->name+version name-version)))
> -      (let* ((url (hackage-cabal-url name version))
> -             (port (http-fetch url))
> -             (result (read-cabal (canonical-newline-port port))))
> -        (close-port port)
> -        result))))
> +  (let-values (((cabal hash) (hackage-fetch-and-hash name-version)))
> +    cabal))
>  
>  (define string->license
>    ;; List of valid values from
> @@ -198,15 +213,20 @@ package being processed and is used to filter references to itself."
>                                     (cons own-name ghc-standard-libraries))))
>            dependencies))
>  
> -(define* (hackage-module->sexp cabal #:key (include-test-dependencies? #t))
> +(define* (hackage-module->sexp cabal cabal-hash
> +                               #:key (include-test-dependencies? #t))
>    "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
> +the hash of the Cabal file."
>  
>    (define name
>      (cabal-package-name cabal))
>  
>    (define version
>      (cabal-package-version cabal))
> +
> +  (define revision
> +    (cabal-package-revision cabal))
>    
>    (define source-url
>      (hackage-source-url name version))
> @@ -252,9 +272,14 @@ representation of a Cabal file as produced by 'read-cabal'."
>                     (list 'quasiquote inputs))))))
>    
>    (define (maybe-arguments)
> -    (if (not include-test-dependencies?)
> -        '((arguments `(#:tests? #f)))
> -        '()))
> +    (match (append (if (not include-test-dependencies?)
> +                       '(#:tests? #f)
> +                       '())
> +                   (if (not (string-null? revision))
> +                       `(#:cabal-revision (,revision ,cabal-hash))
> +                       '()))
> +      (() '())
> +      (args `((arguments (,'quasiquote ,args))))))
>  
>    (let ((tarball (with-store store
>                     (download-to-store store source-url))))
> @@ -294,13 +319,15 @@ symbol 'true' or 'false'.  The value associated with 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=> cabal-meta (compose (cut hackage-module->sexp <>
> -                                    #:include-test-dependencies?
> -                                    include-test-dependencies?)
> -                               (cut eval-cabal <> cabal-environment)))))
> +  (let-values (((cabal hash)
> +                (if port
> +                    (read-cabal-and-hash port)
> +                    (hackage-fetch-and-hash package-name))))
> +    (and=> cabal (compose (cut hackage-module->sexp <>
> +                               hash
> +                               #:include-test-dependencies?
> +                               include-test-dependencies?)
> +                          (cut eval-cabal <> cabal-environment)))))
>  
>  (define hackage->guix-package/m                   ;memoized variant
>    (memoize hackage->guix-package))
> diff --git a/tests/hackage.scm b/tests/hackage.scm
> index 38a5825af7..14176b2cf9 100644
> --- a/tests/hackage.scm
> +++ b/tests/hackage.scm
> @@ -274,6 +274,51 @@ executable cabal
>  (test-assert "hackage->guix-package test multiline desc (braced)"
>    (eval-test-with-cabal test-cabal-multiline-braced match-ghc-foo))
>  
> +;; Check Hackage Cabal revisions.
> +(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       >= 4000.2.5 && < 4000.3,
> +    mtl        >= 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))
> +
>  (test-assert "read-cabal test 1"
>    (match (call-with-input-string test-read-cabal-1 read-cabal)
>      ((("name" ("test-me"))

Applied as ca45da9fc9b1eee399ce4344b18cbb129daeca4c!

I did make a few changes.  Most notably, I tried importing a package and
it didn’t work!  It turns out that “http-fetch” returns two values, so I
told “let-values” to ignore the second one.  Other than that, I made a
few changes to the commit message and docstrings, and kept “cabal-meta”
and “cabal-hash” from before (instead of “cabal” and “hash”).

Thanks for the patch!


-- Tim




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 12 Jul 2019 11:24:06 GMT) Full text and rfc822 format available.

This bug report was last modified 4 years and 261 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.