| Message ID | f699a25c43805136ada95ef7c7213a78abf5909e.1685030838.git.zimon.toutoune@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#63647,v3,1/3] guix: import: go: Sort hint about available versions. | expand |
Simon Tournier <zimon.toutoune@gmail.com> skribis: > * guix/import/go.scm (git-checkout-hash): Catch Git error. [...] > + (catch 'git-error > + (lambda _ > + (let-values (((checkout commit _) > + (parameterize ((%repository-cache-directory cache)) > + (update-cached-checkout url > + #:ref > + `(tag-or-commit . ,reference))))) > + (file-hash* checkout #:algorithm algorithm #:recursive? #true))) > + (lambda (key error . rest) > + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url) > + (nix-base32-string->bytevector > + "0000000000000000000000000000000000000000000000000000")))) I’d rather let the exception through. How about adding ‘with-git-error-handling’ at the UI level, in (guix scripts import go)? Thanks, Ludo’.
Hi Ludo, On Mon, 05 Jun 2023 at 14:45, Ludovic Courtès <ludo@gnu.org> wrote: > Simon Tournier <zimon.toutoune@gmail.com> skribis: > >> * guix/import/go.scm (git-checkout-hash): Catch Git error. > > [...] > >> + (catch 'git-error >> + (lambda _ >> + (let-values (((checkout commit _) >> + (parameterize ((%repository-cache-directory cache)) >> + (update-cached-checkout url >> + #:ref >> + `(tag-or-commit . ,reference))))) >> + (file-hash* checkout #:algorithm algorithm #:recursive? #true))) >> + (lambda (key error . rest) >> + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url) >> + (nix-base32-string->bytevector >> + "0000000000000000000000000000000000000000000000000000")))) > > I’d rather let the exception through. How about adding > ‘with-git-error-handling’ at the UI level, in (guix scripts import go)? What do you mean by “let the exception through”? It seems better to be non-blocking and thus catch the exception then raise a meaningful warning; it’s required when running with the option recursive. Well, maybe an improvement could be in the addition of some ’report-git-warning’ and/or ’with-git-error-handling*’, in (guix git); hum, I do not know. Last, considering that the module (guix import go) already contains 4 UI messages (G_), trying to move this warning about Git to (guix scripts import go) will add some complexity – re-raise the exception 2 or 3 times, IIUC – and thus it will not change much about the UI, IMHO. I mean, such move should be for all the messages or nothing. Cheers, simon
Hello! Simon Tournier <zimon.toutoune@gmail.com> skribis: > On Mon, 05 Jun 2023 at 14:45, Ludovic Courtès <ludo@gnu.org> wrote: >> Simon Tournier <zimon.toutoune@gmail.com> skribis: >> >>> * guix/import/go.scm (git-checkout-hash): Catch Git error. >> >> [...] >> >>> + (catch 'git-error >>> + (lambda _ >>> + (let-values (((checkout commit _) >>> + (parameterize ((%repository-cache-directory cache)) >>> + (update-cached-checkout url >>> + #:ref >>> + `(tag-or-commit . ,reference))))) >>> + (file-hash* checkout #:algorithm algorithm #:recursive? #true))) >>> + (lambda (key error . rest) >>> + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url) >>> + (nix-base32-string->bytevector >>> + "0000000000000000000000000000000000000000000000000000")))) >> >> I’d rather let the exception through. How about adding >> ‘with-git-error-handling’ at the UI level, in (guix scripts import go)? > > What do you mean by “let the exception through”? It seems better to be > non-blocking and thus catch the exception then raise a meaningful > warning; it’s required when running with the option recursive. I thought it would be more appropriate to error out as soon as we have a Git problem, rather than print a warning and emit an incorrect hash. I understand that, when using ‘--recursive’, this means aborting the whole process without producing anything. But maybe that’s better that producing an incorrect (origin …) form? Now, I don’t use ‘guix import go -r’ so it’s possible that I don’t understand the scenario being considered here! Thanks, Ludo’.
diff --git a/guix/import/go.scm b/guix/import/go.scm index 1943869162..c6258296f6 100644 --- a/guix/import/go.scm +++ b/guix/import/go.scm @@ -6,7 +6,7 @@ ;;; Copyright © 2021-2022 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> +;;; Copyright © 2021, 2023 Simon Tournier <zimon.toutoune@gmail.com> ;;; ;;; This file is part of GNU Guix. ;;; @@ -36,9 +36,11 @@ (define-module (guix import go) #:use-module (guix memoization) #:use-module (guix utils) #:autoload (htmlprag) (html->sxml) ;from Guile-Lib - #:autoload (guix base32) (bytevector->nix-base32-string) + #:autoload (guix base32) (bytevector->nix-base32-string nix-base32-string->bytevector) #:autoload (guix build utils) (mkdir-p) + #:autoload (guix ui) (warning) #:autoload (gcrypt hash) (hash-algorithm sha256) + #:autoload (git structs) (git-error-message) #:use-module (ice-9 format) #:use-module (ice-9 match) #:use-module (ice-9 peg) @@ -507,12 +509,18 @@ (define* (git-checkout-hash url reference algorithm) ;; subsequent "guix import" invocations. (mkdir-p cache) (chmod cache #o700) - (let-values (((checkout commit _) - (parameterize ((%repository-cache-directory cache)) - (update-cached-checkout url - #:ref - `(tag-or-commit . ,reference))))) - (file-hash* checkout #:algorithm algorithm #:recursive? #true))) + (catch 'git-error + (lambda _ + (let-values (((checkout commit _) + (parameterize ((%repository-cache-directory cache)) + (update-cached-checkout url + #:ref + `(tag-or-commit . ,reference))))) + (file-hash* checkout #:algorithm algorithm #:recursive? #true))) + (lambda (key error . rest) + (warning (G_ "Git error: ~a in ~a~%") (git-error-message error) url) + (nix-base32-string->bytevector + "0000000000000000000000000000000000000000000000000000")))) (define (vcs->origin vcs-type vcs-repo-url version) "Generate the `origin' block of a package depending on what type of source