bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Philip Munksgaard]

The attoparsec package on hackage defines multiple internal libraries inside one package, named "attoparsec" and "attoparsec-internal", with the first depending on the latter. Importing attoparsec using `guix import hackage attoparsec` therefore yields the following erroneous package definition:

(define-public ghc-attoparsec
  (package
    (name "ghc-attoparsec")
    (version "0.14.4")
    (source
      (origin
        (method url-fetch)
        (uri (hackage-uri "attoparsec" version))
        (sha256
          (base32 "0v4yjz4qi8bwhbyavqxlhsfb1iv07v10gxi64khmsmi4hvjpycrz"))))
    (build-system haskell-build-system)
    (inputs (list ghc-scientific ghc-attoparsec-internal))
    (native-inputs
      (list ghc-quickcheck
            ghc-quickcheck-unicode
            ghc-tasty
            ghc-tasty-quickcheck
            ghc-vector))
    (arguments
      `(#:cabal-revision
        ("1" "149ihklmwnl13mmixq6iq5gzggkgqwsqrjlg2fshqwwbvbd4nn3r")))
    (home-page "https://github.com/bgamari/attoparsec")
    (synopsis "Fast combinator parsing for bytestrings and text")
    (description
      "This package provides a fast parser combinator library, aimed particularly at
dealing efficiently with network protocols and complicated text/binary file
formats.")
    (license license:bsd-3)))

Note that `ghc-attoparsec-internal` is listed as a dependency of `ghc-attoparsec`. I believe that is incorrect, and that we should filter out the internal dependencies from `inputs`, just like we filter out `ghc-attoparsec` from the list of `native-inputs`.

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Lars-Dominik Braun]

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

Hello Philip,

> The attoparsec package on hackage defines multiple internal libraries inside one package, named "attoparsec" and "attoparsec-internal", with the first depending on the latter. Importing attoparsec using `guix import hackage attoparsec` therefore yields the following erroneous package definition: […]

attached patches should fix this. I tried them with both, attoparsec and
raaz, and internal libraries appear neither in inputs nor native inputs
any more.

Cheers,
Lars


[-- Attachment #2: 0001-import-cabal-Support-library-names.patch --]
[-- Type: text/plain, Size: 7160 bytes --]

From ee1e0051b15185b6d3cc808e66979369027b5e7b Mon Sep 17 00:00:00 2001
From: Lars-Dominik Braun <lars@6xq.net>
Date: Sun, 22 May 2022 10:56:01 +0200
Subject: [PATCH 1/2] import: cabal: Support library names

* guix/import/cabal.scm (make-cabal-parser): Add name to section.
(is-lib): Add optional name to regular expression.
(lex-rx-res): Support selecting different substring.
(lex-lib): Match 2nd substring from IS-LIB.
(lex-line): Adapt to changes for lex-lib.
(cabal-library): Add name field and export CABAL-LIBRARY-NAME.
(eval): Remove special case for 'library, which is not required any more.
(make-cabal-section): Move special case for LIBRARY.
* tests/hackage.scm (test-read-cabal-library-name): New variable.
("read-cabal test 1"): Adapt testcase to changed internal structure.
("read-cabal test: library name"): New testcase.
---
 guix/import/cabal.scm | 27 ++++++++++++++-------------
 tests/hackage.scm     | 21 ++++++++++++++++++++-
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/guix/import/cabal.scm b/guix/import/cabal.scm
index 8f59a63cb9..4410c12500 100644
--- a/guix/import/cabal.scm
+++ b/guix/import/cabal.scm
@@ -74,6 +74,7 @@ (define-module (guix import cabal)
             cabal-executable-dependencies
 
             cabal-library?
+            cabal-library-name
             cabal-library-dependencies
 
             cabal-test-suite?
@@ -189,8 +190,8 @@ (define (make-cabal-parser)
                 (bm-sec)                : (list $1))
    (bm-sec      (BENCHMARK OCURLY exprs CCURLY) : `(section benchmark ,$1 ,$3)
                 (BENCHMARK open exprs close)    : `(section benchmark ,$1 ,$3))
-   (lib-sec     (LIB OCURLY exprs CCURLY) : `(section library ,$3)
-                (LIB open exprs close)    : `(section library ,$3))
+   (lib-sec     (LIB OCURLY exprs CCURLY) : `(section library ,$1 ,$3)
+                (LIB open exprs close)    : `(section library ,$1 ,$3))
    (exprs       (exprs PROPERTY)         : (append $1 (list $2))
                 (PROPERTY)               : (list $1)
                 (exprs elif-else)          : (append $1 (list ($2 '(()))))
@@ -382,7 +383,8 @@ (define is-custom-setup (make-rx-matcher "^(custom-setup)"
 (define is-benchmark (make-rx-matcher "^benchmark +([a-z0-9_-]+)"
                                       regexp/icase))
 
-(define is-lib (make-rx-matcher "^library *" regexp/icase))
+;; Libraries can have optional names since Cabal 2.0.
+(define is-lib (make-rx-matcher "^library(\\s+([a-z0-9_-]+))?\\s*" regexp/icase))
 
 (define (is-else s) (string-ci=? s "else"))
 
@@ -476,8 +478,9 @@ (define (lex-braced-property k-rx-res loc port)
      'PROPERTY loc
      (list key `(,(read-braced-value port))))))
 
-(define (lex-rx-res rx-res token loc)
-  (let ((name (string-downcase (match:substring rx-res 1))))
+(define* (lex-rx-res rx-res token loc #:optional (substring-id 1))
+  (let* ((match (match:substring rx-res substring-id))
+         (name (if match (string-downcase match) match)))
     (make-lexical-token token loc name)))
 
 (define (lex-flag flag-rx-res loc) (lex-rx-res flag-rx-res 'FLAG loc))
@@ -495,7 +498,7 @@ (define (lex-custom-setup ts-rx-res loc) (lex-rx-res ts-rx-res 'CUSTOM-SETUP loc
 
 (define (lex-benchmark bm-rx-res loc) (lex-rx-res bm-rx-res 'BENCHMARK loc))
 
-(define (lex-lib loc) (make-lexical-token 'LIB loc #f))
+(define (lex-lib lib-rx-res loc) (lex-rx-res lib-rx-res 'LIB loc 2))
 
 (define (lex-else loc) (make-lexical-token 'ELSE loc #f))
 
@@ -599,7 +602,7 @@ (define (lex-line port loc)
      ((is-common s) => (cut lex-common <> loc))
      ((is-custom-setup s) => (cut lex-custom-setup <> loc))
      ((is-benchmark s) => (cut lex-benchmark <> loc))
-     ((is-lib s) (lex-lib loc))
+     ((is-lib s) => (cut lex-lib <> loc))
      (else (unread-string s port) #f))))
 
 (define (lex-property port loc)
@@ -729,8 +732,9 @@ (define-record-type <cabal-executable>
   (dependencies cabal-executable-dependencies)) ; list of <cabal-dependency>
 
 (define-record-type <cabal-library>
-  (make-cabal-library dependencies)
+  (make-cabal-library name dependencies)
   cabal-library?
+  (name cabal-library-name)
   (dependencies cabal-library-dependencies)) ; list of <cabal-dependency>
 
 (define-record-type <cabal-test-suite>
@@ -861,9 +865,6 @@ (define (eval sexp)
        (list 'section 'flag name parameters))
       (('section 'custom-setup parameters)
        (list 'section 'custom-setup parameters))
-      ;; library does not have a name parameter
-      (('section 'library parameters)
-       (list 'section 'library (eval parameters)))
       (('section type name parameters)
        (list 'section type name (eval parameters)))
       (((? string? name) values)
@@ -923,6 +924,8 @@ (define (make-cabal-section sexp section-type)
                                             name
                                             (lookup-join parameters "type")
                                             (lookup-join parameters "location")))
+                      ((library) (make-cabal-library name
+                                                     (dependencies parameters)))
                       ((flag)
                        (let* ((default (lookup-join parameters "default"))
                               (default-true-or-false
@@ -939,8 +942,6 @@ (define (make-cabal-section sexp section-type)
                                           default-true-or-false
                                           manual-true-or-false)))
                       (else #f)))
-                   (('section (? (cut equal? <> section-type) lib) parameters)
-                    (make-cabal-library (dependencies parameters)))
                    (_ #f))
               sexp))
 
diff --git a/tests/hackage.scm b/tests/hackage.scm
index d7ecd0cc21..85a5c2115c 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -172,6 +172,15 @@ (define test-read-cabal-brackets-newline
     }
 ")
 
+;; Test library with (since Cabal 2.0) and without names.
+(define test-read-cabal-library-name
+  "name: test-me
+library foobar
+    build-depends: foo, bar
+library
+    build-depends: bar, baz
+")
+
 (test-begin "hackage")
 
 (define-syntax-rule (define-package-matcher name pattern)
@@ -507,7 +516,7 @@ (define-package-matcher match-ghc-foo-revision
 (test-assert "read-cabal test 1"
   (match (call-with-input-string test-read-cabal-1 read-cabal)
     ((("name" ("test-me"))
-      ('section 'library
+      ('section 'library #f
                 (('if ('flag "base4point8")
                       (("build-depends" ("base >= 4.8 && < 5")))
                       (('if ('flag "base4")
@@ -543,6 +552,16 @@ (define-package-matcher match-ghc-foo-revision
      #t)
     (x (pk 'fail x #f))))
 
+(test-assert "read-cabal test: library name"
+  (match (call-with-input-string test-read-cabal-library-name read-cabal)
+    ((("name" ("test-me"))
+        ('section 'library "foobar"
+          (("build-depends" ("foo, bar"))))
+        ('section 'library #f
+          (("build-depends" ("bar, baz")))))
+     #t)
+    (x (pk 'fail x #f))))
+
 (define test-cabal-import
   "name: foo
 version: 1.0.0
-- 
2.35.1


[-- Attachment #3: 0002-import-hackage-Filter-internal-libraries-from-inputs.patch --]
[-- Type: text/plain, Size: 3661 bytes --]

From 1aa0732e8995c3872ddbb67c1a845d39cc8ce6f9 Mon Sep 17 00:00:00 2001
From: Lars-Dominik Braun <lars@6xq.net>
Date: Sun, 22 May 2022 11:20:07 +0200
Subject: [PATCH 2/2] import: hackage: Filter internal libraries from inputs
 and native-inputs.

* guix/import/hackage.scm (filter-dependencies): Support multiple
OWN-NAMES.
(hackage-module->sexp): Filter OWN-NAMES from HACKAGE-DEPENDENCIES and
HACKAGE-NATIVE-DEPENDENCIES.
* tests/hackage.scm (test-cabal-internal-library-ignored): New variable.
("hackage->guix-package test internal libraries are ignored"): New testcase.
---
 guix/import/hackage.scm | 17 ++++++++++-------
 tests/hackage.scm       | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 0d6c77e399..6e982366cf 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -222,12 +222,13 @@ (define (cabal-custom-setup-dependencies->names cabal)
                                         '())))
     (map cabal-dependency-name custom-setup-dependencies)))
 
-(define (filter-dependencies dependencies own-name)
+(define (filter-dependencies dependencies own-names)
   "Filter the dependencies included with the GHC compiler from DEPENDENCIES, a
-list with the names of dependencies.  OWN-NAME is the name of the Cabal
-package being processed and is used to filter references to itself."
+list with the names of dependencies.  OWN-NAMES is the name of the Cabal
+package being processed and its internal libaries and is used to filter
+references to itself."
   (filter (lambda (d) (not (member (string-downcase d)
-                                   (cons own-name ghc-standard-libraries))))
+                                   (append own-names ghc-standard-libraries))))
           dependencies))
 
 (define* (hackage-module->sexp cabal cabal-hash
@@ -248,9 +249,11 @@ (define revision
   (define source-url
     (hackage-source-url name version))
 
+  (define own-names (cons (cabal-package-name cabal)
+                          (map cabal-library-name (cabal-package-library cabal))))
+
   (define hackage-dependencies
-    (filter-dependencies (cabal-dependencies->names cabal)
-                         (cabal-package-name cabal)))
+    (filter-dependencies (cabal-dependencies->names cabal) own-names))
 
   (define hackage-native-dependencies
     (lset-difference
@@ -260,7 +263,7 @@ (define hackage-native-dependencies
                   (cabal-test-dependencies->names cabal)
                   '())
               (cabal-custom-setup-dependencies->names cabal))
-      (cabal-package-name cabal))
+      own-names)
      hackage-dependencies))
 
   (define dependencies
diff --git a/tests/hackage.scm b/tests/hackage.scm
index 85a5c2115c..a11dd14846 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -368,6 +368,25 @@ (define test-cabal-no-final-newline
 (test-assert "hackage->guix-package test without final newline"
   (eval-test-with-cabal test-cabal-no-final-newline match-ghc-foo))
 
+;; Make sure internal libraries will not be part of the dependencies.
+(define test-cabal-internal-library-ignored
+  "name: foo
+version: 1.0.0
+homepage: http://test.org
+synopsis: synopsis
+description: description
+license: BSD3
+executable cabal
+  build-depends:
+    HTTP       >= 4000.2.5 && < 4000.3,
+    internal
+library internal
+  build-depends: mtl        >= 2.0      && < 3
+")
+
+(test-assert "hackage->guix-package test internal libraries are ignored"
+  (eval-test-with-cabal test-cabal-internal-library-ignored match-ghc-foo))
+
 ;; Check if-elif-else statements
 (define test-cabal-if
   "name: foo
-- 
2.35.1

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Philip Munksgaard]

Hi Lars,

On Sun, 22 May 2022, at 11:37, Lars-Dominik Braun wrote:
>> The attoparsec package on hackage defines multiple internal libraries inside one package, named "attoparsec" and "attoparsec-internal", with the first depending on the latter. Importing attoparsec using `guix import hackage attoparsec` therefore yields the following erroneous package definition: […]
>
> attached patches should fix this. I tried them with both, attoparsec and
> raaz, and internal libraries appear neither in inputs nor native inputs
> any more.
>

Indeed, that seems to work for those packages, but it still doesn't seem to work for OneTuple, as mentioned here: https://issues.guix.gnu.org/52152#3

Also, attoparsec still won't build with result of the import, but for different reasons.

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Philip Munksgaard]

On Thu, 2 Jun 2022, at 11:53, Philip Munksgaard wrote:
> Also, attoparsec still won't build with result of the import, but for 
> different reasons.

Actually, that's weird. Didn't attoparsec used to build?

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Philip Munksgaard]

On Thu, 2 Jun 2022, at 11:59, Philip Munksgaard wrote:
> On Thu, 2 Jun 2022, at 11:53, Philip Munksgaard wrote:
>> Also, attoparsec still won't build with result of the import, but for 
>> different reasons.
>
> Actually, that's weird. Didn't attoparsec used to build?

Aha! The upgrade to GHC 9.0.0 caused ghc-hashable to break. I suppose many other packages could be broken as well.

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Lars-Dominik Braun]

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

Hey,

> Indeed, that seems to work for those packages, but it still doesn't seem to work for OneTuple, as mentioned here:
I’m attaching a patch for that, please have a look. I’ll merge these
into master, since they don’t change any builds.

> Aha! The upgrade to GHC 9.0.0 caused ghc-hashable to break. I suppose many other packages could be broken as well.
I didn’t actually upgrade any packages on wip-haskell yet, so it’s
likely they fail with a newer GHC than 8.10.

Cheers,
Lars


[-- Attachment #2: 0001-import-hackage-Filter-upper-mixed-case-dependencies-.patch --]
[-- Type: text/plain, Size: 2805 bytes --]

From 261736187d51c85c203ad08fbc1ae89340256a8c Mon Sep 17 00:00:00 2001
From: Lars-Dominik Braun <lars@6xq.net>
Date: Thu, 2 Jun 2022 13:52:08 +0200
Subject: [PATCH] import: hackage: Filter upper/mixed case dependencies too.

* guix/import/hackage.scm (filter-dependencies): Convert OWN-NAMES
to lowercase before filtering.
(hackage-module->sexp): Remove #f from OWN-NAMES, which is used for
unnamed (default) libraries.
* tests/hackage.scm (test-cabal-internal-library-ignored): Add mismatched
uppercase letters.
---
 guix/import/hackage.scm | 8 +++++---
 tests/hackage.scm       | 7 ++++---
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 6e982366cf..878a7d2f9c 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -227,9 +227,10 @@ (define (filter-dependencies dependencies own-names)
 list with the names of dependencies.  OWN-NAMES is the name of the Cabal
 package being processed and its internal libaries and is used to filter
 references to itself."
-  (filter (lambda (d) (not (member (string-downcase d)
+  (let ((ignored-dependencies (map string-downcase
                                    (append own-names ghc-standard-libraries))))
-          dependencies))
+    (filter (lambda (d) (not (member (string-downcase d) ignored-dependencies)))
+            dependencies)))
 
 (define* (hackage-module->sexp cabal cabal-hash
                                #:key (include-test-dependencies? #t))
@@ -250,7 +251,8 @@ (define source-url
     (hackage-source-url name version))
 
   (define own-names (cons (cabal-package-name cabal)
-                          (map cabal-library-name (cabal-package-library cabal))))
+                          (filter (lambda (x) (not (eqv? x #f)))
+                            (map cabal-library-name (cabal-package-library cabal)))))
 
   (define hackage-dependencies
     (filter-dependencies (cabal-dependencies->names cabal) own-names))
diff --git a/tests/hackage.scm b/tests/hackage.scm
index a11dd14846..ad2ee4b7f9 100644
--- a/tests/hackage.scm
+++ b/tests/hackage.scm
@@ -368,7 +368,8 @@ (define test-cabal-no-final-newline
 (test-assert "hackage->guix-package test without final newline"
   (eval-test-with-cabal test-cabal-no-final-newline match-ghc-foo))
 
-;; Make sure internal libraries will not be part of the dependencies.
+;; Make sure internal libraries will not be part of the dependencies,
+;; ignore case.
 (define test-cabal-internal-library-ignored
   "name: foo
 version: 1.0.0
@@ -379,8 +380,8 @@ (define test-cabal-internal-library-ignored
 executable cabal
   build-depends:
     HTTP       >= 4000.2.5 && < 4000.3,
-    internal
-library internal
+    internAl
+library internaL
   build-depends: mtl        >= 2.0      && < 3
 ")
 
-- 
2.35.1

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Philip Munksgaard]

Hi again,

On Thu, 2 Jun 2022, at 13:57, Lars-Dominik Braun wrote:
> Hey,
>
>> Indeed, that seems to work for those packages, but it still doesn't seem to work for OneTuple, as mentioned here:
> I’m attaching a patch for that, please have a look. I’ll merge these
> into master, since they don’t change any builds.

That works, thanks!

>> Aha! The upgrade to GHC 9.0.0 caused ghc-hashable to break. I suppose many other packages could be broken as well.
> I didn’t actually upgrade any packages on wip-haskell yet, so it’s
> likely they fail with a newer GHC than 8.10.

Aha. Let me know if there's anything I can do to help.

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Lars-Dominik Braun]

Hi Philip,

> Aha. Let me know if there's anything I can do to help.
at this point there’s two options:

1) We manually update all packages coming from Stackage (i.e. run `guix
refresh -t stackage` and adjust #:cabal-revision/inputs by hand)
2) We improve the updater so it automatically adjusts the entire package
– not just version and source – and then run `guix refresh`.

1 is more labor-intensive, but 2 might take more time to figure out.

Which option can you help with?

Thanks,
Lars

bug#54760: import: hackage: Internal libraries are not filtered out of dependency list

[Lars-Dominik Braun]

Hi,

fix pushed to master as c3fbaee34548fbfb1617dc7fccc94c598efbd7a6 and following.

Cheers,
Lars