GNU bug report logs - #74481
[PATCH 0/2] Handle corner cases of 'guix import go'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 74481 in the body.
You can then email your comments to 74481 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH 0/2] Handle corner cases of 'guix import go'
Date: Fri, 22 Nov 2024 20:05:57 +0100

Hi,

Instead of that:

--8<---------------cut here---------------start------------->8---
$ guix import go software.sslmate.com/src/go-pkcs12
guix import: Importing package "software.sslmate.com/src/go-pkcs12"...
guix import: warning: Unable to determine repository root of 'software.sslmate.com/src/go-pkcs12'. Guessing 'software.sslmate.com/src/go-pkcs12'.
SWH: found revision fa70679f0f1622a2705336a97225ee8d6c555f96 with directory at 'https://archive.softwareheritage.org/api/1/directory/43d3e9f7167cdcd9f179e8d1de33d0d22e8fbd46/'
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/HEAD
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/branches/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/config
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/description
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/hooks/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/exclude
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/refs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/packs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.idx
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.pack
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/master
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/tags/
guix import: warning: Backtrace:
In ice-9/boot-9.scm:
  1752:10 12 (with-exception-handler _ _ #:unwind? _ # _)
In unknown file:
          11 (apply-smob/0 #<thunk 7fa8659d9300>)
In ice-9/boot-9.scm:
    724:2 10 (call-with-prompt _ _ #<procedure default-prompt-handle?>)
In ice-9/eval.scm:
    619:8  9 (_ #(#(#<directory (guile-user) 7fa8659dcc80>)))
In guix/ui.scm:
   2330:7  8 (run-guix . _)
  2293:10  7 (run-guix-command _ . _)
In guix/scripts/import.scm:
     82:6  6 (guix-import . _)
In ice-9/boot-9.scm:
  1752:10  5 (with-exception-handler _ _ #:unwind? _ # _)
In guix/scripts/import/go.scm:
   116:29  4 (_)
In ice-9/eval.scm:
    619:8  3 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   174:20  2 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   177:32  1 (lp (#<procedure 7fa8598a1380 at ice-9/eval.scm:191:12?>))
In ice-9/boot-9.scm:
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure git-error-message: Wrong type argument: #<&compound-exception components: (#<&error> #<&irritants irritants: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)> #<&exception-with-kind-and-args kind: git-error args: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)>)>
--8<---------------cut here---------------end--------------->8---

which is annoying because it is blocking when using the importer recursively,
the patch set produces that:

--8<---------------cut here---------------start------------->8---
$ ./pre-inst-env guix import go software.sslmate.com/src/go-pkcs12
guix import: Importing package "software.sslmate.com/src/go-pkcs12"...
guix import: warning: Unable to determine repository root of 'software.sslmate.com/src/go-pkcs12'. Guessing 'software.sslmate.com/src/go-pkcs12'.
SWH: found revision fa70679f0f1622a2705336a97225ee8d6c555f96 with directory at 'https://archive.softwareheritage.org/api/1/directory/43d3e9f7167cdcd9f179e8d1de33d0d22e8fbd46/'
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/HEAD
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/branches/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/config
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/description
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/hooks/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/exclude
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/info/refs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/info/packs
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.idx
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/objects/pack/pack-19cac523aaea8e45710dd5cc36bbee1ecc68369c.pack
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/heads/master
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/refs/tags/
guix import: warning: Git cannot find the reference v0.5.0
(define-public go-software-sslmate-com-src-go-pkcs12
  (package
    (name "go-software-sslmate-com-src-go-pkcs12")
    (version "0.5.0")
    (source
     (origin
       (method git-fetch)
       (uri (git-reference
             (url "https://software.sslmate.com/src/go-pkcs12.git")
             (commit (string-append "v" version))))
       (file-name (git-file-name name version))
       (sha256
        (base32 "0000000000000000000000000000000000000000000000000000"))))
    (build-system go-build-system)
    (arguments
     (list
      #:import-path "software.sslmate.com/src/go-pkcs12"))
    (home-page "https://software.sslmate.com/src/go-pkcs12")
    (synopsis "package pkcs12")
    (description
     "Package pkcs12 implements some of PKCS#12 (also known as P12 or PFX).  It is
intended for decoding DER-encoded P12/PFX files for use with the crypto/tls
package, and for encoding P12/PFX files for use by legacy applications which do
not support newer formats.  Since PKCS#12 uses weak encryption primitives, it
SHOULD NOT be used for new applications.")
    (license license:bsd-3)))
--8<---------------cut here---------------end--------------->8---


Please double check because it tweaks 'update-cached-checkout' which is used
by "guix pull". :-)

Cheers,
simon


Simon Tournier (2):
  git: Catch Git errors when updating cached checkout.
  import: go: Warn instead of error out when reference is missing.

 guix/git.scm       | 128 ++++++++++++++++++++++++++-------------------
 guix/import/go.scm |  10 ++--
 2 files changed, 82 insertions(+), 56 deletions(-)


base-commit: 043f02462766a913080723ad286028a288b79373
-- 
2.46.0

Message #8 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 74481 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH 2/2] import: go: Warn instead of error out when reference is
 missing.
Date: Fri, 22 Nov 2024 20:09:27 +0100

* guix/import/go.scm (git-checkout-hash): Return some hash although the
checkout does not contain the expected reference.

Change-Id: I40287fae29042a0ba106939a413239df6efde97d
---
 guix/import/go.scm | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 32cba25b33..25ae989378 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -6,7 +6,7 @@
 ;;; Copyright © 2021-2022 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2021 Xinglu Chen <public <at> yoctocell.xyz>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian <at> mgsn.dev>
-;;; Copyright © 2021 Simon Tournier <zimon.toutoune <at> gmail.com>
+;;; Copyright © 2021, 2024 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;; Copyright © 2023 Efraim Flashner <efraim <at> flashner.co.il>
 ;;; Copyright © 2024 Christina O'Donnell <cdo <at> mutix.org>
 ;;;
@@ -38,7 +38,8 @@ (define-module (guix import go)
   #:use-module (guix http-client)
   #:use-module (guix memoization)
   #: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)
@@ -566,7 +567,10 @@ (define* (git-checkout-hash url reference algorithm)
                   (update-cached-checkout url
                                           #:ref
                                           `(tag-or-commit . ,reference)))))
-    (file-hash* checkout #:algorithm algorithm #:recursive? #true)))
+    (if (and checkout commit)
+        (file-hash* checkout #:algorithm algorithm #:recursive? #true)
+        (nix-base32-string->bytevector
+         "0000000000000000000000000000000000000000000000000000"))))
 
 (define (vcs->origin vcs-type vcs-repo-url version subdir)
   "Generate the `origin' block of a package depending on what type of source
-- 
2.46.0

Message #11 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 74481 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH 1/2] git: Catch Git errors when updating cached checkout.
Date: Fri, 22 Nov 2024 20:09:26 +0100

* guix/git.scm (resolve-reference): Catch Git error when reference does not
exist and return #false.
(switch-to-ref): Adjust.
(update-cached-checkout)[close-and-clean!]: New helper.
Catch Git error when reference does not exist and warn.
Return #false values when reference does not exist.

Change-Id: If6e244fe40ebee978ec8de51a6a68bcbd4a2c79e
---
 guix/git.scm | 128 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 53 deletions(-)

diff --git a/guix/git.scm b/guix/git.scm
index 410cd4c153..328e2d5c8c 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -5,7 +5,7 @@
 ;;; Copyright © 2021 Marius Bakke <marius <at> gnu.org>
 ;;; Copyright © 2022 Maxime Devos <maximedevos <at> telenet.be>
 ;;; Copyright © 2023 Tobias Geerinckx-Rice <me <at> tobias.gr>
-;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
+;;; Copyright © 2023, 2024 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -265,7 +265,7 @@ (define (tag->commit repository tag)
 
 (define (resolve-reference repository ref)
   "Resolve the branch, commit or tag specified by REF, and return the
-corresponding Git object."
+corresponding Git object. Return #false if REF is not found."
   (let resolve ((ref ref))
     (match ref
       (('branch . branch)
@@ -281,9 +281,12 @@ (define (resolve-reference repository ref)
          ;; can't be sure it's available.  Furthermore, 'string->oid' used to
          ;; read out-of-bounds when passed a string shorter than 40 chars,
          ;; which is why we delay calls to it below.
-         (if (< len 40)
-             (object-lookup-prefix repository (string->oid commit) len)
-             (object-lookup repository (string->oid commit)))))
+         (catch 'git-error
+           (lambda ()
+             (if (< len 40)
+                 (object-lookup-prefix repository (string->oid commit) len)
+                 (object-lookup repository (string->oid commit))))
+           (lambda _ #f))))
       (('tag-or-commit . str)
        (cond ((and (string-contains str "-g")
                    (match (string-split str #\-)
@@ -300,7 +303,10 @@ (define (resolve-reference repository ref)
               => (lambda (commit) (resolve `(commit . ,commit))))
              ((or (> (string-length str) 40)
                   (not (string-every char-set:hex-digit str)))
-              (resolve `(tag . ,str)))      ;definitely a tag
+              (catch 'git-error         ;definitely a tag
+                (lambda ()
+                  (resolve `(tag . ,str)))
+                (lambda _ #f)))
              (else
               (catch 'git-error
                 (lambda ()
@@ -336,13 +342,15 @@ (define (switch-to-ref repository ref)
   (define obj
     (resolve-reference repository ref))
 
-  (reset repository obj RESET_HARD)
+  (and obj
+       (begin
+         (reset repository obj RESET_HARD)
 
-  ;; There might still be untracked files in REPOSITORY due to an interrupted
-  ;; checkout for example; delete them.
-  (delete-untracked-files repository)
+         ;; There might still be untracked files in REPOSITORY due to an interrupted
+         ;; checkout for example; delete them.
+         (delete-untracked-files repository)
 
-  (object-id obj))
+         (object-id obj))))
 
 (define (call-with-repository directory proc)
   (let ((repository #f))
@@ -562,6 +570,31 @@ (define* (update-cached-checkout url
                    (string-append directory "/" file)))
                 (or (scandir directory) '())))
 
+  (define (close-and-clean! repository)
+    (repository-close! repository)
+
+    ;; Update CACHE-DIRECTORY's mtime to so the cache logic sees it.
+    (match (gettimeofday)
+      ((seconds . microseconds)
+       (let ((nanoseconds (* 1000 microseconds)))
+         (utime cache-directory
+                seconds seconds
+                nanoseconds nanoseconds))))
+
+    ;; Run 'git gc' if needed.
+    (maybe-run-git-gc cache-directory)
+
+    ;; When CACHE-DIRECTORY is a sub-directory of the default cache
+    ;; directory, remove expired checkouts that are next to it.
+    (let ((parent (dirname cache-directory)))
+      (when (string=? parent (%repository-cache-directory))
+        (maybe-remove-expired-cache-entries parent cache-entries
+                                            #:entry-expiration
+                                            cached-checkout-expiration
+                                            #:delete-entry delete-checkout
+                                            #:cleanup-period
+                                            %checkout-cache-cleanup-period))))
+
   (define canonical-ref
     ;; We used to require callers to specify "origin/" for each branch, which
     ;; made little sense since the cache should be transparent to them.  So
@@ -583,56 +616,45 @@ (define* (update-cached-checkout url
      ;; Only fetch remote if it has not been cloned just before.
      (when (and cache-exists?
                 (not (reference-available? repository ref)))
-       (remote-fetch (remote-lookup repository "origin")
-                     #:fetch-options (make-default-fetch-options)))
+       (catch 'git-error
+         (lambda ()
+           (remote-fetch (remote-lookup repository "origin")
+                         #:fetch-options (make-default-fetch-options)))
+         (lambda _
+           (warning (G_ "Git cannot update the cache of ~a~%") url))))
      (when recursive?
        (update-submodules repository #:log-port log-port
                           #:fetch-options (make-default-fetch-options)))
 
      ;; Note: call 'commit-relation' from here because it's more efficient
      ;; than letting users re-open the checkout later on.
-     (let* ((oid      (if check-out?
+     (let ((oid      (if check-out?
                           (switch-to-ref repository canonical-ref)
                           (object-id
-                           (resolve-reference repository canonical-ref))))
-            (new      (and starting-commit
-                           (commit-lookup repository oid)))
-            (old      (and starting-commit
-                           (false-if-git-not-found
-                            (commit-lookup repository
-                                           (string->oid starting-commit)))))
-            (relation (and starting-commit
-                           (if old
-                               (commit-relation old new)
-                               'unrelated))))
-
-       ;; Reclaim file descriptors and memory mappings associated with
-       ;; REPOSITORY as soon as possible.
-       (repository-close! repository)
-
-       ;; Update CACHE-DIRECTORY's mtime to so the cache logic sees it.
-       (match (gettimeofday)
-         ((seconds . microseconds)
-          (let ((nanoseconds (* 1000 microseconds)))
-            (utime cache-directory
-                   seconds seconds
-                   nanoseconds nanoseconds))))
-
-       ;; Run 'git gc' if needed.
-       (maybe-run-git-gc cache-directory)
-
-       ;; When CACHE-DIRECTORY is a sub-directory of the default cache
-       ;; directory, remove expired checkouts that are next to it.
-       (let ((parent (dirname cache-directory)))
-         (when (string=? parent (%repository-cache-directory))
-           (maybe-remove-expired-cache-entries parent cache-entries
-                                               #:entry-expiration
-                                               cached-checkout-expiration
-                                               #:delete-entry delete-checkout
-                                               #:cleanup-period
-                                               %checkout-cache-cleanup-period)))
-
-       (values cache-directory (oid->string oid) relation)))))
+                           (resolve-reference repository canonical-ref)))))
+       (if (not oid)
+           (begin
+             (warning (G_ "Git cannot find the reference ~a~%") (match canonical-ref
+                                                                  ((_ . ref) ref)))
+             (close-and-clean! repository)
+             (values #f #f #f))
+           (let* ((new      (and starting-commit
+                                 (commit-lookup repository oid)))
+                  (old      (and starting-commit
+                                 (false-if-git-not-found
+                                  (commit-lookup repository
+                                                 (string->oid starting-commit)))))
+                  (relation (and starting-commit
+                                 (if old
+                                     (commit-relation old new)
+                                     'unrelated))))
+
+             ;; Reclaim file descriptors and memory mappings associated with
+             ;; REPOSITORY as soon as possible.
+
+             (close-and-clean! repository)
+
+             (values cache-directory (oid->string oid) relation)))))))
 
 (define* (latest-repository-commit store url
                                    #:key
-- 
2.46.0

Message #14 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 74481 <at> debbugs.gnu.org
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Sharlatan Hellseher <sharlatanus <at> gmail.com>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Katherine Cox-Buday <cox.katherine.e+guix <at> gmail.com>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'
Date: Tue, 10 Dec 2024 15:09:48 +0100

Hi,

On Fri, 22 Nov 2024 at 20:05, Simon Tournier <zimon.toutoune <at> gmail.com> wrote:

> Please double check because it tweaks 'update-cached-checkout' which is used
> by "guix pull". :-)

If no more comment on that, I will push shortly.

Cheers,
simon

Message #17 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Sharlatan Hellseher <sharlatanus <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Katherine Cox-Buday <cox.katherine.e+guix <at> gmail.com>,
 Christopher Baines <guix <at> cbaines.net>, 74481 <at> debbugs.gnu.org
Subject: Re: [bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'
Date: Tue, 10 Dec 2024 18:40:02 +0000

[Message part 1 (text/plain, inline)]

Hi,

If it solves import issues let's have it merged.
I've not faced with such an issue before after importing 1k packages,
maybe it's just specific for some particular ones.

I've got go-build-system modifications pending to review,
improving test coverage for subdirs and relaxing build noise.

Your patches are LGFM.

Thanks,
Oleg

[Message part 2 (text/html, inline)]

Message #20 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 74481 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Thu, 12 Dec 2024 12:34:57 +0100

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> * guix/git.scm (resolve-reference): Catch Git error when reference does not
> exist and return #false.
> (switch-to-ref): Adjust.
> (update-cached-checkout)[close-and-clean!]: New helper.
> Catch Git error when reference does not exist and warn.
> Return #false values when reference does not exist.
>
> Change-Id: If6e244fe40ebee978ec8de51a6a68bcbd4a2c79e

[...]

>  (define (resolve-reference repository ref)
>    "Resolve the branch, commit or tag specified by REF, and return the
> -corresponding Git object."
> +corresponding Git object. Return #false if REF is not found."
>    (let resolve ((ref ref))
>      (match ref
>        (('branch . branch)
> @@ -281,9 +281,12 @@ (define (resolve-reference repository ref)
>           ;; can't be sure it's available.  Furthermore, 'string->oid' used to
>           ;; read out-of-bounds when passed a string shorter than 40 chars,
>           ;; which is why we delay calls to it below.
> -         (if (< len 40)
> -             (object-lookup-prefix repository (string->oid commit) len)
> -             (object-lookup repository (string->oid commit)))))
> +         (catch 'git-error
> +           (lambda ()
> +             (if (< len 40)
> +                 (object-lookup-prefix repository (string->oid commit) len)
> +                 (object-lookup repository (string->oid commit))))
> +           (lambda _ #f))))
>        (('tag-or-commit . str)
>         (cond ((and (string-contains str "-g")
>                     (match (string-split str #\-)
> @@ -300,7 +303,10 @@ (define (resolve-reference repository ref)
>                => (lambda (commit) (resolve `(commit . ,commit))))
>               ((or (> (string-length str) 40)
>                    (not (string-every char-set:hex-digit str)))
> -              (resolve `(tag . ,str)))      ;definitely a tag
> +              (catch 'git-error         ;definitely a tag
> +                (lambda ()
> +                  (resolve `(tag . ,str)))
> +                (lambda _ #f)))

Sorry for not replynig earlier.  I would avoid such a change: it changes
the semantics of ‘resolve-reference’ in a non-trivial and hard-to-test
way.

Instead, since the goal is to address a problem that’s specific to
importers (or to one importer), I would suggest making a local change in
the importer itself, or in code that is shared among importers only.

(This comment applies to this entire patch.)

Ludo’.

Message #23 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 74481 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Thu, 12 Dec 2024 13:04:08 +0100

[Message part 1 (text/plain, inline)]

Hi Ludo,

On Thu, 12 Dec 2024 at 12:35, Ludovic Courtès <ludo <at> gnu.org> wrote:

> Sorry for not replynig earlier.  I would avoid such a change: it changes
> the semantics of ‘resolve-reference’ in a non-trivial and hard-to-test
> way.

Could you explain more what you mean for "the semantics" of
'resolve-reference'?

> Instead, since the goal is to address a problem that’s specific to
> importers (or to one importer), I would suggest making a local change in
> the importer itself, or in code that is shared among importers only.

Well, from my understanding this suggestion is not possible because of the
way it's implemented.  Or the importer needs a rewrite.  The crash comes
from the call of 'update-cached-checkout' and it appears to me impossible
to catch the error at the level of the importer because it's too late.

If you think that's possible to have a local change somewhere in the
importers, please point me because I'm clueless. :-)

BTW, the bug of looking for an non-existent reference still remains.  In
other words, all the calls to Guile-Git as 'object-lookup repository'
should be protected with some error-catch, IMHO.

Cheers,
simon

[Message part 2 (text/html, inline)]

Message #26 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 74481 <at> debbugs.gnu.org, Josselin Poiret <dev <at> jpoiret.xyz>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Christopher Baines <guix <at> cbaines.net>
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Tue, 17 Dec 2024 11:55:25 +0100

HI,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Thu, 12 Dec 2024 at 12:35, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> Sorry for not replynig earlier.  I would avoid such a change: it changes
>> the semantics of ‘resolve-reference’ in a non-trivial and hard-to-test
>> way.
>
> Could you explain more what you mean for "the semantics" of
> 'resolve-reference'?

We have something that can now return #f instead of throwing an
exception.  The many users of this interface are not prepared for this;
worse, getting #f instead of an exception means we lose information as
to why ‘update-cached-checkout’ or similar failed.

>> Instead, since the goal is to address a problem that’s specific to
>> importers (or to one importer), I would suggest making a local change in
>> the importer itself, or in code that is shared among importers only.

[...]

> If you think that's possible to have a local change somewhere in the
> importers, please point me because I'm clueless. :-)

I haven’t looked in detail, so I’m starting from the use case at the
beginning of this thread:

--8<---------------cut here---------------start------------->8---
$ guix import go software.sslmate.com/src/go-pkcs12
guix import: Importing package "software.sslmate.com/src/go-pkcs12"...
guix import: warning: Unable to determine repository root of 'software.sslmate.com/src/go-pkcs12'. Guessing 'software.sslmate.com/src/go-pkcs12'.
SWH: found revision fa70679f0f1622a2705336a97225ee8d6c555f96 with directory at 'https://archive.softwareheritage.org/api/1/directory/43d3e9f7167cdcd9f179e8d1de33d0d22e8fbd46/'
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/HEAD
swh:1:rev:fa70679f0f1622a2705336a97225ee8d6c555f96.git/branches/

[...]

In guix/scripts/import/go.scm:
   116:29  4 (_)
In ice-9/eval.scm:
    619:8  3 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   174:20  2 (_ #(#(#<directory (guix import go) 7fa85b580d20> # ?) ?))
   177:32  1 (lp (#<procedure 7fa8598a1380 at ice-9/eval.scm:191:12?>))
In ice-9/boot-9.scm:
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure git-error-message: Wrong type argument: #<&compound-exception components: (#<&error> #<&irritants irritants: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)> #<&exception-with-kind-and-args kind: git-error args: (#<<git-error> code: -3 message: "reference 'refs/tags/v0.5.0' not found" class: 4>)>)>
--8<---------------cut here---------------end--------------->8---

Would it be an option to catch 'git-error around the
‘update-cached-checkout’ call in (guix import go)?  If an exception is
thrown, it would print a warning and return a fake hash.

Ludo’.

Message #29 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 74481 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH v2] import: go: Warn instead of error out when Git fails.
Date: Tue, 17 Dec 2024 18:45:02 +0100

* guix/import/go.scm (git-checkout-hash): Return some hash although updating
the cache fails.

Change-Id: I87e7701023a5f76f5d1494827473616e6a1275aa
---
 guix/import/go.scm | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/guix/import/go.scm b/guix/import/go.scm
index 32cba25b33..6ede152601 100644
--- a/guix/import/go.scm
+++ b/guix/import/go.scm
@@ -38,7 +38,8 @@ (define-module (guix import go)
   #:use-module (guix http-client)
   #:use-module (guix memoization)
   #: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)
@@ -563,10 +564,18 @@ (define* (git-checkout-hash url reference algorithm)
   (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 ()
+                      (update-cached-checkout url
+                                              #:ref
+                                              `(tag-or-commit . ,reference)))
+                    (lambda (key err)
+                      (warning (G_ "update-cached-checkout: ~s ~s~%") key err)
+                      (values #f #f #f))))))
+        (if (and checkout commit)
+            (file-hash* checkout #:algorithm algorithm #:recursive? #true)
+            (nix-base32-string->bytevector
+             "0000000000000000000000000000000000000000000000000000"))))
 
 (define (vcs->origin vcs-type vcs-repo-url version subdir)
   "Generate the `origin' block of a package depending on what type of source

base-commit: a9003b8e6b40b59c9545ae87bb441d3549630db7
-- 
2.45.2

Message #32 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Christopher Baines <guix <at> cbaines.net>, 74481 <at> debbugs.gnu.org,
 Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Mathieu Othacehe <othacehe <at> gnu.org>
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Tue, 17 Dec 2024 19:06:14 +0100

Hi Ludo,

On Tue, 17 Dec 2024 at 11:55, Ludovic Courtès <ludo <at> gnu.org> wrote:

> We have something that can now return #f instead of throwing an
> exception.  The many users of this interface are not prepared for this;
> worse, getting #f instead of an exception means we lose information as
> to why ‘update-cached-checkout’ or similar failed.

Thanks for explaining.

Well, I’d say “mouais“ :-) about « lose information » since I added some
’pk’ here or there before finding the relevant piece of code.  So the
exception is not very helpful, not to say fully useless, IMHO.

That’s said, I understand your concern and I will not discuss it
here. :-) If I read you correctly and apply the argument, you propose to
guard all the calls of ’update-cached-checkout’, it means:

--8<---------------cut here---------------start------------->8---
./guix/import/egg.scm:94:                  (directory commit _ (update-cached-checkout url)))
./guix/scripts/system/reconfigure.scm:355: (update-cached-checkout
./guix/git.scm:670:                        (update-cached-checkout url
./guix/inferior.scm:877:                   (update-cached-checkout (channel-url channel)
--8<---------------cut here---------------end--------------->8---

Either using ‘(catch 'git-error’ or either using
‘(with-git-error-handling’.

Right?

> Would it be an option to catch 'git-error around the
> ‘update-cached-checkout’ call in (guix import go)?  If an exception is
> thrown, it would print a warning and return a fake hash.

See v2 for this case of ‘guix import go’.

Cheers,
simon

PS: As I said earlier, I think all the call to Guile-Git should be
guarded and each call should manage the exception instead of propagate
it… and so barely catch them. :-)

Message #35 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Sharlatan Hellseher <sharlatanus <at> gmail.com>
To: 74481 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH 0/2] Handle corner cases of 'guix import go'
Date: Wed, 18 Dec 2024 20:18:01 +0000

[Message part 1 (text/plain, inline)]

Hi Simon,

With your patch applied I could import 971 packages' templates which
100% cover this tasks!

--8<---------------cut here---------------start------------->8---
(define-public sqls
  (package
    (name "sqls")
    ;; TODO: The latest version requires a way more packages to be available
    ;; in Guix.
    (version "0.2.18")
    (source (origin
              (method git-fetch)
              (uri (git-reference
                    (url "https://github.com/sqls-server/sqls")
                    (commit (string-append "v" version))))
              (file-name (git-file-name name version))
              (sha256
               (base32
                "13837v27avdp2nls3vyy7ml12nj7rxragchwf92adn10ffp4aj6c"))))
--8<---------------cut here---------------end--------------->8---

Packages were not tested but importer did not break and completed after
2h.

--
Oleg

[signature.asc (application/pgp-signature, inline)]

Message #38 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 74481 <at> debbugs.gnu.org, Katherine Cox-Buday <cox.katherine.e+guix <at> gmail.com>,
 Sharlatan Hellseher <sharlatanus <at> gmail.com>
Subject: Re: [bug#74481] [PATCH v2] import: go: Warn instead of error out
 when Git fails.
Date: Thu, 26 Dec 2024 22:14:41 +0100

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> * guix/import/go.scm (git-checkout-hash): Return some hash although updating
> the cache fails.
>
> Change-Id: I87e7701023a5f76f5d1494827473616e6a1275aa

[...]

> +                  (catch 'git-error
> +                    (lambda ()
> +                      (update-cached-checkout url
> +                                              #:ref
> +                                              `(tag-or-commit . ,reference)))
> +                    (lambda (key err)
> +                      (warning (G_ "update-cached-checkout: ~s ~s~%") key err)

Rather something like:

  (warning (G_ "failed to check out ~s from Git repository at '~a': ~a~%")
           reference url (git-error-message err))

Apart from that LGTM!

Thanks,
Ludo’.

Message #41 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Christopher Baines <guix <at> cbaines.net>, Josselin Poiret <dev <at> jpoiret.xyz>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Mathieu Othacehe <othacehe <at> gnu.org>,
 74481 <at> debbugs.gnu.org
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Thu, 26 Dec 2024 22:17:32 +0100

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On Tue, 17 Dec 2024 at 11:55, Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> We have something that can now return #f instead of throwing an
>> exception.  The many users of this interface are not prepared for this;
>> worse, getting #f instead of an exception means we lose information as
>> to why ‘update-cached-checkout’ or similar failed.
>
> Thanks for explaining.
>
> Well, I’d say “mouais“ :-) about « lose information » since I added some
> ’pk’ here or there before finding the relevant piece of code.  So the
> exception is not very helpful, not to say fully useless, IMHO.

The exception conveys information about the failure: no space left on
device, host unreachable, invalid certificate, etc.

Often ‘with-git-error-handling’ is used at the top-level because the
error is not recoverable so we just want to print an error message and
exit.

In other cases, we explicitly want to catch, say, GIT_ENOTFOUND because
there’s a fallback path in this case.

Ludo’.

Message #44 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 74481 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net>,
 Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Mathieu Othacehe <othacehe <at> gnu.org>
Subject: Re: [bug#74481] [PATCH 1/2] git: Catch Git errors when updating
 cached checkout.
Date: Tue, 31 Dec 2024 16:25:43 +0100

Hi Ludo,

On Thu, 26 Dec 2024 at 22:17, Ludovic Courtès <ludo <at> gnu.org> wrote:

> In other cases, we explicitly want to catch, say, GIT_ENOTFOUND because
> there’s a fallback path in this case.

Ah, I see.  Thanks for explaining; all clear now! :-)

Cheers,
simon

Message #47 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Sharlatan Hellseher <sharlatanus <at> gmail.com>, 74481 <at> debbugs.gnu.org
Subject: Re: [bug#74481] [PATCH 0/2] Handle corner cases of 'guix import go'
Date: Mon, 06 Jan 2025 12:26:09 +0100

Hi Oleg,

On Wed, 18 Dec 2024 at 20:18, Sharlatan Hellseher <sharlatanus <at> gmail.com> wrote:

> Packages were not tested but importer did not break and completed after
> 2h.

Oh, cool!

Cheers,
simon

Message #50 received at 74481 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 74481 <at> debbugs.gnu.org, Katherine Cox-Buday <cox.katherine.e+guix <at> gmail.com>,
 Sharlatan Hellseher <sharlatanus <at> gmail.com>
Subject: Re: [bug#74481] [PATCH v2] import: go: Warn instead of error out
 when Git fails.
Date: Mon, 06 Jan 2025 12:25:29 +0100

Hi,

On Thu, 26 Dec 2024 at 22:14, Ludovic Courtès <ludo <at> gnu.org> wrote:

> Apart from that LGTM!

Thanks, pushed as 57fa06352e9f4019298ffda3c066bab7538c8906.

Cheers,
simon

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.