[bug#36048] [PATCH] guix: import: hackage: handle hackage revisions

[Timothy Sample]

Hi Robert,

Robert Vollmert <address@hidden> 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 <address@hidden> 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
     (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))

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