[PATCH 0/5] Fix recursive importers

OpenSubmitted by zimoun.
Details
2 participants
  • Ludovic Courtès
  • zimoun
Owner
unassigned
Severity
normal
Z
Z
zimoun wrote on 19 Jan 16:45 +0100
(address . guix-patches@gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154525.11230-1-zimon.toutoune@gmail.com
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 uglybacktrace, 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:
Toggle snippet (8 lines)$ ./pre-inst-env guix import cran do-not-exist -rerror: 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 -rfollowing redirection to `https://pypi.org/pypi/do-not-exist/json/'...#f
This non-existent message is because the error is poorly handled. With the 4patches, the situation is the same as "guix import gem" for all the importerswith the recursive option. One way for a better error handling is done in thelast commit for 'guix import gem' only; the same trick can be done for all.
Toggle snippet (7 lines)$ guix import gem do-not-exist -r#f
$ ./pre-inst-env guix import gem do-not-exist -rguix import: error: failed to download meta-data for package 'do-not-exist'
In my opinions, UI messages should not appear in guix/import/*.scm but only inguix/scripts/*.scm.

If I understand correctly, then the way the errors are reported could beuniformized 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: 2d9c6542c804eb2ef3d8934e1e3ab8b24e9bbafbprerequisite-patch-id: 6ce76af47a26307f4b99162b5ae2b47f5e5f220fprerequisite-patch-id: 32b0ac51a8fbe72cc9204c5a6d0e2b987af7b7f6prerequisite-patch-id: 3fa663069818b59ab4d324cc69fabcd62c5a9b50-- 2.29.2
Z
Z
zimoun wrote on 19 Jan 16:47 +0100
[PATCH 1/5] import: pypi: Return 'values'.
(address . 45984@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154721.11999-1-zimon.toutoune@gmail.com
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(-)
Toggle diff (39 lines)diff --git a/guix/import/pypi.scm b/guix/import/pypi.scmindex 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
Z
Z
zimoun wrote on 19 Jan 16:47 +0100
[PATCH 2/5] import: hackage: Return 'values'.
(address . 45984@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154721.11999-2-zimon.toutoune@gmail.com
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(-)
Toggle diff (41 lines)diff --git a/guix/import/hackage.scm b/guix/import/hackage.scmindex 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
Z
Z
zimoun wrote on 19 Jan 16:47 +0100
[PATCH 3/5] import: elpa: Return 'values'.
(address . 45984@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154721.11999-3-zimon.toutoune@gmail.com
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(-)
Toggle diff (27 lines)diff --git a/guix/import/elpa.scm b/guix/import/elpa.scmindex 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
Z
Z
zimoun wrote on 19 Jan 16:47 +0100
[PATCH 4/5] import: cran: Return 'values'.
(address . 45984@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154721.11999-4-zimon.toutoune@gmail.com
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(-)
Toggle diff (25 lines)diff --git a/guix/import/cran.scm b/guix/import/cran.scmindex 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
Z
Z
zimoun wrote on 19 Jan 16:47 +0100
[PATCH 5/5] scripts: import: gem: Fix recursive error handling.
(address . 45984@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20210119154721.11999-5-zimon.toutoune@gmail.com
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(-)
Toggle diff (47 lines)diff --git a/guix/scripts/import/gem.scm b/guix/scripts/import/gem.scmindex 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
L
L
Ludovic Courtès wrote on 26 Jan 23:24 +0100
Re: bug#45984: [PATCH 0/5] Fix recursive importers
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 45984@debbugs.gnu.org)
87ft2nwd3b.fsf@gnu.org
Hi!
zimoun <zimon.toutoune@gmail.com> skribis:
Toggle quote (7 lines)> 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!
Toggle quote (21 lines)> 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 ignorenon-existent packages when doing ‘-r’? It would think they’re still aproblem, no?
Toggle quote (3 lines)> 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 isbeneficial (maybe not in this case?).
Toggle quote (4 lines)> 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 nowthat most of them (?) support it, it’d make sense to rethink theinterfaces.
Thanks for looking into it!
Ludo’.
Z
Z
zimoun wrote on 27 Jan 00:16 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 45984@debbugs.gnu.org)
86im7ji90s.fsf@gmail.com
Hi Ludo,
Thanks for the review.
On Tue, 26 Jan 2021 at 23:24, Ludovic Courtès <ludo@gnu.org> wrote:
Toggle quote (10 lines)>> $ 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 thepatch does), displaying which dependent package has failed? Somethingalong 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. :-)

Toggle quote (8 lines)>> 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 “#:keyrepo->guix-package” provided to ’recursive-import’ should always return’values’), then let rethink the interface and how the errors arehandled.

Cheers,simon
L
L
Ludovic Courtès wrote on 28 Jan 14:22 +0100
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 45984@debbugs.gnu.org)
87r1m5p56j.fsf@gnu.org
Hi!
zimoun <zimon.toutoune@gmail.com> skribis:
Toggle quote (23 lines)> 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:
(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 silentlyignored.
But I guess what happens is that #f is interpreted by the caller as afailure, and that’s how we get the “failed to download meta-data”message, right?
If so, forget my comment. Sorry for the confusion!
Ludo’.
Z
Z
zimoun wrote on 28 Jan 23:07 +0100
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 45984@debbugs.gnu.org)
86lfcchg15.fsf@gmail.com
Hi Ludo,
Thanks for look at it.
On Thu, 28 Jan 2021 at 14:22, Ludovic Courtès <ludo@gnu.org> wrote:
Toggle quote (18 lines)> 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 oneerror 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:
Toggle snippet (15 lines)$ guix import elpa do-not-existBacktrace: 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'."
because the function ’elpa->guix-package’ raises an error poorly handled.
With the patch, the situation becomes:
Toggle snippet (4 lines)$ ./pre-inst-env guix import elpa do-not-existguix import: error: failed to download package 'do-not-exist'
And the error message is handled by ’guix/scripts/elpa.scm’ with:
Toggle snippet (5 lines) (unless sexp (leave (G_ "failed to download package '~a'~%") package-name)) sexp)))
Does it make sense?


Now, about the #3. The current situation is:
Toggle snippet (4 lines)$ guix import elpa do-not-exist -rguix import: error: couldn't find meta-data for ELPA package `do-not-exist'.
So here, the error is correctly handled. But it means to add errorhandler 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:
Toggle snippet (5 lines)$ guix import pypi do-not-existfollowing redirection to `https://pypi.org/pypi/do-not-exist/json/'...guix import: error: failed to download meta-data for package 'do-not-exist'
and the error message comes from ’guix/scripts/pypi.scm’. However, therecursive fails with:
Toggle snippet (18 lines)$ guix import pypi do-not-exist -rfollowing 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)
The reason is that the ’lookup-node’ function in ’recursive-import’ isexpecting ’values’ when ’pypi->guix-package’ return just ’#f’.
Toggle snippet (6 lines) (define (lookup-node name version) (let* ((package dependencies (repo->guix-package name #:version version #:repo repo))
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 forthe recursive it becomes:
Toggle snippet (5 lines)$ ./pre-inst-env guix import pypi do-not-exist -rfollowing redirection to `https://pypi.org/pypi/do-not-exist/json/'...#f
where this ’#f’ is from ’guix/scripts/pypi.scm’. The error messagecould be handled here. An example is done for the ’gem’ importer withthe patch:
«scripts: import: gem: Fix recursive error handling.»

Does it make sense?

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

All in all, it is worth to rethink all that. Maybe let drop this patchset 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 discussedonce on December 2018, drinking a beer pre-Reproducible event. Ah goodol’ time with beers in bars, you are missing. ;-)
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send email to 45984@debbugs.gnu.org