bug#21829: guix import hackage failures

[Ludovic Courtès]

Federico Beffa <address@hidden> skribis:

> On Fri, Nov 13, 2015 at 10:19 PM, Ludovic Courtès <address@hidden> wrote:
>> Federico Beffa <address@hidden> skribis:

[...]

>> In practice this discards LF even if it’s not following CR; that’s
>> probably a good enough approximation, but an XXX comment would be
>> welcome.
>
> This is intentional because, in my ignorance, I only know of uses of
> '\r' before or after '\n'. Do you know of any other use in text files?

ISTR that some OSes (MacOS 9 and earlier?!  who cares?! :-)) use(d) a
single LF instead of a single CR.

Again that’s fine in practice I guess, but I always think it’s good to
add a note when we make an approximation so we can notice later, just in
case.

> The attached patches fix the parsing of all but two of the failures
> reported by Paul.
> Two cabal files are still not imported correctly because they are buggy:
>
> * streaming-commons: indentation changes from 4 to 2. But this is
> explicitly forbidden. From [1]: "Field names may be indented, but all
> field values in the same section must use the same indentation."
>
> * fgl: uses braces to delimit the value of a field. As far as I
> understand this is not allowed by [1]: "To continue a field value,
> indent the next line relative to the field name." and "Flags,
> conditionals, library and executable sections use layout to indicate
> structure. ... As an alternative to using layout you can also use
> explicit braces {}. ". Thus I understand that braces may be used to
> delimit sections, not field values.

Fair enough!

> Obviously the official 'cabal' program is more permissive than the
> description in the documentation.

We’re more royalist than the king!  ;-)

> From d13f06383d07e0ad4096ff7eb715264463738b0c Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Wed, 11 Nov 2015 10:39:38 +0100
> Subject: [PATCH 1/6] import: hackage: Add recognition of 'true' and 'false'
>  symbols.
>
> * guix/import/cabal.scm (is-true, is-false, lex-true, lex-false): New 
> procedures.
>   (lex-word): Use them.
>   (make-cabal-parser): Add TRUE and FALSE tokens.
>   (eval): Add entries for 'true and 'false symbols.

LGTM.

> From 445f1b6197c0e266027ac033c52629d990137171 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Wed, 11 Nov 2015 11:22:42 +0100
> Subject: [PATCH 2/6] import: hackage: Imporve parsing of tests.
>
> * guix/import/cabal.scm (lex-word): Add support for tests with no spaces.
>   (impl): Fix handling of operator "==".

LGTM, but I think it’d be great to add a test that illustrates the case
that this fixes (and to make sure it doesn’t come back later.)

> From f796d814821289a98e401a3e3df13334a2e8689b Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Wed, 11 Nov 2015 15:31:46 +0100
> Subject: [PATCH 3/6] import: hackage: Make it resilient to missing final
>  newline.
>
> * guix/import/cabal.scm (peek-next-line-indent): Check for missing final
>   newline.

[...]

> +  (if (eof-object? (peek-char port))
> +      ;; If the file is missing the #\newline on the last line, add it and 
> act
> +      ;; as if it were there. This is needed for propoer operation of
                                                      ^^^^
Typo.

> +      ;; indentation based block recognition.
> +      (begin (unread-char #\newline port) (read-char port) 0)

Isn’t this equivalent to: 0 ?

Could you add a test for this one?

> From 225164d2355afd6f9455251d87cbd34b08f68cdb Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Wed, 11 Nov 2015 16:20:45 +0100
> Subject: [PATCH 4/6] import: hackage: Make parsing of tests and fields more
>  flexible.
>
> * guix/import/cabal.scm (is-test): Allow spaces between keyword and
>   parentheses.
>   (is-id): Add argument 'port'.  Allow spaces between keyword and column.
>   (lex-word): Adjust call to 'is-id'.

LGTM, and would be perfect with a test.  ;-)

> From 1b26410f4a7a920382750bffbf5381394acafdbc Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Sat, 14 Nov 2015 15:00:36 +0100
> Subject: [PATCH 5/6] utils: Add 'canonical-newline-port'.
>
> * guix/utils.scm (canonical-newline-port): New procedure.
> * tests/utils.scm ("canonical-newline-port"): New test.

[...]

> +(test-equal "canonical-newline-port"
> +  "This is a journey"
> +  (let ((port (open-string-input-port
> +               "This is a journey\r\n")))
> +    (get-line (canonical-newline-port port))))

I would rather use ‘get-string-all’ and make sure the result is exactly:

  "This is a journey\n"

(Because ‘get-line’ could have been doing its own thing regardless of
the EOL style.)

A test with several lines, including lines with just \n would be nice.

> From c57be8cae9b3642beff1462acd32a0aee54ad7c6 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Sat, 14 Nov 2015 15:15:00 +0100
> Subject: [PATCH 6/6] import: hackage: Handle CRLF end of line style.
>
> * guix/import/hackage.scm (hackage-fetch, hackage->guix-package): Do it.

Rather “Use ‘canonical-newline-port’.” instead of “Do it.”

Thanks for all the work!

Ludo’.