Message ID | 87r1m5p56j.fsf@gnu.org |
---|---|
State | Accepted |
Headers | show |
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. ;-)
(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 '()))