Message ID | 20210625163749.65196-3-zimon.toutoune@gmail.com |
---|---|
State | Accepted |
Headers | show |
Series | [bug#49196,v3,1/3] import: go: Return false for package not found. | 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 |
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, Apologies for the delay. Thanks for the work. zimoun <zimon.toutoune@gmail.com> writes: > - #:export (go-module->guix-package > + #:export (go-module->guix-package* Would it be better to export both, in case a callee wants access to the actual errors? > +(define go-module->guix-package* > + (memoize > + (lambda args I would remove the memoize from this method (to put it back in later on), because multiple invocations with errors would only yield one error reported. I do not think this makes sense if another tool is using this. > + (else > + (warning (G_ "Something went wrong with ~s.~%") args) A catch-all is fine, but we should at least report the actual error, even if it's not pretty: (warning (G_ "Failed to import package ~s.~% reason: ~s") package-name (exception-args c)) However, if we want to move in the direction of proper error handling like guix/packages.scm and guix/ui.scm, we can do something like... --8<---------------cut here---------------start------------->8--- (define (report-import-error package-name error) (cond ((http-get-error? error) [...] (else [...])))) (define* (go-module->guix-package* module-path #:key (on-error report-import-error) #:allow-other-keys #:rest args) (with-exception-handler (lambda (error) (on-error module-path error) (values #f '())) (lambda () (apply go-module->guix-package module-path args)) #:unwind? #t)) (define* (go-module-recursive-import package-name #:key (goproxy "https://proxy.golang.org") version pin-versions?) (recursive-import package-name #:repo->guix-package (memoize (lambda* (name #:key version repo) (go-module->guix-package* name #:goproxy goproxy #:version version #:pin-versions? pin-versions?))) #:guix-name go-module->guix-package-name #:version version)) --8<---------------cut here---------------end--------------->8--- Looks like I got a little carried away :) But breaking out the error reporting gives us the future option of "plugging in" other error reporting strategies, such as collating them and returning them alongside a programmatic recursive import, or being able to provide a list of packages which failed to import at the end. This will be much more useful once we have a proper set of import error conditions. Too much, perhaps? Sarah
Hi, On Sun, 27 Jun 2021 at 06:46, Sarah Morgensen <iskarian@mgsn.dev> wrote: > A catch-all is fine, but we should at least report the actual error, > even if it's not pretty: > > (warning (G_ "Failed to import package ~s.~% reason: ~s") > package-name (exception-args c)) Well, why not, even if I am not convinced the reason is meaningful because it is mostly an incorrect parsing which returns: reason: ("match" "no matching pattern" #f). and I think it is better to display the complete 'args' instead of extract the URL (package-name) from 'args'. > However, if we want to move in the direction of proper error handling > like guix/packages.scm and guix/ui.scm, we can do something like... Thanks for the idea. As explained patch#45984 [1], all the UI messages must be in guix/scripts/import and not in guix/import and therefore, indeed, error reporting needs to be unified between all the importers and raised accordingly; that's what we are working on with jeko (Jérémy Korwin-Zmijowski) as pair-programming exercise. :-) 1: <http://issues.guix.gnu.org/issue/45984> > --8<---------------cut here---------------start------------->8--- > (define (report-import-error package-name error) > (cond > ((http-get-error? error) > [...] > (else > [...])))) > > (define* (go-module->guix-package* module-path > #:key (on-error report-import-error) > #:allow-other-keys #:rest args) > (with-exception-handler > (lambda (error) > (on-error module-path error) > (values #f '())) > (lambda () (apply go-module->guix-package module-path args)) > #:unwind? #t)) > > (define* (go-module-recursive-import package-name > #:key (goproxy "https://proxy.golang.org") > version > pin-versions?) > (recursive-import > package-name > #:repo->guix-package > (memoize > (lambda* (name #:key version repo) > (go-module->guix-package* name #:goproxy goproxy > #:version version > #:pin-versions? pin-versions?))) > #:guix-name go-module->guix-package-name > #:version version)) > --8<---------------cut here---------------end--------------->8--- > > Looks like I got a little carried away :) But breaking out the error > reporting gives us the future option of "plugging in" other error > reporting strategies, such as collating them and returning them > alongside a programmatic recursive import, or being able to provide a > list of packages which failed to import at the end. This will be much > more useful once we have a proper set of import error conditions. Back to the initial patch, I think it is better to simply fix with the minor improvements of v3 your proposed and let this last proposal for #45984; feel free to comment there. ;-) Cheers, simon
zimoun <zimon.toutoune@gmail.com> writes: Hello, Thanks for the v4. > Hi, > > On Sun, 27 Jun 2021 at 06:46, Sarah Morgensen <iskarian@mgsn.dev> wrote: > >> A catch-all is fine, but we should at least report the actual error, >> even if it's not pretty: >> >> (warning (G_ "Failed to import package ~s.~% reason: ~s") >> package-name (exception-args c)) > > Well, why not, even if I am not convinced the reason is meaningful > because it is mostly an incorrect parsing which returns: > > reason: ("match" "no matching pattern" #f). > Yes, it is a less than ideal compromise... I could not quickly figure out how to properly format it without a lot of complexity (like guix/ui.scm does in 'call-with-error-handling'). I found it hard to read the full exception object, but I would not object strongly to printing the full exception object either. As you say, your patch will fix it anyway ;) > and I think it is better to display the complete 'args' instead of > extract the URL (package-name) from 'args'. You're not wrong; I was just trying to keep it somewhat consistent with the other error message. >> However, if we want to move in the direction of proper error handling >> like guix/packages.scm and guix/ui.scm, we can do something like... > > Thanks for the idea. As explained patch#45984 [1], all the UI > messages must be in guix/scripts/import and not in guix/import and Yes, this was my secret trick: having separated out the error reporting, it could be easily be moved to scripts/import later. > therefore, indeed, error reporting needs to be unified between all the > importers and raised accordingly; that's what we are working on with > jeko (Jérémy Korwin-Zmijowski) as pair-programming exercise. :-) I look forward to the results! > Back to the initial patch, I think it is better to simply fix with the > minor improvements of v3 your proposed and let this last proposal for > #45984; feel free to comment there. ;-) I agree. Your v4 looks good to me, except... > #:repo->guix-package > (lambda* (name #:key version repo) I apologize for not being clear earlier; by "put [memoize] back in later on" I meant "later on in the call chain," e.g. #:repo->guix-package + (memoize (lambda* (name #:key version repo) That's my only nit this time! ;) Thanks for bearing with me. Regards, Sarah
diff --git a/guix/import/go.scm b/guix/import/go.scm index c859098492..c559f02e5b 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -6,6 +6,7 @@ ;;; Copyright © 2021 Ludovic Courtès <ludo@gnu.org> ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz> ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev> +;;; Copyright © 2021 Simon Tournier <zimon.toutoune@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -61,7 +62,7 @@ #:use-module (web response) #:use-module (web uri) - #:export (go-module->guix-package + #:export (go-module->guix-package* go-module-recursive-import)) ;;; Commentary: @@ -630,17 +631,9 @@ hint: use one of the following available versions ~a\n" dependencies+versions dependencies)))) -(define go-module->guix-package* (memoize go-module->guix-package)) - -(define* (go-module-recursive-import package-name - #:key (goproxy "https://proxy.golang.org") - version - pin-versions?) - - (recursive-import - package-name - #:repo->guix-package - (lambda* (name #:key version repo) +(define go-module->guix-package* + (memoize + (lambda args ;; Disable output buffering so that the following warning gets printed ;; consistently. (setvbuf (current-error-port) 'none) @@ -648,15 +641,29 @@ hint: use one of the following available versions ~a\n" (warning (G_ "Failed to import package ~s. reason: ~s could not be fetched: HTTP error ~a (~s). This package and its dependencies won't be imported.~%") - name + (match args ((name _ ...) name)) (uri->string (http-get-error-uri c)) (http-get-error-code c) (http-get-error-reason c)) + (values #f '())) + (else + (warning (G_ "Something went wrong with ~s.~%") args) (values #f '()))) - (receive (package-sexp dependencies) - (go-module->guix-package* name #:goproxy goproxy - #:version version - #:pin-versions? pin-versions?) - (values package-sexp dependencies)))) + (apply go-module->guix-package args))))) + +(define* (go-module-recursive-import package-name + #:key (goproxy "https://proxy.golang.org") + version + pin-versions?) + + (recursive-import + package-name + #:repo->guix-package + (lambda* (name #:key version repo) + (receive (package-sexp dependencies) + (go-module->guix-package* name #:goproxy goproxy + #:version version + #:pin-versions? pin-versions?) + (values package-sexp dependencies))) #:guix-name go-module->guix-package-name #:version version)) diff --git a/guix/scripts/import/go.scm b/guix/scripts/import/go.scm index 74e8e60cce..071ecdb2ef 100644 --- a/guix/scripts/import/go.scm +++ b/guix/scripts/import/go.scm @@ -115,10 +115,10 @@ that are not yet in Guix")) (map package->definition* (apply go-module-recursive-import arguments)) ;; Single import. - (let ((sexp (apply go-module->guix-package arguments))) + (let ((sexp (apply go-module->guix-package* arguments))) (unless sexp - (leave (G_ "failed to download meta-data for module '~a'~%") - module-name)) + (leave (G_ "failed to download meta-data for module '~a'.~%") + name)) (package->definition* sexp)))))) (() (leave (G_ "too few arguments~%"))) diff --git a/tests/go.scm b/tests/go.scm index b088ab50d2..929a8b39d1 100644 --- a/tests/go.scm +++ b/tests/go.scm @@ -286,6 +286,6 @@ package.") (nix-base32-string->bytevector "0sjjj9z1dhilhpc8pq4154czrb79z9cm044jvn75kxcjv6v5l2m5") #f))) - (go-module->guix-package "github.com/go-check/check"))))))) + (go-module->guix-package* "github.com/go-check/check"))))))) (test-end "go")