[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 2021 16:45
(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 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:

Toggle snippet (8 lines)
$ ./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.

Toggle snippet (7 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'

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
Z
Z
zimoun wrote on 19 Jan 2021 16:47
[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

* 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.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
Z
Z
zimoun wrote on 19 Jan 2021 16:47
[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

* 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.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
Z
Z
zimoun wrote on 19 Jan 2021 16:47
[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

* 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.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
Z
Z
zimoun wrote on 19 Jan 2021 16:47
[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

* 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.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
Z
Z
zimoun wrote on 19 Jan 2021 16:47
[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

* 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.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
L
L
Ludovic Courtès wrote on 26 Jan 2021 23:24
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 ignore
non-existent packages when doing ‘-r’? It would think they’re still a
problem, 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 is
beneficial (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 now
that most of them (?) support it, it’d make sense to rethink the
interfaces.

Thanks for looking into it!

Ludo’.
Z
Z
zimoun wrote on 27 Jan 2021 00:16
(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 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. :-)


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 “#: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
L
L
Ludovic Courtès wrote on 28 Jan 2021 14:22
(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 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’.
Z
Z
zimoun wrote on 28 Jan 2021 23:07
(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 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:

Toggle snippet (15 lines)
$ 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'."

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-exist
guix 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 -r
guix 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 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:

Toggle snippet (5 lines)
$ 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'

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

Toggle snippet (18 lines)
$ 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)

The reason is that the ’lookup-node’ function in ’recursive-import’ is
expecting ’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 for
the recursive it becomes:

Toggle snippet (5 lines)
$ ./pre-inst-env guix import pypi do-not-exist -r
following redirection to `https://pypi.org/pypi/do-not-exist/json/'...
#f

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. ;-)
?
Your comment

Commenting via the web interface is currently disabled.

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