| Message ID | 07d7149fc0f89f7f2d11fba47e1b0b2db5ceb809.1624479231.git.iskarian@mgsn.dev |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#49196] import: utils: 'recursive-import' skips unfound packages | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi, Thanks for the patch. On mer., 23 juin 2021 at 13:49, Sarah Morgensen <iskarian@mgsn.dev> wrote: > It seems like several importers (gem, egg, go) expect to be able to return #f or > '() in #:repo->guix-package when a package is not found (while printing a > warning to the user) but recursive-import doesn't look like it handles it. I > suppose this worked before but was silently broken at some point. This patch > (re-)enables this behavior. I added a test for this as well. Indeed, there is an inconsistency betweem all the recursive importers. An attempt to fix this is done by [1]. 1: <http://issues.guix.gnu.org/issue/45984> > diff --git a/guix/import/go.scm b/guix/import/go.scm > index d110954664..c859098492 100644 > --- a/guix/import/go.scm > +++ b/guix/import/go.scm > @@ -5,6 +5,7 @@ > ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com> > ;;; Copyright © 2021 Ludovic Courtès <ludo@gnu.org> > ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz> > +;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -651,7 +652,7 @@ This package and its dependencies won't be imported.~%") > (uri->string (http-get-error-uri c)) > (http-get-error-code c) > (http-get-error-reason c)) > - (values '() '()))) > + (values #f '()))) Yes, there is an inconsistency... > (receive (package-sexp dependencies) > (go-module->guix-package* name #:goproxy goproxy > #:version version > diff --git a/guix/import/utils.scm b/guix/import/utils.scm > index d817318a91..49f38cfa2a 100644 > --- a/guix/import/utils.scm > +++ b/guix/import/utils.scm > @@ -8,6 +8,7 @@ > ;;; Copyright © 2020 Helio Machado <0x2b3bfa0+guix@googlemail.com> > ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net> > ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com> > +;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> > ;;; > ;;; This file is part of GNU Guix. > ;;; > @@ -469,16 +470,17 @@ to obtain the Guix package name corresponding to the upstream name." > (normalized-deps (map (match-lambda > ((name version) (list name version)) > (name (list name #f))) dependencies))) > - (make-node name version package normalized-deps))) > + (and package > + (make-node name version package normalized-deps)))) > > (map node-package > (topological-sort (list (lookup-node package-name version)) > (lambda (node) > - (map (lambda (name-version) > - (apply lookup-node name-version)) > - (remove (lambda (name-version) > - (apply exists? name-version)) > - (node-dependencies node)))) > + (filter-map (lambda (name-version) > + (apply lookup-node name-version)) > + (remove (lambda (name-version) > + (apply exists? name-version)) > + (node-dependencies node)))) ...however, I am not convinced this fixes the issue. Compare: --8<---------------cut here---------------start------------->8--- $ guix import go do-not-exist -r guix import: warning: Failed to import package "do-not-exist". reason: "https://proxy.golang.org/do-not-exist/@v/list" could not be fetched: HTTP error 410 ("Gone"). This package and its dependencies won't be imported. Backtrace: 4 (primitive-load "/home/sitour/.config/guix/current/bin/guix") In guix/ui.scm: 2147:12 3 (run-guix-command _ . _) In guix/scripts/import.scm: 120:11 2 (guix-import . _) In srfi/srfi-1.scm: 586:17 1 (map1 (())) In guix/import/utils.scm: 280:2 0 (package->definition _ _) guix/import/utils.scm:280:2: In procedure package->definition: Throw to key `match-error' with args `("match" "no matching pattern" ())'. --8<---------------cut here---------------end--------------->8--- with: --8<---------------cut here---------------start------------->8--- $ ./pre-inst-env guix import go do-not-exist -r guix import: warning: Failed to import package "do-not-exist". reason: "https://proxy.golang.org/do-not-exist/@v/list" could not be fetched: HTTP error 410 ("Gone"). This package and its dependencies won't be imported. Backtrace: In ice-9/boot-9.scm: 1752:10 8 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _) In unknown file: 7 (apply-smob/0 #<thunk 7f3ca6977f60>) In ice-9/boot-9.scm: 724:2 6 (call-with-prompt _ _ #<procedure default-prompt-handler (k proc)>) In ice-9/eval.scm: 619:8 5 (_ #(#(#<directory (guile-user) 7f3ca6971c80>))) In guix/ui.scm: 2147:12 4 (run-guix-command _ . _) In guix/scripts/import.scm: 120:11 3 (guix-import . _) In guix/scripts/import/go.scm: 116:20 2 (guix-import-go . _) In guix/import/utils.scm: 440:34 1 (recursive-import _ #:repo->guix-package _ #:guix-name _ #:version _ #:repo _) 486:28 0 (_ #f) guix/import/utils.scm:486:28: In procedure struct-vtable: Wrong type argument in position 1 (expecting struct): #f --8<---------------cut here---------------end--------------->8--- Then, the patch introduces issues against other importers, for instance: --8<---------------cut here---------------start------------->8--- r@bioinfomeary01-Precision-7820-Tower$ guix import gem do-not-exist -r #f $ ./pre-inst-env guix import gem do-not-exist -r Backtrace: In ice-9/boot-9.scm: 1752:10 8 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _) In unknown file: 7 (apply-smob/0 #<thunk 7fd3a8934f60>) In ice-9/boot-9.scm: 724:2 6 (call-with-prompt _ _ #<procedure default-prompt-handler (k proc)>) In ice-9/eval.scm: 619:8 5 (_ #(#(#<directory (guile-user) 7fd3a892ec80>))) In guix/ui.scm: 2147:12 4 (run-guix-command _ . _) In guix/scripts/import.scm: 120:11 3 (guix-import . _) In guix/scripts/import/gem.scm: 97:16 2 (guix-import-gem . _) In guix/import/utils.scm: 440:34 1 (recursive-import _ #:repo->guix-package _ #:guix-name _ #:version _ #:repo _) 486:28 0 (_ #f) guix/import/utils.scm:486:28: In procedure struct-vtable: Wrong type argument in position 1 (expecting struct): #f --8<---------------cut here---------------end--------------->8--- Well, it is not worse for most of the importers. And perhaps this patch is worth. As explained in [1], all the recursive importers should be unified and the errors correctly handled, IMHO. With jeko, we are pair-programming to work on it... but we are really slow. ;-) > diff --git a/tests/import-utils.scm b/tests/import-utils.scm > index 874816442e..1b728104e0 100644 > --- a/tests/import-utils.scm > +++ b/tests/import-utils.scm Thanks for adding a test. :-) Cheers, simon
Hello, Thanks for the review! zimoun <zimon.toutoune@gmail.com> writes: > Indeed, there is an inconsistency betweem all the recursive importers. > An attempt to fix this is done by [1]. > > 1: <http://issues.guix.gnu.org/issue/45984> Thanks, that was a good read. With context, I see where you're coming from. I agree that the direction to take with these importers is to unify and standardize. The goal of this patch is just to allow recursive import to provide a usable result despite some failures, when the importer supports it. I'd rather hunt down one package than 20+ :) This may make reporting errors more difficult, but I think the use-case is worth it. > ...however, I am not convinced this fixes the issue. Compare: > > $ guix import go do-not-exist -r > > with: > > $ ./pre-inst-env guix import go do-not-exist -r Good catch. I did not think to handle the toplevel package not being found! This actually leads to making this a much simpler patch... --8<---------------cut here---------------start------------->8--- - (map node-package + (filter-map node-package (topological-sort (list (lookup-node package-name version)) --8<---------------cut here---------------end--------------->8--- ...which also works for other importers which return (values #f ...): --8<---------------cut here---------------start------------->8--- ~/guix$ for importer in stackage elpa gem cran go ; do printf "\n### $importer\n" ; ./pre-inst-env guix import $importer really-does-not-exist -r ;done ### stackage guix import: error: really-does-not-exist: Stackage package not found ### elpa guix import: error: couldn't find meta-data for ELPA package `really-does-not-exist'. ### gem ### cran error: failed to retrieve package information from "https://cran.r-project.org/web/packages/really-does-not-exist/DESCRIPTION": 404 ("Not Found") guix import: error: couldn't find meta-data for R package ### go guix import: warning: Failed to import package "really-does-not-exist". reason: "https://proxy.golang.org/really-does-not-exist/@v/list" could not be fetched: HTTP error 410 ("Gone"). This package and its dependencies won't be imported. --8<---------------cut here---------------end--------------->8--- > Well, it is not worse for most of the importers. And perhaps this patch > is worth. As explained in [1], all the recursive importers should be > unified and the errors correctly handled, IMHO. With jeko, we are > pair-programming to work on it... but we are really slow. ;-) Yes, this is very much just a stopgap. In your words (from #45984): > Well, this patch set are trivial changes that quickly fix the current > broken situation without a deep revamp. I will follow up with an updated patch. Regards, Sarah
Hi, On Fri, 25 Jun 2021 at 06:22, Sarah Morgensen <iskarian@mgsn.dev> wrote: > The goal of this patch is just to allow recursive import to provide a > usable result despite some failures, when the importer supports it. I'd > rather hunt down one package than 20+ :) This may make reporting errors > more difficult, but I think the use-case is worth it. I agree. > Good catch. I did not think to handle the toplevel package not being > found! This actually leads to making this a much simpler patch... > > --8<---------------cut here---------------start------------->8--- > - (map node-package > + (filter-map node-package > (topological-sort (list (lookup-node package-name version)) > --8<---------------cut here---------------end--------------->8--- Cool! > Yes, this is very much just a stopgap. In your words (from #45984): > > > Well, this patch set are trivial changes that quickly fix the current > > broken situation without a deep revamp. I agree. Heh! I am consistent with my words. ;-) > I will follow up with an updated patch. Cool, thanks! Cheers, simon
Hi Sarah,
I sent a v3 where your v2 is split into 2 parts. Then I move the
guard. Now, your initial example returns:
--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import go git.sr.ht/~sircmpwn/aerc -r
following redirection to `https://pkg.go.dev/github.com/zenhack/go.notmuch'...
following redirection to `https://pkg.go.dev/github.com/zenhack/go.notmuch'...
following redirection to `https://github.com/ProtonMail/go-crypto?go-get=1'...
following redirection to `https://github.com/jtolio/gls?go-get=1'...
guix import: warning: Something went wrong with ("github.com/neelance/sourcemap" #:goproxy "https://proxy.golang.org" #:version #f #:pin-versions? #f).
guix import: warning: Something went wrong with ("github.com/neelance/astrewrite" #:goproxy "https://proxy.golang.org" #:version #f #:pin-versions? #f).
(define-public go-git-sr-ht-~sircmpwn-getopt
(package
(name "go-git-sr-ht-~sircmpwn-getopt")
(version "0.0.0-20201218204720-9961a9c6298f")
(source
[...]
("go-github-com-creack-pty"
,go-github-com-creack-pty)
("go-git-sr-ht-~sircmpwn-getopt"
,go-git-sr-ht-~sircmpwn-getopt)))
(home-page "https://git.sr.ht/~sircmpwn/aerc")
(synopsis "aerc")
(description
"aerc is an email client for your terminal.")
(license license:expat)))
--8<---------------cut here---------------end--------------->8---
WDYT?
Well, I do not know if it is good idea to catch the 'else' case because
it hiddes what is wrong by the importer. On the other hand, it seems a
bit friendler for the end-user. :-)
Cheers,
simon
diff --git a/guix/import/go.scm b/guix/import/go.scm index d110954664..c859098492 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -5,6 +5,7 @@ ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com> ;;; Copyright © 2021 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz> +;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> ;;; ;;; This file is part of GNU Guix. ;;; @@ -651,7 +652,7 @@ This package and its dependencies won't be imported.~%") (uri->string (http-get-error-uri c)) (http-get-error-code c) (http-get-error-reason c)) - (values '() '()))) + (values #f '()))) (receive (package-sexp dependencies) (go-module->guix-package* name #:goproxy goproxy #:version version diff --git a/guix/import/utils.scm b/guix/import/utils.scm index d817318a91..49f38cfa2a 100644 --- a/guix/import/utils.scm +++ b/guix/import/utils.scm @@ -8,6 +8,7 @@ ;;; Copyright © 2020 Helio Machado <0x2b3bfa0+guix@googlemail.com> ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net> ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com> +;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> ;;; ;;; This file is part of GNU Guix. ;;; @@ -469,16 +470,17 @@ to obtain the Guix package name corresponding to the upstream name." (normalized-deps (map (match-lambda ((name version) (list name version)) (name (list name #f))) dependencies))) - (make-node name version package normalized-deps))) + (and package + (make-node name version package normalized-deps)))) (map node-package (topological-sort (list (lookup-node package-name version)) (lambda (node) - (map (lambda (name-version) - (apply lookup-node name-version)) - (remove (lambda (name-version) - (apply exists? name-version)) - (node-dependencies node)))) + (filter-map (lambda (name-version) + (apply lookup-node name-version)) + (remove (lambda (name-version) + (apply exists? name-version)) + (node-dependencies node)))) (lambda (node) (string-append (node-name node) diff --git a/tests/import-utils.scm b/tests/import-utils.scm index 874816442e..1b728104e0 100644 --- a/tests/import-utils.scm +++ b/tests/import-utils.scm @@ -2,6 +2,7 @@ ;;; Copyright © 2015, 2017 Ricardo Wurmus <rekado@elephly.net> ;;; Copyright © 2016 Ben Woodcroft <donttrustben@gmail.com> ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net> +;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> ;;; ;;; This file is part of GNU Guix. ;;; @@ -64,6 +65,23 @@ '()))) #:guix-name identity)) +(test-equal "recursive-import: skip false packages" + '((package + (name "foo") + (inputs `(("bar" ,bar))))) + (recursive-import "foo" + #:repo 'repo + #:repo->guix-package + (match-lambda* + (("foo" #:version #f #:repo 'repo) + (values '(package + (name "foo") + (inputs `(("bar" ,bar)))) + '("bar"))) + (("bar" #:version #f #:repo 'repo) + (values #f '()))) + #:guix-name identity)) + (test-assert "alist->package with simple source" (let* ((meta '(("name" . "hello") ("version" . "2.10")
* guix/import/utils.scm (recursive-import): Skip packages when the package returned by repo->guix-package is false. * guix/import/go.scm (go-module-recursive-import): Explicitly return false in 'repo->guix-package' when packages are not found. * tests/import-utils.scm ("recursive-import: skip false packages"): New test. --- Hello Guix, While trying to recursively import a go package I encountered the following error when it was unable to find one of the dependencies: --8<---------------cut here---------------start------------->8--- $ ./pre-inst-env guix import go -r git.sr.ht/~sircmpwn/aerc guix import: warning: Failed to import package "github.com/brunnre8/go.notmuch". reason: "https://pkg.go.dev/github.com/brunnre8/go.notmuch" could not be fetched: HTTP error 404 ("Not Found"). This package and its dependencies won't be imported. following redirection to `https://github.com/ProtonMail/go-crypto?go-get=1'... following redirection to `https://github.com/jtolio/gls?go-get=1'... Backtrace: In srfi/srfi-1.scm: 586:29 19 (map1 ((package (name "go-github-com-rivo-uniseg") …) …)) 586:29 18 (map1 ((package (name "go-github-com-mattn-go-r…") …) …)) 586:29 17 (map1 ((package (name "go-github-com-miolini-da…") …) …)) 586:29 16 (map1 ((package (name "go-github-com-riywo-logi…") …) …)) 586:29 15 (map1 ((package (name "go-github-com-neelance-a…") …) …)) 586:29 14 (map1 ((package (name "go-github-com-neelance-s…") …) …)) 586:29 13 (map1 ((package (name "go-github-com-shurcool-go") …) …)) 586:29 12 (map1 ((package (name "go-github-com-shurcool-h…") …) …)) 586:29 11 (map1 ((package (name "go-github-com-shurcool-v…") …) …)) 586:29 10 (map1 ((package (name "go-github-com-gopherjs-g…") …) …)) 586:29 9 (map1 ((package (name "go-github-com-jtolds-gls") # …) …)) 586:29 8 (map1 ((package (name "go-github-com-smartystre…") …) …)) 586:29 7 (map1 ((package (name "go-github-com-smartystre…") …) …)) 586:29 6 (map1 ((package (name "go-github-com-google-go-…") …) …)) 586:29 5 (map1 ((package (name "go-google-golang-org-pro…") …) …)) 586:29 4 (map1 ((package (name "go-github-com-golang-pro…") …) …)) 586:29 3 (map1 ((package (name "go-google-golang-org-app…") …) …)) 586:29 2 (map1 ((package (name "go-github-com-protonmail…") …) …)) 586:17 1 (map1 (() (package (name "go-git-sr-ht-~sircmpwn…") …))) In guix/import/utils.scm: 280:2 0 (package->definition _ _) guix/import/utils.scm:280:2: In procedure package->definition: Throw to key `match-error' with args `("match" "no matching pattern" ())'. --8<---------------cut here---------------end--------------->8--- It seems like several importers (gem, egg, go) expect to be able to return #f or '() in #:repo->guix-package when a package is not found (while printing a warning to the user) but recursive-import doesn't look like it handles it. I suppose this worked before but was silently broken at some point. This patch (re-)enables this behavior. I added a test for this as well. Hope this helps, Sarah guix/import/go.scm | 3 ++- guix/import/utils.scm | 14 ++++++++------ tests/import-utils.scm | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 7 deletions(-) base-commit: acfa55a581ca4e688ee4b8f860fe365b1f153ef9