[bug#67564] [PATCH 0/3] Fixes for haskell importers

[bug#67564] [PATCH 0/3] Fixes for haskell importers

[Saku Laesvuori via Guix-patches via]

The first two patches fix crashes in the hackage[1] and stackage
importers.

The third patch improves the cabal file parser so that it can parse a
larger subset of valid cabal files[2]. It fixes guix refresh for 7
packages in Guix.

[1]: https://issues.guix.gnu.org/64734
[2]: https://issues.guix.gnu.org/35743

Saku Laesvuori (3):
  guix: import: hackage: Fix crash on recursive import
  guix: import: stackage: Fix crash on recursive import
  guix: import: Parse cabal layout blocks correctly

 guix/import/cabal.scm    | 42 ++++++++++++++++++----------------------
 guix/import/hackage.scm  |  2 +-
 guix/import/stackage.scm |  2 +-
 3 files changed, 21 insertions(+), 25 deletions(-)


base-commit: cd46757c1a0f886848fbb6828c028dd2a2532767
-- 
2.41.0

[bug#67564] [PATCH 1/3] guix: import: hackage: Fix crash on recursive import

[Saku Laesvuori via Guix-patches via]

Fixes: https://issues.guix.gnu.org/64734

* guix/import/hackage.scm (hackage-module->sexp): Return package names
  instead of <upstream-input> records.

Change-Id: Id428a8b903b4b59d44205ca366324a0a69a4e05b
---
 guix/import/hackage.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 9333bedbbd..bbaee73a06 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -335,7 +335,7 @@ (define* (hackage-module->sexp cabal cabal-hash
         (synopsis ,(cabal-package-synopsis cabal))
         (description ,(beautify-description (cabal-package-description cabal)))
         (license ,(string->license (cabal-package-license cabal))))
-     inputs)))
+     (map upstream-input-name inputs))))
 
 (define* (hackage->guix-package package-name #:key
                                 (include-test-dependencies? #t)

base-commit: cd46757c1a0f886848fbb6828c028dd2a2532767
-- 
2.41.0

[bug#67564] [PATCH 2/3] guix: import: stackage: Fix crash on recursive import

[Saku Laesvuori via Guix-patches via]

* guix/import/stackage.scm (lts-package-version): Call
  stackage-package-version only when the package is found.

Change-Id: Ic8d7c1b7a42a9c1a6cbba567e148706507a53ee3
---
 guix/import/stackage.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/import/stackage.scm b/guix/import/stackage.scm
index 00814c7d46..f801835b33 100644
--- a/guix/import/stackage.scm
+++ b/guix/import/stackage.scm
@@ -92,7 +92,7 @@ (define (lts-package-version packages name)
   "Return the version of the package with upstream NAME included in PACKAGES."
   (let ((pkg (find (lambda (pkg) (string=? (stackage-package-name pkg) name))
                    packages)))
-    (stackage-package-version pkg)))
+    (and=> pkg stackage-package-version)))
 
 \f
 ;;;
-- 
2.41.0

[bug#67564] [PATCH 3/3] guix: import: Parse cabal layout blocks correctly

[Saku Laesvuori via Guix-patches via]

Cabal consideres lines to be part of a layout block if they are indented
at least one space more than the field line the block belongs to.
Previously Guix considered lines to be a part of the block if they were
indented at least as much as the first line in it.

This also makes a workaround that enabled if statements to have multiple
elses redundant and removes it.

Fixes: https://issues.guix.gnu.org/35743

* guix/import/cabal.scm (current-indentation*): Renamed from
current-indentation.
(previous-indentation, current-indentation): New variables.
(make-cabal-parser): Remove outdated comment.
[open]: Use previous-indentation + 1 instead of
current-indentation.
[elif-else]: Split to elif and else to allow only one ELSE in an if
statement.
(read-cabal)[parameterize]: Use current-indentation* and previous-indentation.

Change-Id: I3a1495b1588a022fabbfe8dad9f3231e578af4f3
---
Fixed packages include conduit and warp, for example.

 guix/import/cabal.scm | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/guix/import/cabal.scm b/guix/import/cabal.scm
index fe03c30254..b969197455 100644
--- a/guix/import/cabal.scm
+++ b/guix/import/cabal.scm
@@ -130,8 +130,17 @@ (define (context-stack-pop!) ((context-stack) 'pop!))
 
 (define (context-stack-clear!) ((context-stack) 'clear!))
 
-;; Indentation of the line being parsed.
-(define current-indentation (make-parameter 0))
+;; Indentation of the line being parsed and that of the previous line.
+(define current-indentation* (make-parameter 0))
+
+(define previous-indentation (make-parameter 0))
+
+(define* (current-indentation #:optional value)
+  (if value
+    (begin
+      (previous-indentation (current-indentation*))
+      (current-indentation* value))
+    (current-indentation*)))
 
 ;; Signal to reprocess the beginning of line, in case we need to close more
 ;; than one indentation level.
@@ -196,27 +205,13 @@ (define (make-cabal-parser)
                 (exprs elif-else)          : (append $1 (list ($2 '(()))))
                 (elif-else)                : (list ($1 '(()))))
    ;; LALR(1) parsers prefer to be left-recursive, which make if-statements slightly involved.
-   ;; XXX: This technically allows multiple else statements.
-   (elif-else   (elif-else ELIF tests OCURLY exprs CCURLY) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
-                (elif-else ELIF tests open exprs close) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
-                (elif-else ELSE OCURLY exprs CCURLY) : (lambda (y) ($1 (list $4)))
-                ;; The 'open' token after 'tests' is shifted after an 'exprs'
-                ;; is found.  This is because, instead of 'exprs' a 'OCURLY'
-                ;; token is a valid alternative.  For this reason, 'open'
-                ;; pushes a <parse-context> with a line indentation equal to
-                ;; the indentation of 'exprs'.
-                ;;
-                ;; Differently from this, without the rule above this
-                ;; comment, when an 'ELSE' token is found, the 'open' token
-                ;; following the 'ELSE' would be shifted immediately, before
-                ;; the 'exprs' is found (because there are no other valid
-                ;; tokens).  The 'open' would therefore create a
-                ;; <parse-context> with the indentation of 'ELSE' and not
-                ;; 'exprs', creating an inconsistency.  We therefore allow
-                ;; mixed style conditionals.
-                (elif-else ELSE open exprs close) : (lambda (y) ($1 (list $4)))
+   (elif        (elif ELIF tests OCURLY exprs CCURLY) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
+                (elif ELIF tests open exprs close) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
                 ;; Terminating rule.
                 (if-then) : (lambda (y) (append $1 y)))
+   (elif-else   (elif ELSE OCURLY exprs CCURLY) : (lambda (y) ($1 (list $4)))
+                (elif ELSE open exprs close)    : (lambda (y) ($1 (list $4)))
+                (elif)                          : $1)
    (if-then     (IF tests OCURLY exprs CCURLY) : (list 'if $2 $4)
                 (IF tests open exprs close)    : (list 'if $2 $4))
    (tests       (TEST OPAREN ID CPAREN)        : `(,$1 ,$3)
@@ -237,7 +232,7 @@ (define (make-cabal-parser)
                (OPAREN tests CPAREN)           : $2)
    (open       () : (context-stack-push!
                                    (make-parse-context (context layout)
-                                                       (current-indentation))))
+                                                       (+ 1 (previous-indentation)))))
    (close      (VCCURLY))))
 
 (define (peek-next-line-indent port)
@@ -655,7 +650,8 @@ (define* (read-cabal #:optional (port (current-input-port))
   (let ((cabal-parser (make-cabal-parser)))
     (parameterize ((cabal-file-name
                     (or file-name (port-filename port) "standard input"))
-                   (current-indentation 0)
+                   (current-indentation* 0)
+                   (previous-indentation 0)
                    (check-bol? #f)
                    (context-stack (make-stack)))
       (cabal-parser (make-lexer port) (errorp)))))
-- 
2.41.0

[bug#67564] [PATCH 0/3] Fixes for haskell importers

[Lars-Dominik Braun]

Hi,

> The third patch improves the cabal file parser so that it can parse a
> larger subset of valid cabal files[2]. It fixes guix refresh for 7
> packages in Guix.

which seven packages in Guix are affected? Could you also adapt the
testcases in tests/hackage.scm and add new ones checking the expected
behavior?

Thanks,
Lars

[bug#67564] [PATCH 0/3] Fixes for haskell importers

[Saku Laesvuori via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

> > The third patch improves the cabal file parser so that it can parse a
> > larger subset of valid cabal files[2]. It fixes guix refresh for 7
> > packages in Guix.
> 
> which seven packages in Guix are affected?

- ghc-conduit
- ghc-warp
- ghc-wai-logger
- ghc-streaming-commons
- ghc-persistent-test
- ghc-language-c
- ghc-hasktags

> Could you also adapt the testcases in tests/hackage.scm and add new
> ones checking the expected behavior?

Done in v2. There was already a test for this that was expected to fail.
I just marked it to be expected to pass instead.

- Saku

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

bug#67564: [PATCH 0/3] Fixes for haskell importers

[Lars-Dominik Braun]

Hi,

> Done in v2. There was already a test for this that was expected to fail.
> I just marked it to be expected to pass instead.

yes, you’re right. Merged as:

5bd00bb54235856dddd11e9f0d03481c5469ca63 guix: import: Parse cabal layout blocks correctly
acef524961d4da3464dbc392699fbe7deb0a467b guix: import: stackage: Fix crash on recursive import
160385c013b0403af427b61b1d1cc9a75bc3315d guix: import: hackage: Fix crash on recursive import

Thanks you very much!
Lars

[bug#67564] [PATCH v2 1/3] guix: import: hackage: Fix crash on recursive import

[Saku Laesvuori via Guix-patches via]

Fixes: https://issues.guix.gnu.org/64734

* guix/import/hackage.scm (hackage-module->sexp): Return package names
  instead of <upstream-input> records.

Change-Id: Id428a8b903b4b59d44205ca366324a0a69a4e05b
---
 guix/import/hackage.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 9333bedbbd..bbaee73a06 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -335,7 +335,7 @@ (define* (hackage-module->sexp cabal cabal-hash
         (synopsis ,(cabal-package-synopsis cabal))
         (description ,(beautify-description (cabal-package-description cabal)))
         (license ,(string->license (cabal-package-license cabal))))
-     inputs)))
+     (map upstream-input-name inputs))))
 
 (define* (hackage->guix-package package-name #:key
                                 (include-test-dependencies? #t)

base-commit: cd46757c1a0f886848fbb6828c028dd2a2532767
-- 
2.41.0

[bug#67564] [PATCH v2 2/3] guix: import: stackage: Fix crash on recursive import

[Saku Laesvuori via Guix-patches via]

* guix/import/stackage.scm (lts-package-version): Call
  stackage-package-version only when the package is found.

Change-Id: Ic8d7c1b7a42a9c1a6cbba567e148706507a53ee3
---
 guix/import/stackage.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/import/stackage.scm b/guix/import/stackage.scm
index 00814c7d46..f801835b33 100644
--- a/guix/import/stackage.scm
+++ b/guix/import/stackage.scm
@@ -92,7 +92,7 @@ (define (lts-package-version packages name)
   "Return the version of the package with upstream NAME included in PACKAGES."
   (let ((pkg (find (lambda (pkg) (string=? (stackage-package-name pkg) name))
                    packages)))
-    (stackage-package-version pkg)))
+    (and=> pkg stackage-package-version)))
 
 \f
 ;;;
-- 
2.41.0

[bug#67564] [PATCH v2 3/3] guix: import: Parse cabal layout blocks correctly

[Saku Laesvuori via Guix-patches via]

Cabal consideres lines to be part of a layout block if they are indented
at least one space more than the field line the block belongs to.
Previously Guix considered lines to be a part of the block if they were
indented at least as much as the first line in it.

This also makes a workaround that enabled if statements to have multiple
elses redundant and removes it.

Fixes: https://issues.guix.gnu.org/35743

* guix/import/cabal.scm (current-indentation*): Renamed from
current-indentation.
(previous-indentation, current-indentation): New variables.
(make-cabal-parser): Remove outdated comment.
[open]: Use previous-indentation + 1 instead of
current-indentation.
[elif-else]: Split to elif and else to allow only one ELSE in an if
statement.
(read-cabal)[parameterize]: Use current-indentation* and previous-indentation.
* tests/hackage.scm (hackage->guix-package test mixed layout): Expect to
  pass.

Change-Id: I3a1495b1588a022fabbfe8dad9f3231e578af4f3
---
 guix/import/cabal.scm | 42 +++++++++++++++++++-----------------------
 tests/hackage.scm     |  2 --
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/guix/import/cabal.scm b/guix/import/cabal.scm
index fe03c30254..b969197455 100644
--- a/guix/import/cabal.scm
+++ b/guix/import/cabal.scm
@@ -130,8 +130,17 @@ (define (context-stack-pop!) ((context-stack) 'pop!))
 
 (define (context-stack-clear!) ((context-stack) 'clear!))
 
-;; Indentation of the line being parsed.
-(define current-indentation (make-parameter 0))
+;; Indentation of the line being parsed and that of the previous line.
+(define current-indentation* (make-parameter 0))
+
+(define previous-indentation (make-parameter 0))
+
+(define* (current-indentation #:optional value)
+  (if value
+    (begin
+      (previous-indentation (current-indentation*))
+      (current-indentation* value))
+    (current-indentation*)))
 
 ;; Signal to reprocess the beginning of line, in case we need to close more
 ;; than one indentation level.
@@ -196,27 +205,13 @@ (define (make-cabal-parser)
                 (exprs elif-else)          : (append $1 (list ($2 '(()))))
                 (elif-else)                : (list ($1 '(()))))
    ;; LALR(1) parsers prefer to be left-recursive, which make if-statements slightly involved.
-   ;; XXX: This technically allows multiple else statements.
-   (elif-else   (elif-else ELIF tests OCURLY exprs CCURLY) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
-                (elif-else ELIF tests open exprs close) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
-                (elif-else ELSE OCURLY exprs CCURLY) : (lambda (y) ($1 (list $4)))
-                ;; The 'open' token after 'tests' is shifted after an 'exprs'
-                ;; is found.  This is because, instead of 'exprs' a 'OCURLY'
-                ;; token is a valid alternative.  For this reason, 'open'
-                ;; pushes a <parse-context> with a line indentation equal to
-                ;; the indentation of 'exprs'.
-                ;;
-                ;; Differently from this, without the rule above this
-                ;; comment, when an 'ELSE' token is found, the 'open' token
-                ;; following the 'ELSE' would be shifted immediately, before
-                ;; the 'exprs' is found (because there are no other valid
-                ;; tokens).  The 'open' would therefore create a
-                ;; <parse-context> with the indentation of 'ELSE' and not
-                ;; 'exprs', creating an inconsistency.  We therefore allow
-                ;; mixed style conditionals.
-                (elif-else ELSE open exprs close) : (lambda (y) ($1 (list $4)))
+   (elif        (elif ELIF tests OCURLY exprs CCURLY) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
+                (elif ELIF tests open exprs close) : (lambda (y) ($1 (list (append (list 'if $3 $5) y))))
                 ;; Terminating rule.
                 (if-then) : (lambda (y) (append $1 y)))
+   (elif-else   (elif ELSE OCURLY exprs CCURLY) : (lambda (y) ($1 (list $4)))
+                (elif ELSE open exprs close)    : (lambda (y) ($1 (list $4)))
+                (elif)                          : $1)
    (if-then     (IF tests OCURLY exprs CCURLY) : (list 'if $2 $4)
                 (IF tests open exprs close)    : (list 'if $2 $4))
    (tests       (TEST OPAREN ID CPAREN)        : `(,$1 ,$3)
@@ -237,7 +232,7 @@ (define (make-cabal-parser)
                (OPAREN tests CPAREN)           : $2)
    (open       () : (context-stack-push!
                                    (make-parse-context (context layout)
-                                                       (current-indentation))))
+                                                       (+ 1 (previous-indentation)))))
    (close      (VCCURLY))))
 
 (define (peek-next-line-indent port)
@@ -655,7 +650,8 @@ (define* (read-cabal #:optional (port (current-input-port))
   (let ((cabal-parser (make-cabal-parser)))
     (parameterize ((cabal-file-name
                     (or file-name (port-filename port) "standard input"))
-                   (current-indentation 0)
+                   (current-indentation* 0)
+                   (previous-indentation 0)
                    (check-bol? #f)
                    (context-stack (make-stack)))
       (cabal-parser (make-lexer port) (errorp)))))
diff --git a/tests/hackage.scm b/tests/hackage.scm
index 8eea818ebd..32e5f39329 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -306,8 +306,6 @@ (define test-cabal-mixed-layout
   ghc-options: -Wall
 ")
 
-;; Fails: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35743
-(test-expect-fail 1)
 (test-assert "hackage->guix-package test mixed layout"
   (eval-test-with-cabal test-cabal-mixed-layout match-ghc-foo))
 
-- 
2.41.0