[bug#45984] [PATCH 0/5] Fix recursive importers

[bug#45984] [PATCH 0/5] Fix recursive importers

[zimoun]

Dear,

Currently, there is 2 issues:

 1. <from>->guix-package returning #f instead of (values #f '())
 2. error incorrectly handled by guix/scripts/import/<from>

This corner case #1 happens when the package does not exist; then the function
'lookup-node' is not able to "unpack" the 'values' and throw and ugly
backtrace, as exposed in bug#44114 <http://issues.guix.gnu.org/44115#3>.

With these trivial patches, it is fixed for all the importers except 'opam'
(because of 'and-let' which needs some care).

Now, instead of throwing an ugly backtrace, it simply say almost nothing:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import cran do-not-exist -r
error: failed to retrieve package information from "https://cran.r-project.org/web/packages/do-not-exist/DESCRIPTION": 404 ("Not Found")

$ ./pre-inst-env guix import pypi do-not-exist -r
following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
#f
--8<---------------cut here---------------end--------------->8---

This non-existent message is because the error is poorly handled.  With the 4
patches, the situation is the same as "guix import gem" for all the importers
with the recursive option.  One way for a better error handling is done in the
last commit for 'guix import gem' only; the same trick can be done for all.

--8<---------------cut here---------------start------------->8---
$ guix import gem do-not-exist -r
#f

$ ./pre-inst-env guix import gem do-not-exist -r
guix import: error: failed to download meta-data for package 'do-not-exist'
--8<---------------cut here---------------end--------------->8---

In my opinions, UI messages should not appear in guix/import/*.scm but only in
guix/scripts/*.scm.


If I understand correctly, then the way the errors are reported could be
uniformized between all the importers, and maybe the snippet in the subcommands
"guix import <from>" could be refactorized a bit.

WDYT?


All the best,
simon

zimoun (5):
  import: pypi: Return 'values'.
  import: hackage: Return 'values'.
  import: elpa: Return 'values'.
  import: cran: Return 'values'.
  scripts: import: gem: Fix recursive error handling.

 guix/import/cran.scm        |  5 ++---
 guix/import/elpa.scm        |  7 ++-----
 guix/import/hackage.scm     | 16 ++++++++++------
 guix/import/pypi.scm        | 10 +++++-----
 guix/scripts/import/gem.scm | 27 +++++++++++++++------------
 5 files changed, 34 insertions(+), 31 deletions(-)


base-commit: 2d9c6542c804eb2ef3d8934e1e3ab8b24e9bbafb
prerequisite-patch-id: 6ce76af47a26307f4b99162b5ae2b47f5e5f220f
prerequisite-patch-id: 32b0ac51a8fbe72cc9204c5a6d0e2b987af7b7f6
prerequisite-patch-id: 3fa663069818b59ab4d324cc69fabcd62c5a9b50
-- 
2.29.2

[bug#45984] [PATCH 1/5] import: pypi: Return 'values'.

[zimoun]

Fixes partially <https://bugs.gnu.org/44115>.

* guix/import/pypi.scm (pypi->guix-package): Exhaustive 'values' return.
---
 guix/import/pypi.scm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/guix/import/pypi.scm b/guix/import/pypi.scm
index bf4dc50138..56f4f3f589 100644
--- a/guix/import/pypi.scm
+++ b/guix/import/pypi.scm
@@ -9,6 +9,7 @@
 ;;; Copyright © 2020 Lars-Dominik Braun <ldb@leibniz-psychology.org>
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -477,12 +478,10 @@ VERSION, SOURCE-URL, HOME-PAGE, SYNOPSIS, DESCRIPTION, and LICENSE."
 `package' s-expression corresponding to that package, or #f on failure."
      (let* ((project (pypi-fetch package-name))
             (info    (and project (pypi-project-info project))))
-       (and project
+       (if project
             (guard (c ((missing-source-error? c)
                        (let ((package (missing-source-error-package c)))
-                         (leave (G_ "no source release for pypi package ~a ~a~%")
-                                (project-info-name info)
-                                (project-info-version info)))))
+                         (values #f '()))))
               (make-pypi-sexp (project-info-name info)
                               (project-info-version info)
                               (and=> (latest-source-release project)
@@ -493,7 +492,8 @@ VERSION, SOURCE-URL, HOME-PAGE, SYNOPSIS, DESCRIPTION, and LICENSE."
                               (project-info-summary info)
                               (project-info-summary info)
                               (string->license
-                               (project-info-license info)))))))))
+                               (project-info-license info))))
+            (values #f '()))))))
 
 (define (pypi-recursive-import package-name)
   (recursive-import package-name
-- 
2.29.2

[bug#45984] [PATCH 2/5] import: hackage: Return 'values'.

[zimoun]

Fixes partially <https://bugs.gnu.org/44115>.

* guix/import/hackage.scm (hackage->guix-package): Return 'values'.
(hackage-recursive-import): Fix number of arguments.
---
 guix/import/hackage.scm | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/guix/import/hackage.scm b/guix/import/hackage.scm
index 6ca4f65cb0..30769cd255 100644
--- a/guix/import/hackage.scm
+++ b/guix/import/hackage.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2016 Nikita <nikita@n0.is>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net>
 ;;; Copyright © 2019 Robert Vollmert <rob@vllmrt.net>
+;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -335,17 +336,20 @@ respectively."
                 (if port
                     (read-cabal-and-hash port)
                     (hackage-fetch-and-hash package-name))))
-    (and=> cabal-meta (compose (cut hackage-module->sexp <> cabal-hash
-                                    #:include-test-dependencies?
-                                    include-test-dependencies?)
-                               (cut eval-cabal <> cabal-environment)))))
+    (if cabal-meta
+        ((compose (cut hackage-module->sexp <> cabal-hash
+                             #:include-test-dependencies?
+                             include-test-dependencies?)
+                        (cut eval-cabal <> cabal-environment))
+         cabal-meta)
+        (values #f '()))))
 
 (define hackage->guix-package/m                   ;memoized variant
   (memoize hackage->guix-package))
 
 (define* (hackage-recursive-import package-name . args)
-  (recursive-import package-name #f
-                    #:repo->guix-package (lambda (name repo)
+  (recursive-import package-name
+                    #:repo->guix-package (lambda* (name #:key version repo)
                                            (apply hackage->guix-package/m
                                                   (cons name args)))
                     #:guix-name hackage-name->package-name))
-- 
2.29.2

[bug#45984] [PATCH 3/5] import: elpa: Return 'values'.

[zimoun]

Fixes partially <https://bugs.gnu.org/44115>.

* guix/import/elpa.scm (elpa->guix-package): Return values.
---
 guix/import/elpa.scm | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/guix/import/elpa.scm b/guix/import/elpa.scm
index c0dc5acf51..7073533daa 100644
--- a/guix/import/elpa.scm
+++ b/guix/import/elpa.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
 ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
 ;;; Copyright © 2020 Ricardo Wurmus <rekado@elephly.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -396,11 +397,7 @@ type '<elpa-package>'."
   "Fetch the package NAME from REPO and produce a Guix package S-expression."
   (match (fetch-elpa-package name repo)
     (#false
-     (raise (condition
-             (&message
-              (message (format #false
-                               "couldn't find meta-data for ELPA package `~a'."
-                               name))))))
+     (values #f '()))
     (package
       ;; ELPA is known to contain only GPLv3+ code.  Other repos may contain
       ;; code under other license but there's no license metadata.
-- 
2.29.2

[bug#45984] [PATCH 4/5] import: cran: Return 'values'.

[zimoun]

Fixes partially <https://bugs.gnu.org/44115>.

* guix/import/pypi.scm (cran->guix-package): Return 'values'.
---
 guix/import/cran.scm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/guix/import/cran.scm b/guix/import/cran.scm
index fd44d80915..79fc2af5c6 100644
--- a/guix/import/cran.scm
+++ b/guix/import/cran.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2015, 2016, 2017, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -601,9 +602,7 @@ s-expression corresponding to that package, or #f on failure."
               ;; Retry import from CRAN
               (cran->guix-package package-name #:repo 'cran))
              (else
-              (raise (condition
-                      (&message
-                       (message "couldn't find meta-data for R package")))))))))))
+              (values #f '()))))))))
 
 (define* (cran-recursive-import package-name #:key (repo 'cran))
   (recursive-import package-name
-- 
2.29.2

[bug#45984] [PATCH 5/5] scripts: import: gem: Fix recursive error handling.

[zimoun]

Fixes partially <https://bugs.gnu.org/44115>.

* guix/scripts/import/gem.scm (guix-import-gem): Handle error.
---
 guix/scripts/import/gem.scm | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/guix/scripts/import/gem.scm b/guix/scripts/import/gem.scm
index c64596b514..99a2955e4c 100644
--- a/guix/scripts/import/gem.scm
+++ b/guix/scripts/import/gem.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 David Thompson <davet@gnu.org>
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust@gmail.com>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -88,18 +89,20 @@ Import and convert the RubyGems package for PACKAGE-NAME.\n"))
                            (reverse opts))))
     (match args
       ((package-name)
-       (if (assoc-ref opts 'recursive)
-           (map (match-lambda
-                  ((and ('package ('name name) . rest) pkg)
-                   `(define-public ,(string->symbol name)
-                      ,pkg))
-                  (_ #f))
-                (gem-recursive-import package-name 'rubygems))
-           (let ((sexp (gem->guix-package package-name)))
-             (unless sexp
-               (leave (G_ "failed to download meta-data for package '~a'~%")
-                      package-name))
-             sexp)))
+       (let ((code (if (assoc-ref opts 'recursive)
+                      (map (match-lambda
+                             ((and ('package ('name name) . rest) pkg)
+                              `(define-public ,(string->symbol name)
+                                 ,pkg))
+                             (_ #f))
+                           (gem-recursive-import package-name 'rubygems))
+                      (let ((sexp (gem->guix-package package-name)))
+                        (if sexp sexp #f)))))
+         (match code
+           ((or #f '(#f))
+            (leave (G_ "failed to download meta-data for package '~a'~%")
+                  package-name))
+           (_ code))))
       (()
        (leave (G_ "too few arguments~%")))
       ((many ...)
-- 
2.29.2

[bug#45984] [PATCH 0/5] Fix recursive importers

[Ludovic Courtès]

Hi!

zimoun <zimon.toutoune@gmail.com> skribis:

> This corner case #1 happens when the package does not exist; then the function
> 'lookup-node' is not able to "unpack" the 'values' and throw and ugly
> backtrace, as exposed in bug#44114 <http://issues.guix.gnu.org/44115#3>.
>
> With these trivial patches, it is fixed for all the importers except 'opam'
> (because of 'and-let' which needs some care).

Neat!

> Now, instead of throwing an ugly backtrace, it simply say almost nothing:
>
> $ ./pre-inst-env guix import cran do-not-exist -r
> error: failed to retrieve package information from "https://cran.r-project.org/web/packages/do-not-exist/DESCRIPTION": 404 ("Not Found")
>
> $ ./pre-inst-env guix import pypi do-not-exist -r
> following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
> #f
>
>
> This non-existent message is because the error is poorly handled.  With the 4
> patches, the situation is the same as "guix import gem" for all the importers
> with the recursive option.  One way for a better error handling is done in the
> last commit for 'guix import gem' only; the same trick can be done for all.
>
> $ guix import gem do-not-exist -r
> #f
>
> $ ./pre-inst-env guix import gem do-not-exist -r
> guix import: error: failed to download meta-data for package 'do-not-exist'

I think we do want this error message.  Why should we ignore
non-existent packages when doing ‘-r’?  It would think they’re still a
problem, no?

> In my opinions, UI messages should not appear in guix/import/*.scm but only in
> guix/scripts/*.scm.

I agree with the general idea, though sometimes taking this shortcut is
beneficial (maybe not in this case?).

> If I understand correctly, then the way the errors are reported could be
> uniformized between all the importers, and maybe the snippet in the subcommands
> "guix import <from>" could be refactorized a bit.

Probably.  ‘-r’ started as an option specific to one importer, but now
that most of them (?) support it, it’d make sense to rethink the
interfaces.

Thanks for looking into it!

Ludo’.

[bug#45984] [PATCH 0/5] Fix recursive importers

[zimoun]

Hi Ludo,

Thanks for the review.

On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo@gnu.org> wrote:

>> $ guix import gem do-not-exist -r
>> #f
>>
>> $ ./pre-inst-env guix import gem do-not-exist -r
>> guix import: error: failed to download meta-data for package 'do-not-exist'
>
> I think we do want this error message.  Why should we ignore
> non-existent packages when doing ‘-r’?  It would think they’re still a
> problem, no?

Do you mean instead of displaying an error about the query (what the
patch does), displaying which dependent package has failed?  Something
along these lines:

  $ ./pre-inst-env guix import gem foo -r
  guix import: error: failed to download meta-data for package 'bar'


If it is what you have in mind, it needs to really re-think how
’recursive-import’ works.  Not only fixing the corner case. :-)


>> If I understand correctly, then the way the errors are reported could be
>> uniformized between all the importers, and maybe the snippet in the subcommands
>> "guix import <from>" could be refactorized a bit.
>
> Probably.  ‘-r’ started as an option specific to one importer, but now
> that most of them (?) support it, it’d make sense to rethink the
> interfaces.

If we agree on the first part (the function argument “#:key
repo->guix-package” provided to ’recursive-import’ should always return
’values’), then let rethink the interface and how the errors are
handled.


Cheers,
simon

[bug#45984] [PATCH 0/5] Fix recursive importers

[Ludovic Courtès]

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

Hi!

zimoun <zimon.toutoune@gmail.com> skribis:

> On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo@gnu.org> wrote:
>
>>> $ guix import gem do-not-exist -r
>>> #f
>>>
>>> $ ./pre-inst-env guix import gem do-not-exist -r
>>> guix import: error: failed to download meta-data for package 'do-not-exist'
>>
>> I think we do want this error message.  Why should we ignore
>> non-existent packages when doing ‘-r’?  It would think they’re still a
>> problem, no?
>
> Do you mean instead of displaying an error about the query (what the
> patch does), displaying which dependent package has failed?  Something
> along these lines:
>
>   $ ./pre-inst-env guix import gem foo -r
>   guix import: error: failed to download meta-data for package 'bar'
>
>
> If it is what you have in mind, it needs to really re-think how
> ’recursive-import’ works.  Not only fixing the corner case. :-)

I was looking at hunks like this one:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 288 bytes --]

   (match (fetch-elpa-package name repo)
     (#false
-     (raise (condition
-             (&message
-              (message (format #false
-                               "couldn't find meta-data for ELPA package `~a'."
-                               name))))))
+     (values #f '()))

[-- Attachment #3: Type: text/plain, Size: 310 bytes --]


… and I interpreted it as meaning failures would now be silently
ignored.

But I guess what happens is that #f is interpreted by the caller as a
failure, and that’s how we get the “failed to download meta-data”
message, right?

If so, forget my comment.  Sorry for the confusion!

Ludo’.

[bug#45984] [PATCH 0/5] Fix recursive importers

[zimoun]

Hi Ludo,

Thanks for look at it.

On Thu, 28 Jan 2021 at 14:22, Ludovic Courtès <ludo@gnu.org> wrote:

> I was looking at hunks like this one:
>
>    (match (fetch-elpa-package name repo)
>      (#false
> -     (raise (condition
> -             (&message
> -              (message (format #false
> -                               "couldn't find meta-data for ELPA package `~a'."
> -                               name))))))
> +     (values #f '()))
>
> … and I interpreted it as meaning failures would now be silently
> ignored.
>
> But I guess what happens is that #f is interpreted by the caller as a
> failure, and that’s how we get the “failed to download meta-data”
> message, right?

The idea is to remove these error messages in each importer—–here
’elpa->guix-package’ from where the hunk is extracted––and have only one
error message.  For 3 reasons:

 1. because it is simpler
 2. because the message should not be in guix/import/ but guix/scripts/
 3. because it eases the recursive importer error message, IMHO.


Basically, the current situation is:

--8<---------------cut here---------------start------------->8---
$ guix import elpa do-not-exist
Backtrace:
           3 (primitive-load "/home/simon/.config/guix/current/bin/guix")
In guix/ui.scm:
  2154:12  2 (run-guix-command _ . _)
In guix/scripts/import.scm:
   120:11  1 (guix-import . _)
In guix/scripts/import/elpa.scm:
   107:23  0 (guix-import-elpa . _)

guix/scripts/import/elpa.scm:107:23: In procedure guix-import-elpa:
ERROR:
  1. &message: "couldn't find meta-data for ELPA package `do-not-exist'."
--8<---------------cut here---------------end--------------->8---

because the function ’elpa->guix-package’ raises an error poorly handled.

With the patch, the situation becomes:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import elpa do-not-exist
guix import: error: failed to download package 'do-not-exist'
--8<---------------cut here---------------end--------------->8---

And the error message is handled by ’guix/scripts/elpa.scm’ with:

--8<---------------cut here---------------start------------->8---
             (unless sexp
               (leave (G_ "failed to download package '~a'~%") package-name))
             sexp)))
--8<---------------cut here---------------end--------------->8---

Does it make sense?



Now, about the #3.  The current situation is:

--8<---------------cut here---------------start------------->8---
$ guix import elpa do-not-exist -r
guix import: error: couldn't find meta-data for ELPA package `do-not-exist'.
--8<---------------cut here---------------end--------------->8---

So here, the error is correctly handled.  But it means to add error
handler and message to all “guix/import/*.scm“ which is IMHO a bad idea.

Let take the example of ’pypi->guix-package’ to underline my point.

The current situation is:

--8<---------------cut here---------------start------------->8---
$ guix import pypi do-not-exist
following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
guix import: error: failed to download meta-data for package 'do-not-exist'
--8<---------------cut here---------------end--------------->8---

and the error message comes from ’guix/scripts/pypi.scm’.  However, the
recursive fails with:

--8<---------------cut here---------------start------------->8---
$ guix import pypi do-not-exist -r
following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
Backtrace:
           5 (primitive-load "/home/simon/.config/guix/current/bin/guix")
In guix/ui.scm:
  2154:12  4 (run-guix-command _ . _)
In guix/scripts/import.scm:
   120:11  3 (guix-import . _)
In guix/scripts/import/pypi.scm:
    97:16  2 (guix-import-pypi . _)
In guix/import/utils.scm:
   462:31  1 (recursive-import "do-not-exist" #:repo->guix-package _ #:guix-name _ #:version _ #:repo …)
   453:33  0 (lookup-node "do-not-exist" #f)

guix/import/utils.scm:453:33: In procedure lookup-node:
Wrong number of values returned to continuation (expected 2)
--8<---------------cut here---------------end--------------->8---

The reason is that the ’lookup-node’ function in ’recursive-import’ is
expecting ’values’ when ’pypi->guix-package’ return just ’#f’.

--8<---------------cut here---------------start------------->8---
  (define (lookup-node name version)
    (let* ((package dependencies (repo->guix-package name
                                                     #:version version
                                                     #:repo repo))
--8<---------------cut here---------------end--------------->8---

where «repo->guix-package == pypi->guix-package».  And this
’lookup-node’ happens in ’topological-sort’ inside a ’map’.


With the patch, the situation for non-recursive is not changed and for
the recursive it becomes:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import pypi do-not-exist -r
following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
#f
--8<---------------cut here---------------end--------------->8---

where this ’#f’ is from ’guix/scripts/pypi.scm’.  The error message
could be handled here.  An example is done for the ’gem’ importer with
the patch:

   «scripts: import: gem: Fix recursive error handling.»


Does it make sense?


Well, this patch set are trivial changes that quickly fix the current
broken situation without a deep revamp.


All in all, it is worth to rethink all that.  Maybe let drop this patch
set and I could come back with a clean fix.  If no one beats me. :-)

To avoid unnecessary boring work, do we agree that, for these cases,
error messages should happen only in ’guix/scripts/import/’?


Cheers,
simon

PS:
The error with recursive importer would be raised at compile time by a
“typed language” as Typed Racket (to not name OCaml or Haskell).
Just to point an use case about «typed vs weak typed» that we discussed
once on December 2018, drinking a beer pre-Reproducible event.  Ah good
ol’ time with beers in bars, you are missing. ;-)

bug#45984: [PATCH 0/5] Fix recursive importers

[Ludovic Courtès]

Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

> zimoun (5):
>   import: pypi: Return 'values'.
>   import: hackage: Return 'values'.
>   import: elpa: Return 'values'.
>   import: cran: Return 'values'.
>   scripts: import: gem: Fix recursive error handling.

Finally pushed, a year later:

  5278cab3dc scripts: import: gem: Fix recursive error handling.
  7229b0e858 import: cran: Return multiple values for unknown packages.
  1fe81b349c import: elpa: Return multiple values for unknown packages.
  6bb92098b4 import: hackage: Return multiple values for unknown packages.
  434925379d import: pypi: Return multiple values for unknown packages.
  ebb03447f8 import: pypi: Gracefully handle missing project home page.

My questions back then were probably unnecessary; we should have applied
these patches right away, my apologies.

Thanks,
Ludo’.

[bug#45984] [PATCH 0/5] Fix recursive importers

[zimoun]

Hi Ludo,

Thanks for the review and the push.

> > All in all, it is worth to rethink all that.  Maybe let drop this patch
> > set and I could come back with a clean fix.  If no one beats me. :-)

[...]

> My questions back then were probably unnecessary; we should have applied
> these patches right away, my apologies.

Well, a complete revamp for a better error handling is still worth.
Sorry, I have been lazy to finish to unknot all that.  The situation
is better and better with the importer though. :-)


Cheers,
simon