[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. ;-)