[PATCH 0/3] Fixes for haskell importers

  • Done
  • quality assurance status badge
Details
2 participants
  • Lars-Dominik Braun
  • Saku Laesvuori
Owner
unassigned
Submitted by
Saku Laesvuori
Severity
normal
S
S
Saku Laesvuori wrote on 1 Dec 2023 10:24
(address . guix-patches@gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
cover.1701421819.git.saku@laesvuori.fi
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.


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
S
S
Saku Laesvuori wrote on 1 Dec 2023 10:29
[PATCH 1/3] guix: import: hackage: Fix crash on recursive import
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
1e1cc11c9361ab747435c2159355d14f6046d41d.1701422983.git.saku@laesvuori.fi

* 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(-)

Toggle diff (17 lines)
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
S
S
Saku Laesvuori wrote on 1 Dec 2023 10:29
[PATCH 2/3] guix: import: stackage: Fix crash on recursive import
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
feedeefc26f2e5d3139b66071e83ecf513d22a4e.1701422983.git.saku@laesvuori.fi
* 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(-)

Toggle diff (15 lines)
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
S
S
Saku Laesvuori wrote on 1 Dec 2023 10:29
[PATCH 3/3] guix: import: Parse cabal layout blocks correctly
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
541836669e170dcef15c5e73a5413592d3fbcc25.1701422983.git.saku@laesvuori.fi
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.


* 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(-)

Toggle diff (78 lines)
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
L
L
Lars-Dominik Braun wrote on 1 Dec 2023 16:06
Re: [bug#67564] [PATCH 0/3] Fixes for haskell importers
(name . Saku Laesvuori)(address . saku@laesvuori.fi)(address . 67564@debbugs.gnu.org)
ZWn2haLWV7N9zort@noor.fritz.box
Hi,

Toggle quote (4 lines)
> 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
S
S
Saku Laesvuori wrote on 2 Dec 2023 18:23
[PATCH v2 1/3] guix: import: hackage: Fix crash on recursive import
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
1e1cc11c9361ab747435c2159355d14f6046d41d.1701537651.git.saku@laesvuori.fi

* 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(-)

Toggle diff (17 lines)
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
S
S
Saku Laesvuori wrote on 2 Dec 2023 18:23
[PATCH v2 2/3] guix: import: stackage: Fix crash on recursive import
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
feedeefc26f2e5d3139b66071e83ecf513d22a4e.1701537651.git.saku@laesvuori.fi
* 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(-)

Toggle diff (15 lines)
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
S
S
Saku Laesvuori wrote on 2 Dec 2023 18:23
[PATCH v2 3/3] guix: import: Parse cabal layout blocks correctly
(address . 67564@debbugs.gnu.org)(name . Saku Laesvuori)(address . saku@laesvuori.fi)
d72b586f76a03b9bfa1e17698c9f58312dbea147.1701537651.git.saku@laesvuori.fi
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.


* 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(-)

Toggle diff (91 lines)
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
S
S
Saku Laesvuori wrote on 2 Dec 2023 18:27
Re: [bug#67564] [PATCH 0/3] Fixes for haskell importers
(name . Lars-Dominik Braun)(address . lars@6xq.net)(address . 67564@debbugs.gnu.org)
x54ow4nedj36izmr7aqzivzpm42nkj46sz3yyy2celravzfkwu@4jahbsnbarin
Toggle quote (6 lines)
> > 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

Toggle quote (3 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEoMkZR3NPB29fCOn/JX0oSiodOjIFAmVraP8ACgkQJX0oSiod
OjLuDBAAgvLLOR3mx+SscRc8AWw+RGSFylHWrlVdf3H6lUGfLnTN2l1XvDNisM/1
F8pGmIllgb/PjEEszyJeAlmgeYWMUJvdWx5V4sY6kgB0qz/J3J5f4eJleE8RIohy
F4dB7TH9B6YzLrrruoUMeYbSpH0kumMhT3nRR9iOr7Kmrj1Cv24yjwrPGolt5dkX
Jtm0vjAw5FJOCb5s5h7j5C46SyFqerOi5pD+Ud6alW7AHJDZ74ER9p+hSnc2pCcc
9IsJCNENokNlmIGnzXDUZxW7BSOeIi7mgxC52u7Bx94pHjEmBsrfOXXCyNubWPnh
ch0VPAd/BnlDHWZ8toaCeEYq61PyAu9/04qa+OPN3pjEIPx9JphnnuCXenEzLZ9p
20L3zyHvJnAfLeXC9U6zO6gHVBgsoetIzWgqUZoZ1adnC5nPMdqQ/T8Q95OF6D2+
OrN8MeROisRV2K2GwCMnrft9wNbZNN0qpE4PJMexNUH7SZJjwSc4fLD9DYokPamg
W8ji458muKl0eBYaISQcO/+9kOCviIbVpl4bPBqPP/Ki1Wj7HMbxG4ff1feRJdXJ
BYsL8W6ADok6os/kGwYFgq071W3HJIvJVqp0c/nGyeirf1inowB2nT2yFr0TwP2c
BQnr71GM0HCyoUqrjkN7J5by49hhwUvarM4dq5I8KMfEvsdCUTM=
=Or4G
-----END PGP SIGNATURE-----


L
L
Lars-Dominik Braun wrote on 3 Dec 2023 09:13
(name . Saku Laesvuori)(address . saku@laesvuori.fi)(address . 67564-done@debbugs.gnu.org)
ZWw4ugrDqSk9eU0v@noor.fritz.box
Hi,

Toggle quote (3 lines)
> 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
Closed
?