GNU bug report logs - #67564
[PATCH 0/3] Fixes for haskell importers

Previous Next

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

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: guix-patches <at> gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH 0/3] Fixes for haskell importers
Date: Fri,  1 Dec 2023 11:24:57 +0200

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH 1/3] guix: import: hackage: Fix crash on recursive import
Date: Fri,  1 Dec 2023 11:29:46 +0200

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH 2/3] guix: import: stackage: Fix crash on recursive import
Date: Fri,  1 Dec 2023 11:29:47 +0200

* 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)))
 
 
 ;;;
-- 
2.41.0

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH 3/3] guix: import: Parse cabal layout blocks correctly
Date: Fri,  1 Dec 2023 11:29:48 +0200

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

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

From: Lars-Dominik Braun <lars <at> 6xq.net>
To: Saku Laesvuori <saku <at> laesvuori.fi>
Cc: 67564 <at> debbugs.gnu.org
Subject: Re: [bug#67564] [PATCH 0/3] Fixes for haskell importers
Date: Fri, 1 Dec 2023 16:06:45 +0100

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH v2 1/3] guix: import: hackage: Fix crash on recursive import
Date: Sat,  2 Dec 2023 19:23:04 +0200

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH v2 2/3] guix: import: stackage: Fix crash on recursive import
Date: Sat,  2 Dec 2023 19:23:05 +0200

* 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)))
 
 
 ;;;
-- 
2.41.0

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: 67564 <at> debbugs.gnu.org
Cc: Saku Laesvuori <saku <at> laesvuori.fi>
Subject: [PATCH v2 3/3] guix: import: Parse cabal layout blocks correctly
Date: Sat,  2 Dec 2023 19:23:06 +0200

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

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

From: Saku Laesvuori <saku <at> laesvuori.fi>
To: Lars-Dominik Braun <lars <at> 6xq.net>
Cc: 67564 <at> debbugs.gnu.org
Subject: Re: [bug#67564] [PATCH 0/3] Fixes for haskell importers
Date: Sat, 2 Dec 2023 19:27:28 +0200

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

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

From: Lars-Dominik Braun <lars <at> 6xq.net>
To: Saku Laesvuori <saku <at> laesvuori.fi>
Cc: 67564-done <at> debbugs.gnu.org
Subject: Re: [bug#67564] [PATCH 0/3] Fixes for haskell importers
Date: Sun, 3 Dec 2023 09:13:46 +0100

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

Previous Next

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