GNU bug report logs - #45984
[PATCH 0/5] Fix recursive 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 45984 in the body.
You can then email your comments to 45984 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: zimoun <zimon.toutoune <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 0/5] Fix recursive importers
Date: Tue, 19 Jan 2021 16:45:25 +0100

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

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: 45984 <at> debbugs.gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 1/5] import: pypi: Return 'values'.
Date: Tue, 19 Jan 2021 16:47:17 +0100

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 <at> leibniz-psychology.org>
 ;;; Copyright © 2020 Arun Isaac <arunisaac <at> systemreboot.net>
 ;;; Copyright © 2020 Martin Becze <mjbecze <at> riseup.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune <at> 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

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: 45984 <at> debbugs.gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 2/5] import: hackage: Return 'values'.
Date: Tue, 19 Jan 2021 16:47:18 +0100

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 <at> n0.is>
 ;;; Copyright © 2018 Ricardo Wurmus <rekado <at> elephly.net>
 ;;; Copyright © 2019 Robert Vollmert <rob <at> vllmrt.net>
+;;; Copyright © 2019 Simon Tournier <zimon.toutoune <at> 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

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: 45984 <at> debbugs.gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 3/5] import: elpa: Return 'values'.
Date: Tue, 19 Jan 2021 16:47:19 +0100

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 <at> gmail.com>
 ;;; Copyright © 2020 Martin Becze <mjbecze <at> riseup.net>
 ;;; Copyright © 2020 Ricardo Wurmus <rekado <at> elephly.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune <at> 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

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: 45984 <at> debbugs.gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 4/5] import: cran: Return 'values'.
Date: Tue, 19 Jan 2021 16:47:20 +0100

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 <at> gnu.org>
 ;;; Copyright © 2017 Mathieu Othacehe <m.othacehe <at> gmail.com>
 ;;; Copyright © 2020 Martin Becze <mjbecze <at> riseup.net>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune <at> 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

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: 45984 <at> debbugs.gnu.org
Cc: zimoun <zimon.toutoune <at> gmail.com>
Subject: [PATCH 5/5] scripts: import: gem: Fix recursive error handling.
Date: Tue, 19 Jan 2021 16:47:21 +0100

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 <at> gnu.org>
 ;;; Copyright © 2018 Oleg Pykhalov <go.wigust <at> gmail.com>
+;;; Copyright © 2021 Simon Tournier <zimon.toutoune <at> 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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: zimoun <zimon.toutoune <at> gmail.com>
Cc: 45984 <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Tue, 26 Jan 2021 23:24:56 +0100

Hi!

zimoun <zimon.toutoune <at> 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’.

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 45984 <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Wed, 27 Jan 2021 00:16:35 +0100

Hi Ludo,

Thanks for the review.

On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo <at> 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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: zimoun <zimon.toutoune <at> gmail.com>
Cc: 45984 <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Thu, 28 Jan 2021 14:22:12 +0100

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

Hi!

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

> On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo <at> 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:

[Message part 2 (text/x-patch, inline)]

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

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

… 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’.

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 45984 <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Thu, 28 Jan 2021 23:07:18 +0100

Hi Ludo,

Thanks for look at it.

On Thu, 28 Jan 2021 at 14:22, Ludovic Courtès <ludo <at> 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. ;-)

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: zimoun <zimon.toutoune <at> gmail.com>
Cc: 45984-done <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Mon, 07 Mar 2022 22:52:51 +0100

Hi,

zimoun <zimon.toutoune <at> 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’.

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

From: zimoun <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 45984-done <at> debbugs.gnu.org
Subject: Re: bug#45984: [PATCH 0/5] Fix recursive importers
Date: Tue, 8 Mar 2022 09:36:35 +0100

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

Previous Next

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